🏗️ Refactor service release compatibility checks and enhance metadata inheritance tests

Author: pcrespov

services/catalog/src/simcore_service_catalog/service/access_rights.py

@@ -123,38 +123,34 @@ async def evaluate_default_service_ownership_and_rights(
     return (owner_gid, default_access_rights)
 
 
-async def _find_previous_compatible_release(
+async def _find_latest_patch_compatible_release(
     services_repo: ServicesRepository, *, service_metadata: ServiceMetaDataPublished
 ) -> ServiceMetaDataDBGet | None:
     """
-    Finds the previous compatible release for a service.
+    Finds the previous patched release for a service.
 
     Args:
         services_repo: Instance of ServicesRepository for database access.
         service_metadata: Metadata of the service being evaluated.
 
     Returns:
-        The previous compatible release if found, None otherwise.
+        Latest patch release of the service if it exists, otherwise None.
     """
     if _is_frontend_service(service_metadata):
         return None
 
     new_version: Version = as_version(service_metadata.version)
-    latest_releases = await services_repo.list_service_releases(
+    patch_releases_latest_first = await services_repo.list_service_releases(
         service_metadata.key,
         major=new_version.major,
         minor=new_version.minor,
-        limit_count=5,
     )
 
-    # FIXME: deprecated versions hsould not coutn!!!
-
     # latest_releases is sorted from newer to older
-    for release in latest_releases:
+    for release in patch_releases_latest_first:
         # COMPATIBILITY RULE:
         # - a patch release is compatible with the previous patch release
-
-        # FIXME: not all compatible releases!!!
+        # - WARNING: this does not account for custom compatibility policies!!!!
         if is_patch_release(new_version, release.version):
             return release
 
@@ -169,7 +165,7 @@ async def inherit_from_latest_compatible_release(
 
     This function applies inheritance policies:
     - AUTO-UPGRADE PATCH policy: new patch releases inherit access rights from previous compatible versions
-    - Metadata inheritance: icon and other metadata fields are inherited if not specified in the new version
+    - Metadata inheritance: icon and thumbnail fields are inherited if not specified in the new version
 
     Args:
         services_repo: Instance of ServicesRepository for database access.
@@ -189,7 +185,7 @@ async def inherit_from_latest_compatible_release(
         "metadata_updates": {},
     }
 
-    previous_release = await _find_previous_compatible_release(
+    previous_release = await _find_latest_patch_compatible_release(
         services_repo, service_metadata=service_metadata
     )
 
@@ -210,10 +206,12 @@ async def inherit_from_latest_compatible_release(
         for access in previous_access_rights
     ]
 
-    # 2. METADATA:
-    #    Inherit icon if not specified in the new service
+    # 2. ServiceMetaDataPublished
+    #    Inherit some fields if not specified in the new service
     if not service_metadata.icon and previous_release.icon:
         inherited_data["metadata_updates"]["icon"] = previous_release.icon
+    if not service_metadata.thumbnail and previous_release.thumbnail:
+        inherited_data["metadata_updates"]["thumbnail"] = previous_release.thumbnail
 
     return inherited_data
 

services/catalog/tests/unit/with_dbs/test_service_access_rights.py

@@ -3,7 +3,7 @@
 # pylint: disable=unused-variable
 
 from collections.abc import Callable
-from typing import Any
+from typing import Any, Protocol
 
 import pytest
 import simcore_service_catalog.service.access_rights
@@ -43,7 +43,9 @@ def new_service_metadata_published(user: dict[str, Any]) -> ServiceMetaDataPubli
         Author(name=user["name"], email=user["email"], affiliation=None)
     ]
     metadata.version = TypeAdapter(ServiceVersion).validate_python("1.0.11")
-    metadata.icon = None  # Remove icon to test inheritance
+    # Set all inheritable fields to None initially
+    metadata.icon = None
+    metadata.thumbnail = None
     return metadata
 
 
@@ -63,6 +65,47 @@ def app_with_repo(
     return app, services_repo
 
 
+class CreateLatetReleaseCallable(Protocol):
+    """Callable to create a latest release with specified metadata fields."""
+
+    async def __call__(self, metadata_fields: dict[str, Any]) -> dict[str, Any]: ...
+
+
+@pytest.fixture
+def create_latest_release(
+    create_fake_service_data: CreateFakeServiceDataCallable,
+    new_service_metadata_published: ServiceMetaDataPublished,
+    target_product: ProductName,
+    services_db_tables_injector: Callable,
+) -> CreateLatetReleaseCallable:
+    """Creates a latest release with specified metadata fields."""
+
+    from packaging.version import Version
+
+    new_version = Version(new_service_metadata_published.version)
+
+    async def _create(metadata_fields: dict[str, Any]) -> dict[str, Any]:
+        latest_release_service, *latest_release_service_access_rights = (
+            create_fake_service_data(
+                new_service_metadata_published.key,
+                f"{new_version.major}.{new_version.minor}.{new_version.micro-1}",
+                team_access="x",
+                everyone_access=None,
+                product=target_product,
+            )
+        )
+
+        # Update with provided metadata fields
+        for field, value in metadata_fields.items():
+            latest_release_service[field] = value
+
+        latest_release = (latest_release_service, *latest_release_service_access_rights)
+        await services_db_tables_injector([latest_release])
+        return latest_release_service
+
+    return _create
+
+
 def test_reduce_access_rights():
     sample = ServiceAccessRightsDB.model_validate(
         {
@@ -118,14 +161,105 @@ def test_reduce_access_rights():
     }
 
 
+async def test_metadata_inheritance_variations(
+    new_service_metadata_published: ServiceMetaDataPublished,
+    app_with_repo: tuple[FastAPI, ServicesRepository],
+    create_latest_release: CreateLatetReleaseCallable,
+):
+    """Test different variations of metadata inheritance with complete previous release."""
+    app, services_repo = app_with_repo
+
+    # Set up a previous release with all metadata fields
+    latest_release_service = await create_latest_release(
+        {
+            "icon": "https://foo/previous_icon.svg",
+            "thumbnail": "https://foo/previous_thumbnail.jpg",
+        }
+    )
+
+    # Case 1: All fields missing in new service - only icon and thumbnail should be inherited
+    new_service = new_service_metadata_published.model_copy(deep=True)
+    inherited_data = await inherit_from_latest_compatible_release(
+        services_repo, service_metadata=new_service
+    )
+
+    assert inherited_data["metadata_updates"] == {
+        "icon": latest_release_service["icon"],
+        "thumbnail": latest_release_service["thumbnail"],
+    }
+
+    # Case 2: Some fields present, others missing
+    new_service = new_service_metadata_published.model_copy(
+        deep=True,
+        update={
+            "thumbnail": None,  # Missing field
+            "icon": "https://foo/new_icon.svg",
+        },
+    )
+
+    inherited_data = await inherit_from_latest_compatible_release(
+        services_repo, service_metadata=new_service
+    )
+
+    # Only thumbnail should be inherited
+    assert "icon" not in inherited_data["metadata_updates"]
+    assert inherited_data["metadata_updates"] == {
+        "thumbnail": latest_release_service["thumbnail"],
+    }
+
+    # Case 3: All fields present in new service - nothing should be inherited
+    new_service = new_service_metadata_published.model_copy(
+        deep=True,
+        update={
+            "icon": "https://foo/new_icon.svg",
+            "thumbnail": "https://foo/new_thumbnail.jpg",
+        },
+    )
+
+    inherited_data = await inherit_from_latest_compatible_release(
+        services_repo, service_metadata=new_service
+    )
+
+    # No metadata should be inherited
+    assert inherited_data["metadata_updates"] == {}
+
+
+async def test_metadata_inheritance_with_incomplete_previous_release(
+    new_service_metadata_published: ServiceMetaDataPublished,
+    app_with_repo: tuple[FastAPI, ServicesRepository],
+    create_latest_release: CreateLatetReleaseCallable,
+):
+    """Test metadata inheritance when previous release has incomplete metadata fields."""
+    app, services_repo = app_with_repo
+
+    # Case 4: Previous release missing some fields
+    latest_release_service = await create_latest_release(
+        {
+            "icon": "https://foo/previous_icon.svg",
+            "thumbnail": "https://foo/previous_thumbnail.jpg",
+        }
+    )
+
+    new_service = new_service_metadata_published.model_copy(deep=True)
+    inherited_data = await inherit_from_latest_compatible_release(
+        services_repo, service_metadata=new_service
+    )
+
+    # Only icon and thumbnail should be inherited
+    assert inherited_data["metadata_updates"] == {
+        "icon": latest_release_service["icon"],
+        "thumbnail": latest_release_service["thumbnail"],
+    }
+    assert "description" not in inherited_data["metadata_updates"]
+
+
 async def test_service_upgrade_metadata_inheritance_old_service(
     user_groups_ids: list[GroupID],
     target_product: ProductName,
-    services_db_tables_injector: Callable,
-    create_fake_service_data: CreateFakeServiceDataCallable,
     mocker: MockerFixture,
     new_service_metadata_published: ServiceMetaDataPublished,
     app_with_repo: tuple[FastAPI, ServicesRepository],
+    create_latest_release: CreateLatetReleaseCallable,
 ):
     """Test inheritance behavior when the service is considered old"""
     everyone_gid, user_gid, team_gid = user_groups_ids
@@ -138,22 +272,15 @@ async def test_service_upgrade_metadata_inheritance_old_service(
         return_value=True,
     )
 
-    # Create latest-release service for testing inheritance
-    latest_release_service, *latest_release_service_access_rights = (
-        create_fake_service_data(
-            new_service_metadata_published.key,
-            "1.0.10",
-            team_access="x",
-            everyone_access=None,
-            product=target_product,
-        )
+    # Create latest-release service for testing inheritance with all metadata fields
+    latest_release_service = await create_latest_release(
+        {
+            "icon": "https://foo/previous_icon.svg",
+            "description": "Previous description",  # This won't be inherited
+            "thumbnail": "https://foo/previous_thumbnail.jpg",
+        }
     )
 
-    latest_release_service["icon"] = "https://foo/previous_icon.svg"
-    latest_release = (latest_release_service, *latest_release_service_access_rights)
-
-    await services_db_tables_injector([latest_release])
-
     # DEFAULT policies for old service
     owner_gid, service_access_rights = (
         await evaluate_default_service_ownership_and_rights(
@@ -185,10 +312,13 @@ async def test_service_upgrade_metadata_inheritance_old_service(
         services_repo, service_metadata=new_service_metadata_published
     )
 
-    # Check metadata inheritance
+    # Check metadata inheritance - only icon and thumbnail should be inherited
     inherited_metadata = inherited_data["metadata_updates"]
-    assert "icon" in inherited_metadata
-    assert inherited_metadata["icon"] == latest_release_service["icon"]
+    assert "description" not in inherited_metadata
+    assert inherited_metadata == {
+        "icon": latest_release_service["icon"],
+        "thumbnail": latest_release_service["thumbnail"],
+    }
 
 
 async def test_service_upgrade_metadata_inheritance_new_service_multi_product(