Skip to content

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

Open
JC-wk wants to merge 111 commits into
microsoft:mainfrom
JC-wk:template-upgrade-properties
Open

Allow Template upgrade properties to be set and remove properties that no longer exist.#4783
JC-wk wants to merge 111 commits into
microsoft:mainfrom
JC-wk:template-upgrade-properties

Conversation

@JC-wk

@JC-wk JC-wk commented Dec 16, 2025

Copy link
Copy Markdown
Collaborator

Resolves #4732 #4730

What is being addressed

Currently if you add a new property to a template there is no way to specify this property before an upgrade is ran.
The upgrade may fail due to the missing property (although the template version is still incremented)
The user then has to click update and supply the property.
Similarly when removing a property from a template and running an upgrade, the property still exists on the resource.

Todo

How is this addressed

  • Adds a form to allow the user to specify new properties prior to the upgrade
  • Removes any template properties that no longer exist in the new template
  • Added tests
  • Updated CHANGELOG.md
  • Increment API version
image image

James Chapman and others added 11 commits December 10, 2025 15:23
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
@JC-wk
JC-wk requested a review from a team as a code owner December 16, 2025 09:54
@github-actions

github-actions Bot commented Dec 16, 2025

Copy link
Copy Markdown

Unit Test Results

1 019 tests   1 019 ✅  40s ⏱️
   28 suites      0 💤
    2 files        0 ❌

Results for commit e5cc5dc.

♻️ This comment has been updated with latest results.

