Template upgrade properties - #3
Open
JC-wk wants to merge 88 commits into
Open
Conversation
This commit aligns the resource upgrade process with the update process by correctly handling conditional properties in the JSON schema. - The schema generation logic in `ConfirmUpgradeResource.tsx` is updated to include conditional blocks (`if`/`then`/`else`) when the condition is based on an existing property. - New read-only properties are now submitted during the upgrade process.
This commit aligns the resource upgrade process with the update process by correctly handling conditional properties in the JSON schema. - The schema generation logic in `ConfirmUpgradeResource.tsx` is updated to include conditional blocks (`if`/`then`/`else`) when the condition is based on an existing property. - New read-only properties are now submitted during the upgrade process. - The `liveOmit` prop is added to the form to prevent the submission of unevaluated properties from conditionally hidden fields.
…1346005040390942732 Fix Upgrade Conditional Properties
Unit Test Results668 tests 668 ✅ 7s ⏱️ Results for commit 2ec1049. ♻️ This comment has been updated with latest results. |
fix: allow upgrades on hidden properties
…property visibility
…ns with current template definitions: • Template Fixtures Updated: Replaced obsolete OS names (Windows 10, Server 2019 Data Science VM, Server 2022 Data Science VM) with active OS options (Windows 11, Windows Server 2025) in sample_resource_template() and sample_resource_template_with_version(). • Upgrade Test Updated: Updated test_validate_patch_rejects_existing_non_updateable_property_during_upgrade to use Windows Server 2025 for os_image.
• old_template_properties is now computed using self._get_all_property_keys_from_template(resource_template), which inspects both root properties and allOf (then.
properties / else.properties) on the old template object or dictionary.
2. Explicit Per-Property Permission Validation:
• Implemented get_prop_schema inside validate_patch to resolve property definitions across both root properties and allOf conditional blocks.
• Every property in resource_patch.properties is explicitly validated against allowed rules:
is_updateable = prop_def.get("updateable", False) is True if prop_def else False
is_new_on_upgrade = is_upgrade and prop_name not in old_template_properties
is_pipeline_prop = prop_name in pipeline_properties
if not (is_updateable or is_new_on_upgrade or is_pipeline_prop):
raise ValidationError(f"Property '{prop_name}' is not updateable.")
3. Added Unit Test:
• Added test_validate_patch_rejects_non_updateable_allOf_property in test_resource_repository.py to verify that non-updateable conditional properties in allOf are
correctly rejected during patch calls.
- Updated `_get_all_property_keys_from_template` to support nested properties with prefixes. - Modified `get_prop_schema` to retrieve property definitions using dotted paths. - Introduced `is_property_allowed` to validate properties during patching, allowing new nested properties under existing non-updateable objects. - Added unit test to verify behavior for new nested properties during template upgrades.
…component for better property handling
… for template upgrades
…prove loading state management in ConfirmUpgradeResource component
…rvice template to avoid unauthorized GETs against admin-only template endpoints.
…sitory and improve clarity in patch validation tests. Update ConfirmUpgradeResource component to streamline error handling and remove redundant workspace ID assignment.
#### 1. Early Error Return Loading States in UI
• Files: ConfirmUpgradeResource.tsx
• Fix: Added setRequestLoadingState(LoadingState.Error) before early returns when:
1. Workspace context is missing for a UserResource (line 147)
2. Parent workspace service info is missing (line 157)
3. Resource type is unsupported (line 165)
This ensures that ExceptionLayout.tsx renders properly instead of leaving the dialog stuck in LoadingState.Ok.
#### 2. Schema Pruning & Nested Property Prefilling
• Files: schemaUpgradeUtils.ts, ConfirmUpgradeResource.tsx, and ConfirmUpgradeResource.test.tsx
• Fix:
• Implemented pruneSchemaNode in schemaUpgradeUtils.ts to recursively prune object schemas so buildReducedSchema only includes
newly-added or modified sub-fields instead of rendering existing, unmodifiable sub-properties.
• Refactored initialValues prefilling in ConfirmUpgradeResource.tsx to populate values only for target missing/updated property
keys rather than deep-cloning whole top-level parent objects (preventing removed or read-only properties from being re-sent).
• Updated test assertions in ConfirmUpgradeResource.test.tsx and added new test cases for early error return paths.
#### 3. Duplicate Template Fetches in Backend
• Files: resources.py
• Fix:
• Updated validate_template_version_patch to return the fetched ResourceTemplate.
• Reused new_template in patch_resource and passed target_template=new_template into validate_patch, eliminating duplicate
database reads for the template during resource upgrade PATCH operations.
#### 4. Install Pipeline Test Fixture
• Files: test_resource_repository.py
• Fix: Updated test_validate_patch_allows_install_pipeline_property to use 'install' instead of 'upgrade' under
template_dict['pipeline'].
──────
### Verification
• UI Tests: Ran npx vitest run in ui/app — 27/27 test files (259 tests) passed.
• Backend Tests: Ran pytest in api_app — 737/737 tests passed.
• initialValues pre-filling now checks whether an existing property value is allowed by the target template's enum schema. If
invalid for the new enum, it prefers the template default (if defined) or leaves the value unset rather than preserving an invalid
enum option.
2. Enum-Invalid Keys Visibility & User Input (ConfirmUpgradeResource.tsx):
• visibleNewPropKeys now keeps keys with invalid enum values in the list of properties needing user input regardless of tre-hidden.
• sanitizedUiSchema strips tre-hidden from uiSchema for any property requiring user input so the input control is rendered visibly
in the form for user interaction.
3. Prototype Pollution Protection (schemaUpgradeUtils.ts):
• Added partGuard checks (__proto__, constructor, prototype) to getAllPropertyKeys and getTopLevelKeysFromTemplate to ensure
consistent prototype pollution safety across schema utilities.
…src/utils/schemaUpgradeUtils.ts
• Issue: schemaUpgradeUtils.ts was evaluating conditional allOf schema rules using only newPropertyValues. If a conditional
requirement depended on existing resource properties already on the resource, the condition would evaluate to false and allow
upgrading without requiring the newly required field.
• Fix: Added schemaUpgradeUtils.ts to deeply merge props.resource.properties with newPropertyValues. Updated the Upgrade button's
ConfirmUpgradeResource.tsx check in ConfirmUpgradeResource.tsx to pass this combined state into isPropertyRequiredInState.
• Test: Added unit test in ConfirmUpgradeResource.test.tsx to verify that conditional required rules based on existing resource
properties properly disable the Upgrade button until filled.
2. api_app/db/repositories/resources.py
• Issue: resources.py returned the original resource_template object even when templateVersion was patched and new_template was
fetched and validated, causing downstream callers to receive a stale template object.
• Fix: Updated line 212 of resources.py to return new_template if new_template is not None else resource_template.
• Test: Updated test_resource_repository.py to assert that patch_resource returns new_template on upgrade.
…between unexpected and non-updateable fields
…or resource upgrade
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves HASH_SIGN_FOLLOWED_BY_ISSUE_NUMBER
What is being addressed
Describe the current behavior you are modifying. Please also remember to update any impacted documentation.
How is this addressed