Allow Template upgrade properties to be set and remove properties that no longer exist. - #4783
Allow Template upgrade properties to be set and remove properties that no longer exist.#4783JC-wk wants to merge 111 commits into
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 Results1 019 tests 1 019 ✅ 40s ⏱️ Results for commit e5cc5dc. ♻️ This comment has been updated with latest results. |
fix: allow upgrades on hidden properties
…property visibility
There was a problem hiding this comment.
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),
newPropertiesToFillstill 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-281requiresauth_typein itsif; an existing resource withauth_type: "Manual"is therefore evaluated as theelsebranch here, and a newly added requiredclient_idis not rendered even though the button's separatecombinedStatevalidation 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
requiredconstraint from the target schema when it is invoked below, including properties introduced by the upgrade and conditionalthen.required/else.requiredrules. Sincemerged_propertiesrepresents 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
newPropKeysToSendis the union of properties from boththenandelsebranches, 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 innewPropertyValuesand sent; the API's target schema hasunevaluatedProperties: 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);
There was a problem hiding this comment.
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
fooin Cosmos afterfoodisappeared from the current template,foois absent from both schema sets and survives every later upgrade. Derive removals from the actualresource.propertiespaths 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. Removetre-hiddenfrom 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:
newPropertyValueshas no value, this condition skipsundefined, 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 callupgradeCallonly after the complete schema validates.
<PrimaryButton
primaryDisabled={
!selectedVersion ||
loadingSchema ||
(newPropertiesToFill.length > 0 &&
There was a problem hiding this comment.
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
omitExtraDatawithliveOmit, existing resource fields that are not in the reduced schema are removed frome.formData. If anallOfcondition for a new field depends on one of those existing fields,extractNewPropertyValuesevaluates 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
formHasErrorsis 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);
There was a problem hiding this comment.
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 fromrequiredvalidation 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
formHasErrorswhen changing versions. Otherwise, after a user makes the first version's form invalid, selecting another version retains the staletruevalue 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 visitsitems.properties. Existing templates contain arrays of objects (for exampletemplates/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);
There was a problem hiding this comment.
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
liveOmitstrips existing resource fields because they are absent from the reduced schema. Those fields are still needed to evaluate retainedallOfconditions: if a new conditional field depends on an existing property, the first edit removes the controller frome.formData,isKeyActiveInTemplatetreats the branch as inactive, and the entered value is discarded. Keep extra fields during edits;extractNewPropertyValuesalready 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
obsoleteand the resource contains{ obsolete: { a: 1 } }, this loop deletesobsolete.abut persistsobsolete: {}; 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:orderis 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 exampletemplates/shared_services/sonatype-nexus-vm/template_schema.json:67) can fail while rendering the dialog. Recursively prune eachui:orderto 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
propertiesbut not object schemas underitems. 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 exampletemplates/shared_services/firewall/template_schema.json:9-22) and the backend upgrade diff traversesitems. 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);
…upgrade components
There was a problem hiding this comment.
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 downstreamgetNestedValue,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 intonewPropertyValues, 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-15definesrule_collectionswith 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
allOfbranch, 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
There was a problem hiding this comment.
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
undefinedenters 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_propertiesis the union of bothallOfbranches, so pruning retains values from a branch that becomes inactive after the upgrade. If an enum migration changes a selector fromAutomatictoManual, for example, old else-only fields remain inresource.properties; validation then rejects them because resource templates defaultunevaluatedPropertiesto 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)
There was a problem hiding this comment.
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.allNewPropertiesalready 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 fromfinalSchema, AJV reports them as unevaluated after any form edit;formHasErrorsthen 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
There was a problem hiding this comment.
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
onChangeonly supplies validation errors during live validation. WithoutliveValidate, a user can enter a value that violates schema constraints such aspattern,minLength,minimum, ormaximum, leaveformHasErrorsfalse, and submit an upgrade that the API rejects. Enable live validation (or explicitly run the validator inupgradeCall) 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_upgradefalse andhas_existingfalse, 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
There was a problem hiding this comment.
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
initialValuesare not fed back intoinitialCombinedState, so when a newly added selector's default activates anallOfbranch, 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.propertiesis not detected or presented to the user. These are supported by existing templates (for exampletemplates/shared_services/firewall/template_schema.json:9-22andtemplates/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:
…improve user messaging
There was a problem hiding this comment.
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 mutatesprops.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
setNestedValuecreates 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
setNestedValuemutates the nested object inprops.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.1denotes 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.0and keep the lockfile version entries in sync.
"version": "0.8.31",
…ionality with array property handling
There was a problem hiding this comment.
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_propertiesemits paths such asredirect_uris.name, but this walker only descends throughproperties(the child schema is underitems.properties), andget_nested_vallikewise cannot cross the persisted list. Consequently, an upgrade that addsredirect_uris.valueand sends each full item—as the new UI does—rejects the unchanged existingnameas 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", {})
There was a problem hiding this comment.
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 directproperties. The upgrade form consequently renders existing item fields as editable, andsetNestedValuelater sends the full edited items. Editing an existing non-updateable sibling then makes the upgrade fail (or changes unrelated updateable data). Pruneitems.propertiesto the newly added item fields while retaining existing sibling values only in the PATCH payload.
prunedProperties[propName] = { ...(propSchema as any) };
…ested properties and improve validation during upgrades
There was a problem hiding this comment.
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_valreturns 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_updatethen 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
mainstep supplies properties for the resource being upgraded; for example, the Health Services pipeline's other step suppliesrule_collectionsto 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 tostepId == "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
mainstep 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 tostep.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 activeallOf.then/elsebranch (asdata_source_configis in the OHDSI schema),nextSchemabecomes undefined, so a newly added required child is reported as optional. BecauseformHasErrorsstarts false and is only updated byonChange, 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-levelproperties.
const nextSchema = currentSchema.properties ? currentSchema.properties[part] : undefined;
currentSchema = nextSchema;
currState = currState ? currState[part] : undefined;
…and schema validation during upgrades
There was a problem hiding this comment.
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
partis 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). Thusredirect_uris.0.valuechecks 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_propertiesemits instance paths such asredirect_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):
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