Allow Template upgrade properties to be set and remove properties that no longer exist.

CHANGELOG.md

@@ -9,6 +9,7 @@ ENHANCEMENTS:
 * API: Replace HTTP_422_UNPROCESSABLE_ENTITY response with HTTP_422_UNPROCESSABLE_CONTENT as per RFC 9110 ([#4742](https://github.com/microsoft/AzureTRE/issues/4742))
 * Change Group.ReadWrite.All permission to Group.Create for AUTO_WORKSPACE_GROUP_CREATION ([#4772](https://github.com/microsoft/AzureTRE/issues/4772))
 * Make workspace shared storage quota updateable ([#4314](https://github.com/microsoft/AzureTRE/issues/4314))
+* Allow new template properties to be specified during template upgrades. Remove Template properties that no longer exist.
 * Update Porter, AzureCLI, Terraform and its providers across the solution ([#4799](https://github.com/microsoft/AzureTRE/issues/4799))
 
 BUG FIXES:

api_app/_version.py

@@ -1 +1 @@
-__version__ = "0.25.7"
+__version__ = "0.25.8"

api_app/db/repositories/resources.py

@@ -111,6 +111,16 @@ async def validate_input_against_template(self, template_name: str, resource_inp
 
         return parse_obj_as(ResourceTemplate, template)
 
+    def _get_all_property_keys_from_template(self, resource_template: ResourceTemplate) -> set:
+        properties = set(resource_template.properties.keys())
+        if "allOf" in resource_template:
+            for condition in resource_template.allOf:
+                if "then" in condition and "properties" in condition["then"]:
+                    properties.update(condition["then"]["properties"].keys())
+                if "else" in condition and "properties" in condition["else"]:
+                    properties.update(condition["else"]["properties"].keys())
+        return properties
+
     async def patch_resource(self, resource: Resource, resource_patch: ResourcePatch, resource_template: ResourceTemplate, etag: str, resource_template_repo: ResourceTemplateRepository, resource_history_repo: ResourceHistoryRepository, user: User, resource_action: str, force_version_update: bool = False) -> Tuple[Resource, ResourceTemplate]:
         await resource_history_repo.create_resource_history_item(resource)
         # now update the resource props
@@ -123,10 +133,20 @@ async def patch_resource(self, resource: Resource, resource_patch: ResourcePatch
 
         if resource_patch.templateVersion is not None:
             await self.validate_template_version_patch(resource, resource_patch, resource_template_repo, resource_template, force_version_update)
+            new_template = await resource_template_repo.get_template_by_name_and_version(resource.templateName, resource_patch.templateVersion, resource.resourceType)
+
+            old_properties = self._get_all_property_keys_from_template(resource_template)
+            new_properties = self._get_all_property_keys_from_template(new_template)
+
+            properties_to_remove = old_properties - new_properties
+            for prop in properties_to_remove:
+                if prop in resource.properties:
+                    del resource.properties[prop]
+
             resource.templateVersion = resource_patch.templateVersion
 
         if resource_patch.properties is not None and len(resource_patch.properties) > 0:
-            self.validate_patch(resource_patch, resource_template_repo, resource_template, resource_action)
+            await self.validate_patch(resource_patch, resource_template_repo, resource_template, resource_action)
 
             # if we're here then we're valid - update the props + persist
             resource.properties.update(resource_patch.properties)
@@ -183,16 +203,39 @@ async def validate_template_version_patch(self, resource: Resource, resource_pat
         except EntityDoesNotExist:
             raise TargetTemplateVersionDoesNotExist(f"Template '{resource_template.name}' not found for resource type '{resource_template.resourceType}' with target template version '{resource_patch.templateVersion}'")
 
-    def validate_patch(self, resource_patch: ResourcePatch, resource_template_repo: ResourceTemplateRepository, resource_template: ResourceTemplate, resource_action: str):
+    async def validate_patch(self, resource_patch: ResourcePatch, resource_template_repo: ResourceTemplateRepository, resource_template: ResourceTemplate, resource_action: str):
         # get the enriched (combined) template
         enriched_template = resource_template_repo.enrich_template(resource_template, is_update=True)
 
-        # validate the PATCH data against a cut down version of the full template.
+        # get the old template properties for comparison during upgrades
+        old_template_properties = set(enriched_template["properties"].keys())
+
+        # get the schema for the target version if upgrade is happening
+        if resource_patch.templateVersion is not None:
+            # fetch the template for the target version
+            target_template = await resource_template_repo.get_template_by_name_and_version(resource_template.name, resource_patch.templateVersion, resource_template.resourceType)
+            enriched_template = resource_template_repo.enrich_template(target_template, is_update=True)
+
+        # validate the PATCH data against the target schema.
         update_template = copy.deepcopy(enriched_template)
         update_template["required"] = []
         update_template["properties"] = {}
         for prop_name, prop in enriched_template["properties"].items():
-            if (resource_action == RESOURCE_ACTION_INSTALL or prop.get("updateable", False) is True):
+            # Allow property if:
+            # 1. Installing (all properties allowed)
+            # 2. Property is explicitly updateable
+            # 3. Upgrading and the property is NEW (not in old template) and being added in this patch
+            if (
+                resource_action == RESOURCE_ACTION_INSTALL
+                or prop.get("updateable", False) is True
+                or (
+                    resource_patch.templateVersion is not None
+                    and resource_patch.templateVersion != resource_template.version
+                    and resource_patch.properties is not None
+                    and prop_name in resource_patch.properties
+                    and prop_name not in old_template_properties
+                )
+            ):
                 update_template["properties"][prop_name] = prop
 
         self._validate_resource_parameters(resource_patch.dict(), update_template)

api_app/tests_ma/test_db/test_repositories/test_resource_repository.py

@@ -4,6 +4,7 @@
 import pytest
 import pytest_asyncio
 from mock import patch, MagicMock
+from pydantic import parse_obj_as
 
 from jsonschema.exceptions import ValidationError
 from resources import strings
@@ -140,6 +141,58 @@ def sample_nested_template() -> ResourceTemplate:
     ).dict(exclude_none=True)
 
 
+def sample_resource_template_with_new_property(version: str = "0.2.0") -> dict:
+    """
+    Returns a template similar to sample_resource_template but with an additional
+    'new_property' that is not updateable. Useful for testing template upgrades.
+    """
+    return ResourceTemplate(
+        id="123",
+        name="tre-user-resource",
+        description="description",
+        version=version,
+        resourceType=ResourceType.UserResource,
+        current=True,
+        required=['os_image', 'title'],
+        properties={
+            'title': {
+                'type': 'string',
+                'title': 'Title of the resource'
+            },
+            'os_image': {
+                'type': 'string',
+                'title': 'Windows image',
+                'description': 'Select Windows image to use for VM',
+                'enum': [
+                    'Windows 10',
+                    'Server 2019 Data Science VM'
+                ],
+                'updateable': False
+            },
+            'vm_size': {
+                'type': 'string',
+                'title': 'VM Size',
+                'description': 'Select Windows image to use for VM',
+                'enum': [
+                    'small',
+                    'large'
+                ],
+                'updateable': True
+            },
+            'new_property': {
+                'type': 'string',
+                'title': 'New non-updateable property',
+                'enum': [
+                    'value1',
+                    'value2'
+                ],
+                'updateable': False
+            }
+        },
+        actions=[]
+    ).dict(exclude_none=True)
+
+
 @pytest.mark.asyncio
 @patch("db.repositories.resources.ResourceRepository._get_enriched_template")
 @patch("db.repositories.resources.ResourceRepository._validate_resource_parameters", return_value=None)
@@ -370,22 +423,24 @@ async def test_patch_resource_preserves_property_history(_, __, ___, resource_re
     resource_repo.update_item_with_etag.assert_called_with(expected_resource, etag)
 
 
+@pytest.mark.asyncio
 @patch('db.repositories.resources.ResourceTemplateRepository.enrich_template')
-def test_validate_patch_with_good_fields_passes(template_repo, resource_repo):
+async def test_validate_patch_with_good_fields_passes(template_repo, resource_repo):
     """
-    Make sure that patch is NOT valid when non-updateable fields are included
+    Make sure that patch is valid when updateable fields are included
     """
 
     template_repo.enrich_template = MagicMock(return_value=sample_resource_template())
     template = sample_resource_template()
 
     # check it's valid when updating a single updateable prop
     patch = ResourcePatch(isEnabled=True, properties={'vm_size': 'large'})
-    resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_UPDATE)
+    await resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_UPDATE)
 
 
+@pytest.mark.asyncio
 @patch('db.repositories.resources.ResourceTemplateRepository.enrich_template')
-def test_validate_patch_with_bad_fields_fails(template_repo, resource_repo):
+async def test_validate_patch_with_bad_fields_fails(template_repo, resource_repo):
     """
     Make sure that patch is NOT valid when non-updateable fields are included
     """
@@ -396,14 +451,121 @@ def test_validate_patch_with_bad_fields_fails(template_repo, resource_repo):
     # check it's invalid when sending an unexpected field
     patch = ResourcePatch(isEnabled=True, properties={'vm_size': 'large', 'unexpected_field': 'surprise!'})
     with pytest.raises(ValidationError):
-        resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_INSTALL)
+        await resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_UPDATE)
 
-    # check it's invalid when sending a bad value
+    # check it's invalid when sending a bad value (new install)
     patch = ResourcePatch(isEnabled=True, properties={'vm_size': 'huge'})
     with pytest.raises(ValidationError):
-        resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_INSTALL)
+        await resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_INSTALL)
+
+    # check it's invalid when sending a bad value (update)
+    patch = ResourcePatch(isEnabled=True, properties={'vm_size': 'huge'})
+    with pytest.raises(ValidationError):
+        await resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_UPDATE)
 
     # check it's invalid when trying to update a non-updateable field
     patch = ResourcePatch(isEnabled=True, properties={'vm_size': 'large', 'os_image': 'linux'})
     with pytest.raises(ValidationError):
-        resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_INSTALL)
+        await resource_repo.validate_patch(patch, template_repo, template, strings.RESOURCE_ACTION_UPDATE)
+
+
+@pytest.mark.asyncio
+@patch('db.repositories.resources.ResourceTemplateRepository.get_template_by_name_and_version')
+@patch('db.repositories.resources.ResourceTemplateRepository.enrich_template')
+async def test_validate_patch_allows_new_non_updateable_property_during_upgrade(enrich_template_mock, get_template_mock, resource_repo):
+    """
+    Test that during a template upgrade, new properties (not in old version) can be specified
+    even if they are marked as updateable: false in the new template version
+    """
+    # Old template has os_image and vm_size
+    old_template = sample_resource_template()
+    old_template['version'] = '0.1.0'
+
+    # New template adds a new property 'new_property' that is not updateable
+    new_template = sample_resource_template_with_new_property(version='0.2.0')
+
+    # Mock the template repository
+    template_repo = MagicMock()
+    template_repo.get_template_by_name_and_version = AsyncMock(return_value=parse_obj_as(ResourceTemplate, new_template))
+    template_repo.enrich_template = MagicMock(side_effect=[old_template, new_template])
+
+    # Patch includes the new property during upgrade - this should be ALLOWED
+    patch = ResourcePatch(templateVersion='0.2.0', properties={'new_property': 'value1'})
+
+    # This should NOT raise a ValidationError
+    await resource_repo.validate_patch(patch, template_repo, parse_obj_as(ResourceTemplate, old_template), strings.RESOURCE_ACTION_UPDATE)
+
+
+@pytest.mark.asyncio
+@patch('db.repositories.resources.ResourceTemplateRepository.get_template_by_name_and_version')
+@patch('db.repositories.resources.ResourceTemplateRepository.enrich_template')
+async def test_validate_patch_rejects_existing_non_updateable_property_during_upgrade(enrich_template_mock, get_template_mock, resource_repo):
+    """
+    Test that during a template upgrade, existing non-updateable properties still cannot be modified
+    """
+    # Old template has os_image (non-updateable) and vm_size (updateable)
+    old_template = sample_resource_template()
+
+    # New template is the same but version 0.2.0
+    new_template = copy.deepcopy(old_template)
+
+    # Mock the template repository
+    template_repo = MagicMock()
+    template_repo.get_template_by_name_and_version = AsyncMock(return_value=parse_obj_as(ResourceTemplate, new_template))
+    template_repo.enrich_template = MagicMock(side_effect=[old_template, new_template])
+
+    # Try to update existing non-updateable property during upgrade - this should FAIL
+    patch = ResourcePatch(templateVersion='0.2.0', properties={'os_image': 'Windows 10'})
+
+    with pytest.raises(ValidationError):
+        await resource_repo.validate_patch(patch, template_repo, parse_obj_as(ResourceTemplate, old_template), strings.RESOURCE_ACTION_UPDATE)
+
+
+@pytest.mark.asyncio
+@patch('db.repositories.resources.ResourceTemplateRepository.get_template_by_name_and_version')
+@patch('db.repositories.resources.ResourceTemplateRepository.enrich_template')
+async def test_validate_patch_allows_updateable_property_during_upgrade(enrich_template_mock, get_template_mock, resource_repo):
+    """
+    Test that during a template upgrade, updateable properties can still be modified
+    """
+    # Old template 0.1.0
+    old_template = sample_resource_template()
+
+    # New template 0.2.0
+    new_template = copy.deepcopy(old_template)
+
+    # Mock the template repository
+    template_repo = MagicMock()
+    template_repo.get_template_by_name_and_version = AsyncMock(return_value=parse_obj_as(ResourceTemplate, new_template))
+    template_repo.enrich_template = MagicMock(side_effect=[old_template, new_template])
+
+    # Update existing updateable property during upgrade - this should work
+    patch = ResourcePatch(templateVersion='0.2.0', properties={'vm_size': 'large'})
+
+    # This should NOT raise a ValidationError
+    await resource_repo.validate_patch(patch, template_repo, parse_obj_as(ResourceTemplate, old_template), strings.RESOURCE_ACTION_UPDATE)
+
+
+@pytest.mark.asyncio
+@patch('db.repositories.resources.ResourceTemplateRepository.get_template_by_name_and_version')
+@patch('db.repositories.resources.ResourceTemplateRepository.enrich_template')
+async def test_validate_patch_allows_mix_of_new_and_updateable_properties_during_upgrade(enrich_template_mock, get_template_mock, resource_repo):
+    """
+    Test that during upgrade, you can specify both new non-updateable properties and existing updateable properties
+    """
+    # Old template
+    old_template = sample_resource_template()
+
+    # New template adds new_property
+    new_template = sample_resource_template_with_new_property(version='0.2.0')
+
+    # Mock the template repository
+    template_repo = MagicMock()
+    template_repo.get_template_by_name_and_version = AsyncMock(return_value=parse_obj_as(ResourceTemplate, new_template))
+    template_repo.enrich_template = MagicMock(side_effect=[old_template, new_template])
+
+    # Patch with both new non-updateable property and existing updateable property
+    patch = ResourcePatch(templateVersion='0.2.0', properties={'new_property': 'value1', 'vm_size': 'large'})
+
+    # This should NOT raise a ValidationError
+    await resource_repo.validate_patch(patch, template_repo, parse_obj_as(ResourceTemplate, old_template), strings.RESOURCE_ACTION_UPDATE)

ui/app/package.json

@@ -1,6 +1,6 @@
 {
   "name": "tre-ui",
-  "version": "0.8.20",
+  "version": "0.8.21",
   "private": true,
   "type": "module",
   "dependencies": {