@JC-wk
JC-wk marked this pull request as draft December 16, 2025 10:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • ui/app/package-lock.json: Generated file
Suppressed comments (4)

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:538

  • The Upgrade button remains enabled while the selected template schema is being fetched. Immediately after selecting a version (or while switching versions), newPropertiesToFill still reflects the empty or previous schema, so a user can submit the upgrade before newly required fields and removed properties are known. Set loading synchronously when the selection changes and include it in the disabled condition.
              <PrimaryButton
                primaryDisabled={
                  !selectedVersion ||
                  (newPropertiesToFill.length > 0 &&

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:518

  • Conditional schemas are evaluated by RJSF against only newPropertyValues, so selectors that already exist on the resource are missing. For example, templates/workspaces/base/template_schema.json:253-281 requires auth_type in its if; an existing resource with auth_type: "Manual" is therefore evaluated as the else branch here, and a newly added required client_id is not rendered even though the button's separate combinedState validation keeps Upgrade disabled. Include existing selector values in the form's condition-evaluation state while still extracting only new fields for the PATCH payload.
                    schema={finalSchema}
                    formData={newPropertyValues}
                    uiSchema={uiSchema}
                    validator={validator}
                    onChange={(e) => setNewPropertyValues(e.formData)}

api_app/db/repositories/resources.py:463

  • This helper removes every required constraint from the target schema when it is invoked below, including properties introduced by the upgrade and conditional then.required/else.required rules. Since merged_properties represents the resource's full target state during an upgrade, a client can omit a newly required value and validation still succeeds, leaving deployment to fail after the template version is advanced. Preserve required validation for upgrades, while explicitly handling values supplied later by pipeline substitution.
        def _strip_required(schema_node: Any):
            if isinstance(schema_node, dict):
                schema_node.pop("required", None)
                for v in schema_node.values():
                    _strip_required(v)

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:320

  • newPropKeysToSend is the union of properties from both then and else branches, and this loop applies defaults without checking which branch matches the existing resource. If both branches introduce fields, defaults from the inactive branch are included in newPropertyValues and sent; the API's target schema has unevaluatedProperties: false, so that otherwise valid upgrade is rejected as containing an unexpected property. Evaluate conditionals against the merged resource state and initialize/send only active-branch keys.

This issue also appears on line 535 of the same file.

        // prefill newPropertyValues with schema defaults (excluding pipeline properties)
        const initialValues: any = {};
        newPropKeysToSend.forEach((key) => {
          const propSchema = getSchemaProperty(newTemplate, key);
          const currentValue = getNestedValue(props.resource.properties, key);

Comment thread api_app/db/repositories/resources.py Outdated
Comment thread api_app/db/repositories/resources.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • ui/app/package-lock.json: Generated file
Suppressed comments (4)

api_app/db/repositories/resources.py:205

  • The removal set is derived only from the two schemas, so it cannot clean up properties that are already stale on the resource. For example, if an earlier upgrade left foo in Cosmos after foo disappeared from the current template, foo is absent from both schema sets and survives every later upgrade. Derive removals from the actual resource.properties paths against the enriched target schema (while retaining system properties).
            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

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:457

  • For a nested enum-invalid field, the visibility filter can require input even when an ancestor has tre-hidden, but this code removes the class only from the leaf. The ancestor remains hidden, so the user cannot provide the required replacement and the Upgrade button can remain disabled. Remove tre-hidden from every traversed path segment for fields made visible.
        if (i === parts.length - 1) {
          if (typeof current[part].classNames === "string") {
            current[part].classNames = current[part].classNames.replace(/\btre-hidden\b/g, "").trim();
          }
          if (typeof current[part]["ui:classNames"] === "string") {
            current[part]["ui:classNames"] = current[part]["ui:classNames"].replace(/\btre-hidden\b/g, "").trim();
          }
        } else {
          current = current[part];
        }

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:583

  • An enum-invalid existing value with no target default is not blocked when the field is optional: newPropertyValues has no value, this condition skips undefined, and the required check also passes. The upgrade then sends the old invalid value from the merged resource state and backend schema validation rejects it. Require a valid replacement whenever the resource's current enum value is no longer allowed, even for optional fields.
                        const val = getNestedValue(newPropertyValues, key);

                        // Check if value is invalid enum (for both required and optional fields)
                        const propSchema = getSchemaProperty(newTemplateSchema, key);
                        if (
                          propSchema &&
                          propSchema.enum &&
                          val !== undefined &&
                          val !== "" &&
                          !propSchema.enum.includes(val)
                        ) {
                          return true;
                        }

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:564

  • This external button never submits the RJSF form, and its disabled logic only reimplements required and enum checks. Other target-schema constraints such as pattern, minLength, numeric bounds, and array constraints can therefore be PATCHed even when invalid, causing the upgrade to fail at the API. Submit through the form or track validator errors and call upgradeCall only after the complete schema validates.
              <PrimaryButton
                primaryDisabled={
                  !selectedVersion ||
                  loadingSchema ||
                  (newPropertiesToFill.length > 0 &&

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • ui/app/package-lock.json: Generated file
Suppressed comments (4)

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:536

  • Because this reduced form uses omitExtraData with liveOmit, existing resource fields that are not in the reduced schema are removed from e.formData. If an allOf condition for a new field depends on one of those existing fields, extractNewPropertyValues evaluates the condition as inactive and drops the user's new value. Merge the persisted properties back before evaluating active branches.
                    onChange={(e) => {
                      const updatedNewVals = extractNewPropertyValues(e.formData, newTemplateSchema, allNewProperties);
                      setNewPropertyValues(updatedNewVals);
                      setFormHasErrors(Boolean(e.errors && e.errors.length > 0));

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:102

  • Missing optional values are always converted to "". That persists absent optional strings and makes optional number/boolean/array properties fail target-schema validation during upgrade. Only add a key when the form actually contains a value; required fields are already blocked separately.
        setNestedValue(updatedNewVals, key, val !== undefined ? val : "");

api_app/db/repositories/resources.py:205

  • This collects only leaf paths. If the target template removes a non-empty object property, its children are deleted but the object key itself remains as {}, so the upgrade does not fully remove the deleted property. Collect intermediate object paths as well for this removal pass (without changing _get_leaf_properties, which is also used by authorization validation).
            existing_paths = [path for path, _ in self._get_leaf_properties(resource.properties)]

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:552

  • formHasErrors is never reset when the selected target version changes. After entering an invalid value for one version, switching to another version can leave Upgrade permanently disabled even when the new schema is valid and has no editable fields.
                    setSelectedVersion(option.text);
                    setLoadingSchema(true);

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • ui/app/package-lock.json: Generated file
Suppressed comments (4)

api_app/db/repositories/resources.py:278

  • This combines install- and upgrade-pipeline properties, but pipeline substitution executes only the selected primary action (service_bus/helpers.py:77-90). During an upgrade, an install-only property is therefore treated as pipeline-supplied and removed from required validation even though no upgrade step will populate it, allowing an invalid resource into deployment. Collect properties for the active phase only.
        if pipeline:
            for phase in ["install", "upgrade"]:
                if phase in pipeline and pipeline[phase]:
                    for step in pipeline[phase]:
                        if "properties" in step and step["properties"]:

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:557

  • Reset formHasErrors when changing versions. Otherwise, after a user makes the first version's form invalid, selecting another version retains the stale true value and keeps Upgrade disabled even when the newly loaded schema has no errors or no fields.
                  if (option) {
                    setSelectedVersion(option.text);
                    setLoadingSchema(true);
                  }

api_app/db/repositories/resources.py:137

  • The recursive key walk only follows object properties; it treats arrays as indivisible leaves and never visits items.properties. Existing templates contain arrays of objects (for example templates/workspaces/base/template_schema.json:292-323), so removing a field from an item schema will leave that field in every persisted array element, contrary to this upgrade's removal behavior. The schema/data traversal needs to recurse through array items as well.
            for k, v in properties.items():
                full_key = f"{prefix}{k}"
                keys.add(full_key)
                if isinstance(v, dict) and "properties" in v:
                    keys.update(self._get_all_property_keys_from_template(v, prefix=f"{full_key}."))

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:268

  • Properties that already existed but become required in the target version are excluded here. If such a property is absent from the resource, no form is shown for it, while backend target-schema validation rejects the upgrade as missing a required property. Include missing properties that are required in the target state, not only newly named or enum-invalid properties.
        const newPropKeys = newKeys.filter((key) => {
          if (!currentKeys.includes(key)) {
            return true;
          }
          const propSchema = getSchemaProperty(newTemplate, key);

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • ui/app/package-lock.json: Generated file
Suppressed comments (4)

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:537

  • liveOmit strips existing resource fields because they are absent from the reduced schema. Those fields are still needed to evaluate retained allOf conditions: if a new conditional field depends on an existing property, the first edit removes the controller from e.formData, isKeyActiveInTemplate treats the branch as inactive, and the entered value is discarded. Keep extra fields during edits; extractNewPropertyValues already limits the PATCH to new keys.
                    liveOmit={true}

api_app/db/repositories/resources.py:237

  • Removing only runtime leaf paths leaves deleted container properties behind. For example, if the target drops obsolete and the resource contains { obsolete: { a: 1 } }, this loop deletes obsolete.a but persists obsolete: {}; an array of objects is similarly left as an array of empty objects. Remove the highest schema path that disappeared (or otherwise prune containers recursively) so a property removed from the target template is actually removed in full.
            existing_paths = [path for path, _ in self._get_leaf_properties(resource.properties)]
            for path in existing_paths:
                if path not in target_properties:
                    self._remove_property_by_path(resource.properties, path)

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:473

  • The full template's ui:order is copied into a schema that contains only upgrade fields. RJSF rejects order entries that are not properties in the rendered schema, so upgrades for existing templates with explicit orders (for example templates/shared_services/sonatype-nexus-vm/template_schema.json:67) can fail while rendering the dialog. Recursively prune each ui:order to the corresponding reduced schema, retaining or adding * for remaining upgrade fields.
  // Compose final uiSchema merging sanitizedUiSchema with our overrides
  const uiSchema = {
    ...sanitizedUiSchema,
    "ui:submitButtonOptions": { norender: true },
  };

ui/app/src/utils/schemaUpgradeUtils.ts:21

  • This recursion handles nested properties but not object schemas under items. Consequently, adding a required field inside an existing array item is not detected as a new property, even though such arrays are supported by repository templates (for example templates/shared_services/firewall/template_schema.json:9-22) and the backend upgrade diff traverses items. The form/path utilities also need array-aware traversal so these upgrades can collect and submit the new item values.
    if (value && typeof value === "object" && "properties" in value) {
      // recur for nested properties
      keys = keys.concat(getAllPropertyKeys((value as any)["properties"], prefix + key + "."));
    } else {
      keys.push(prefix + key);

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • ui/app/package-lock.json: Generated file
Suppressed comments (3)

ui/app/src/utils/schemaUpgradeUtils.ts:26

  • Array item properties are flattened into dotted paths here (for example, rule_collections.name), but downstream getNestedValue, setNestedValue, and required-state traversal treat every segment as an object key. When an array item gains a required field, form edits cannot be extracted into newPropertyValues, so the PATCH omits them and the upgrade fails validation. Either retain the array property as the unit of comparison/patching or add array-aware traversal that maps values across items.
        "items" in value &&
        typeof (value as any).items === "object" &&
        (value as any).items !== null &&
        "properties" in (value as any).items
      ) {
        keys = keys.concat(getAllPropertyKeys((value as any).items["properties"], prefix + key + "."));

api_app/db/repositories/resources.py:282

  • This recursively deletes every empty dict/list in the resource, not just containers left empty by removed schema fields. Empty arrays are valid values in existing templates (for example, templates/shared_services/firewall/template_schema.json:9-15 defines rule_collections with default []), so any template upgrade will silently remove such properties even though they still exist in the target schema. Limit pruning to ancestors of paths actually removed, or preserve containers whose path remains defined by the target schema.
            # Prune any empty dict/list containers left behind after removal
            self._prune_empty_containers(resource.properties)

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:290

  • Newly-required detection evaluates conditionals only against the old resource state. If the target adds a selector with a default that activates an allOf branch, and that branch makes a previously optional property required, the selector is added to the form but the dependent property is omitted because its condition is false before defaults are applied. Once the default activates the branch, the user has no field to satisfy it and the upgrade is blocked or rejected. Apply target defaults before this comparison and re-evaluate conditional requirements from the resulting state.
          if (currentValue === undefined && isPropertyRequiredInState(newTemplate, key, props.resource.properties)) {
            return true;

…er ancestors and apply default values in ConfirmUpgradeResource

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • ui/app/package-lock.json: Generated file
Suppressed comments (3)

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:631

  • This treats an omitted optional enum as invalid because undefined enters this branch. A newly added optional enum without a default therefore disables Upgrade indefinitely even though the target JSON Schema and API allow it to be absent. Only validate enum membership when a value is present; the following required-field check should handle missing required enums.
                        // Check if value is invalid enum (for both required and optional fields)
                        if (
                          propSchema &&
                          propSchema.enum &&
                          (valInState === undefined ||

api_app/db/repositories/resources.py:263

  • target_properties is the union of both allOf branches, so pruning retains values from a branch that becomes inactive after the upgrade. If an enum migration changes a selector from Automatic to Manual, for example, old else-only fields remain in resource.properties; validation then rejects them because resource templates default unevaluatedProperties to false. Determine active target branches from the post-patch state and remove or exclude inactive-branch fields before validation.
            enriched_target_template = resource_template_repo.enrich_template(new_template, is_update=True)
            target_properties = self._get_all_property_keys_from_template(enriched_target_template)

            # Remove at the highest path that is completely absent from the target template,
            # so that containers (e.g. obsolete: {}) and array remnants are also cleaned up.
            existing_paths = [path for path, _ in self._get_leaf_properties(resource.properties)]
            removed_top_paths: set[str] = set()
            for path in existing_paths:
                if path not in target_properties:

api_app/tests_ma/test_api/test_routes/test_workspaces.py:696

  • Remove this unconditional debug print; it adds response bodies to normal test output and CI logs on every successful run.
        print("RESPONSE STATUS:", response.status_code, response.text)

Comment thread ui/app/src/utils/schemaUpgradeUtils.ts Outdated
Comment thread ui/app/src/components/shared/ConfirmUpgradeResource.tsx

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • ui/app/package-lock.json: Generated file
Suppressed comments (2)

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:451

  • Building the PATCH from every target-template key re-sends complete existing object/array containers. If the target removed a nested field, the container value still includes that stale field from props.resource.properties; the backend first prunes it, then rejects the PATCH for reintroducing an unexpected property, so nested-property removal cannot complete from this dialog. allNewProperties already contains new, newly-required, and enum-invalid fields from both conditional branches and has pipeline fields filtered out, so extract from that list instead.
      const allTemplateKeys = getAllPropertyKeysFromTemplate(newTemplateSchema).filter(
        (k) => !pipelinePropsSet.has(k.split(".")[0]),
      );
      const activePropertiesToPatch = extractNewPropertyValues(mergedFormData, newTemplateSchema, allTemplateKeys);

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:484

  • The reduced schema retains the API template's default unevaluatedProperties: false, while the form data includes the resource's complete existing property bag. Since most existing fields were pruned from finalSchema, AJV reports them as unevaluated after any form edit; formHasErrors then disables Upgrade even when the new fields are valid. Allow unevaluated properties only in this temporary reduced form schema (the API will still validate the final patch strictly).
  const finalSchema = reducedSchemaProperties ? { ...reducedSchemaProperties, ...conditionalBlocks } : null;

…d allow unevaluated properties in final schema

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • ui/app/package-lock.json: Generated file
Suppressed comments (2)

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:589

  • The external Upgrade button bypasses RJSF submission validation, while onChange only supplies validation errors during live validation. Without liveValidate, a user can enter a value that violates schema constraints such as pattern, minLength, minimum, or maximum, leave formHasErrors false, and submit an upgrade that the API rejects. Enable live validation (or explicitly run the validator in upgradeCall) so the external button reflects all schema errors.
                  <Form
                    liveOmit={false}
                    omitExtraData={false}
                    schema={finalSchema}
                    formData={mergePropertyValues(props.resource.properties, newPropertyValues)}
                    uiSchema={uiSchema}
                    validator={validator}

api_app/db/repositories/resources.py:523

  • A property that already existed in the old schema but was absent from the resource can never be populated here if it is non-updateable. For example, when an optional non-updateable field becomes required in the target version, omitting it fails target-schema validation, but supplying it makes is_new_on_upgrade false and has_existing false, so this function rejects the patch. The UI explicitly prompts for missing target-required fields, so this leaves that upgrade impossible. During an upgrade, allow initial assignment when the path is absent from the persisted resource and is required by the active target schema (while continuing to reject changes to persisted non-updateable values).
            is_new_on_upgrade = is_upgrade and prop_path not in old_template_properties

            if is_updateable or is_new_on_upgrade:
                return True

            if current_properties is not None and is_upgrade:
                has_existing, existing_val = get_nested_val(current_properties, prop_path)
                if has_existing and existing_val == prop_val:
                    return True

Comment thread api_app/db/repositories/resources.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • ui/app/package-lock.json: Generated file
Suppressed comments (3)

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:385

  • This active-branch check always uses the original resource state. Defaults written to initialValues are not fed back into initialCombinedState, so when a newly added selector's default activates an allOf branch, defaults in that branch are skipped. A required conditional field can then display its default in RJSF while the external Upgrade button remains disabled until the user changes the field; optional conditional defaults are omitted from the PATCH entirely. Update the combined state whenever a default is initialized so later conditional keys are evaluated against it.
        newPropKeysToSend.forEach((key) => {
          if (!isKeyActiveInTemplate(newTemplate, key, initialCombinedState)) {
            return;
          }

ui/app/src/utils/schemaUpgradeUtils.ts:21

  • Arrays of objects are treated as atomic, so a template upgrade that adds a required property under items.properties is not detected or presented to the user. These are supported by existing templates (for example templates/shared_services/firewall/template_schema.json:9-22 and templates/workspaces/base/template_schema.json:292-304); the UI will submit no value and backend validation will reject existing array items that lack the new required field. Add array-item traversal and a way to collect values for each existing item, or explicitly block such upgrades with an actionable message.
    if (value && typeof value === "object" && "properties" in value) {
      // Include the object container itself so required-object detection works, then recurse into children.
      // Arrays-of-objects are treated as atomic leaves (getNestedValue/setNestedValue don't support array-index traversal).
      keys.push(prefix + key);
      keys = keys.concat(getAllPropertyKeys((value as any)["properties"], prefix + key + "."));

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:578

  • This message is also shown for optional additions (which the button intentionally allows to remain empty) and for existing properties whose enum value became invalid, so “must specify” and “new properties” are both misleading. Use wording such as “Review values for new or changed properties:” instead.
                    You must specify values for new properties:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • ui/app/package-lock.json: Generated file
Suppressed comments (5)

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:386

  • mergePropertyValues(existing, {}) retains references to nested objects, so setting a nested default below also mutates props.resource.properties. Initialize this working state from a deep clone to keep upgrade-form defaults isolated from the persisted resource model.
        const initialCombinedState = mergePropertyValues(props.resource.properties, {});

ui/app/src/utils/schemaUpgradeUtils.ts:23

  • Indexed array-item paths are emitted here, but downstream setNestedValue creates object keys such as { redirect_uris: { "0": ... } }, and the PATCH merge replaces arrays atomically. Adding/defaulting an item field will therefore either fail the array schema or discard the item's existing sibling fields. Preserve the complete array value (with its existing item data) whenever a new key traverses an array, and add an end-to-end payload test for this case.
      if (Array.isArray(currentData) && (value as any).items.properties) {
        currentData.forEach((_item: any, index: number) => {
          keys = keys.concat(getAllPropertyKeys((value as any).items.properties, `${prefix + key}.${index}.`, _item));
        });

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:289

  • This is only a shallow copy. Applying a default to a newly added nested field through setNestedValue mutates the nested object in props.resource.properties, so merely selecting (or canceling) an upgrade can alter the resource held by the UI. Deep-clone the JSON property bag before applying target-template defaults.

This issue also appears on line 386 of the same file.

        const stateWithNewDefaults = { ...props.resource.properties };

api_app/_version.py:1

  • The API gains new backward-compatible upgrade behavior, which requires a MINOR version increment under the project's semantic-versioning policy; 0.26.1 denotes only a patch fix.
__version__ = "0.26.1"

ui/app/package.json:3

  • This PR adds backward-compatible upgrade functionality, so the project's semantic-versioning policy requires a MINOR UI bump rather than a PATCH bump. Update this to 0.9.0 and keep the lockfile version entries in sync.
  "version": "0.8.31",

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • ui/app/package-lock.json: Generated file
Suppressed comments (1)

api_app/db/repositories/resources.py:462

  • Array-of-object paths cannot be authorized correctly here. _get_leaf_properties emits paths such as redirect_uris.name, but this walker only descends through properties (the child schema is under items.properties), and get_nested_val likewise cannot cross the persisted list. Consequently, an upgrade that adds redirect_uris.value and sends each full item—as the new UI does—rejects the unchanged existing name as non-updateable. Make the schema/current-value traversal array-aware and cover the end-to-end array upgrade case in the repository tests.
                    current = current[part].get("properties", {})

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • ui/app/package-lock.json: Generated file
Suppressed comments (1)

ui/app/src/utils/schemaUpgradeUtils.ts:267

  • When an array item gains one field, this copies the entire array schema because arrays expose item properties under items, not direct properties. The upgrade form consequently renders existing item fields as editable, and setNestedValue later sends the full edited items. Editing an existing non-updateable sibling then makes the upgrade fail (or changes unrelated updateable data). Prune items.properties to the newly added item fields while retaining existing sibling values only in the PATCH payload.
        prunedProperties[propName] = { ...(propSchema as any) };

Comment thread api_app/db/repositories/resources.py Outdated
…ested properties and improve validation during upgrades

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • ui/app/package-lock.json: Generated file
Suppressed comments (4)

api_app/db/repositories/resources.py:638

  • For arrays of objects, get_nested_val returns all existing leaf values as a list, so this membership check treats each submitted value as unchanged without preserving item identity or cardinality. A PATCH to a non-updateable array can therefore delete, duplicate, reorder, or recombine items as long as each leaf value appeared somewhere in the old array; _deep_dict_update then replaces the entire array. Compare the complete array value for non-updateable arrays, or retain item indices while validating leaves.
            if current_properties is not None and is_upgrade:
                has_existing, existing_val = get_nested_val(current_properties, prop_path)
                if has_existing and (existing_val == prop_val or (isinstance(existing_val, list) and prop_val in existing_val)):
                    return True

api_app/db/repositories/resources.py:424

  • This collects property names from every upgrade pipeline step, including steps that patch other resources. Only the main step supplies properties for the resource being upgraded; for example, the Health Services pipeline's other step supplies rule_collections to the firewall template. If the upgraded template has a new property with the same name, this code incorrectly exempts it from required validation. Restrict this set to stepId == "main".

This issue also appears on line 635 of the same file.

            for step in pipeline[action]:
                if "properties" in step and step["properties"]:
                    for prop in step["properties"]:
                        if isinstance(prop, dict) and prop.get("name"):
                            properties.add(prop["name"])

ui/app/src/components/shared/ConfirmUpgradeResource.tsx:335

  • This treats properties sent to downstream resources by any upgrade pipeline step as if the backend will populate them on the resource currently being upgraded. Only properties on the main step belong to this resource; a same-named new property is otherwise removed from the form and omitted from the PATCH, defeating the new-property upgrade flow. Filter to step.stepId === "main" before collecting names.
        if (newTemplate?.pipeline?.upgrade) {
          newTemplate.pipeline.upgrade.forEach((step: any) => {
            if (step.properties) {
              step.properties.forEach((prop: any) => {
                pipelineProps.add(prop.name);
              });
            }
          });

ui/app/src/utils/schemaUpgradeUtils.ts:229

  • Nested traversal only consults currentSchema.properties. If the parent object is defined in an active allOf.then/else branch (as data_source_config is in the OHDSI schema), nextSchema becomes undefined, so a newly added required child is reported as optional. Because formHasErrors starts false and is only updated by onChange, the Upgrade button can initially remain enabled and submit a payload that the backend rejects. Resolve the next schema from the active conditional branch when it is absent from top-level properties.
    const nextSchema = currentSchema.properties ? currentSchema.properties[part] : undefined;
    currentSchema = nextSchema;
    currState = currState ? currState[part] : undefined;

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • ui/app/package-lock.json: Generated file
Suppressed comments (2)

api_app/db/repositories/resources.py:566

  • When part is an array index, this switches to the item schema but then processes the index as though it were an item property (and always selects item 0). Thus redirect_uris.0.value checks whether "0" is required instead of "value". An item field that existed as optional in the old template and becomes required in the target is consequently rejected as non-updateable even when the upgrade supplies it.
                if isinstance(curr_schema.get("items"), dict):
                    curr_schema = curr_schema["items"]
                    curr_state = curr_state[0] if isinstance(curr_state, list) and curr_state else {}

api_app/db/repositories/resources.py:293

  • Array-item schema paths omit indexes (for example, redirect_uris.value), but _get_leaf_properties emits instance paths such as redirect_uris.0.value. This direct comparison never matches when only an item field was removed, so that deleted field remains on every persisted array item and can also make validation against the target item schema fail. Normalize array instance paths to schema paths and remove the deleted field across all items.
            for path in existing_paths:
                if any(path == tp or path.startswith(tp + ".") for tp in removed_template_paths):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When upgrading a template that has a new property the user should be prompted to enter it or the defaults used (if provided)

4 participants