# fix(angular-material): don't mutate the given UI schema in detail renderers - #2628
Open
lucas-koehler wants to merge 9 commits into
Open
lucas-koehler wants to merge 9 commits into
lucas-koehler wants to merge 9 commits into
Conversation
ArrayLayoutRenderer#getProps was called from the template, i.e. on every change detection cycle, and called setReadonly/unsetReadonly on the result of findUISchema. For an inline options.detail - or a ui schema from the registry - findUISchema hands back the very object the user passed in, and setReadonly/unsetReadonly modify it in place. While the array is empty getProps is never called, so the first "add" is the first time options.readonly is written into the user's ui schema. Ancestor layouts track their children by `path + JSON.stringify(uischema)`, so that write changes the track key of the whole branch, NgForOf destroys and re-creates the embedded view, and any state below it is lost. For a Categorization that means the selected tab jumps back to the first one - only on the first "add", because later ones write the same value. findUISchema is now called from mapAdditionalProps instead of the template, its result is copied before setting the readonly option, and it is only recalculated when one of its inputs actually changed. The items' props are precalculated as well, so that the template no longer hands a new object to jsonforms-outlet on every change detection cycle - each of those made the outlet deep clone the whole form state, once per item. Also adds an example reproducing the issue. Fixes #2343
…trols ObjectControlRenderer wrote `type`, `label` and the `readonly` option directly into the ui schema returned by findUISchema, which for an inline options.detail - or a ui schema from the registry - is the very object the user passed in. Besides corrupting the user's ui schema, this can destroy and re-create unrelated parts of the form, because ancestor layouts track their children by the serialized ui schema. The result of findUISchema is now copied before it is modified, and it is only recalculated when one of its inputs actually changed. As a side effect this also fixes the detail staying readonly forever once the control had been disabled: there was no branch undoing the previous setReadonly, and now there is nothing to undo in the first place.
…detail MasterListComponent set the readonly option directly on the ui schema returned by findUISchema, which for an inline options.detail - or a ui schema from the registry - is the very object the user passed in. Besides corrupting the user's ui schema, this can destroy and re-create unrelated parts of the form, because ancestor layouts track their children by the serialized ui schema. The result of findUISchema is now copied before the readonly option is set, and it is only recalculated when one of its inputs actually changed - which also keeps the detail's ui schema reference stable while items are added or removed. As a side effect this also fixes the detail staying readonly forever once the control had been disabled.
ArrayLayoutRenderer called unsetReadonly on the item's ui schema whenever the array was enabled. That only existed to undo the renderer's own setReadonly call, which it no longer performs on the user's ui schema - the item ui schema is now rebuilt from the original whenever the enabled state changes. All unsetReadonly did on top of that was overwrite a readonly option the user had deliberately set on a control inside options.detail, so it is removed.
ArrayLayoutRenderer is OnPush but never scheduled a check of its own, so it was only rendered when an event inside its own view happened to mark it dirty. A state change originating anywhere else - the form being set readonly, data being set programmatically, an item being added by another renderer - updated the component's fields but never reached the template. It now calls markForCheck when it maps new props, like the layout renderers and the list with detail already do.
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
coverage: 84.55% (+0.03%) from 84.521% — lk/2343-angular-category-issue into master |
sdirix
previously approved these changes
Sep 25, 2026
sdirix
left a comment
sdirix
left a comment
Member
There was a problem hiding this comment.
Works for me. I have minor suggestions.
I also would like to track a follow up to get rid of the cloning at some point.
Comment on lines
+197
to
+209
| it('does not modify the given ui schema when rendering items', () => { | ||
| const uischema = cloneDeep(TEST_UISCHEMA); | ||
| const pristine = cloneDeep(TEST_UISCHEMA); | ||
|
|
||
| setupMockStore(fixture, { | ||
| data: { test: [{}] }, | ||
| schema: TEST_SCHEMA, | ||
| uischema, | ||
| }); | ||
| fixture.componentInstance.ngOnInit(); | ||
| fixture.detectChanges(); | ||
|
|
||
| expect(uischema).toEqual(pristine); |
Member
There was a problem hiding this comment.
With the old code this test still passes when it runs after the Array layout tests above (it only failed for me when run alone). Those hand TEST_UISCHEMA itself to the renderer, so the old code writes readonly: false into it and pristine already contains the mutation. We should build pristine from a separate literal or deep freeze TEST_UISCHEMA, otherwise the check depends on Jasmine's random test order.
Comment on lines
+275
to
+306
| private resolveDetailUiSchema(props: ArrayLayoutProps): UISchemaElement { | ||
| const deps = [ | ||
| props.uischema, | ||
| props.uischemas, | ||
| props.schema, | ||
| props.rootSchema, | ||
| props.path, | ||
| this.isEnabled(), | ||
| ]; | ||
| if (!depsChanged(this.detailUiSchemaDeps, deps)) { | ||
| return this.detailUiSchema; | ||
| } | ||
| this.detailUiSchemaDeps = deps; | ||
|
|
||
| // `findUISchema` can hand back the inline `options.detail` UI schema or a | ||
| // UI schema from the registry, i.e. objects owned by the user. We set the | ||
| // `readonly` option on the result, so we have to work on a copy. | ||
| const detailUiSchema = cloneDeep( | ||
| findUISchema( | ||
| props.uischemas, | ||
| props.schema, | ||
| props.uischema.scope, | ||
| props.path, | ||
| undefined, | ||
| props.uischema, | ||
| props.rootSchema | ||
| ) | ||
| ); | ||
| if (this.isEnabled()) { | ||
| unsetReadonly(uischema); | ||
| } else { | ||
| setReadonly(uischema); | ||
| if (!this.isEnabled()) { | ||
| setReadonly(detailUiSchema); | ||
| } | ||
| return { | ||
| schema: this.scopedSchema, | ||
| path: Paths.compose(this.propsPath, `${index}`), | ||
| uischema, | ||
| }; | ||
| return detailUiSchema; |
Member
There was a problem hiding this comment.
This block (deps, findUISchema, cloneDeep, setReadonly) is basically the same in master.ts and object.renderer.ts, including the six deps. I would move it into a shared helper next to depsChanged so the three can't drift apart, which also fixes the naming (detailUISchema in master.ts vs. detailUiSchema in the other two).
Comment on lines
+289
to
+291
| // `findUISchema` can hand back the inline `options.detail` UI schema or a | ||
| // UI schema from the registry, i.e. objects owned by the user. We set the | ||
| // `readonly` option on the result, so we have to work on a copy. |
Member
There was a problem hiding this comment.
Minor: This repeats the doc comment on detailUiSchema a few lines above, and the same text is also in master.ts and object.renderer.ts. I would keep it once and trim the other new comments too (e.g. on uischemas and before markForCheck) to match the comment density of the surrounding code.
Comment on lines
131
to
135
| if (isEmpty(props.path)) { | ||
| this.detailUiSchema.type = 'VerticalLayout'; | ||
| detailUiSchema.type = 'VerticalLayout'; | ||
| } else { | ||
| (this.detailUiSchema as GroupLayout).label = startCase(props.path); | ||
| (detailUiSchema as GroupLayout).label = startCase(props.path); | ||
| } |
Member
| - Nothing is written into your UI schema anymore. If you relied on reading the `readonly` option, the `type` or the `label` back out of your own object, you need to track that state yourself. | ||
| - The array layout no longer forces the `readonly` option to `false` on the controls of an enabled array's detail. That option takes precedence over both the global config and the JSON schema, so the write suppressed all of them. For controls inside an array's `options.detail`, the following now take effect where they previously did not, in this order of precedence: a `readonly`/`readOnly` option you set yourself, a `readonly`/`readOnly` entry in the global config, and a `readOnly: true` in the corresponding JSON schema. If a field inside an array detail unexpectedly became readonly, one of these is now being honored. | ||
| - The detail UI schema is only recalculated when one of its inputs changes. Modifying your UI schema in place, without replacing the object, is not picked up - as everywhere else in JSON Forms, provide a new UI schema object instead. | ||
| - The array layout renders one item per array entry. Previously, data that was neither an array nor empty - for example an object at a path the schema declares as an array - rendered a single item at a path that does not exist, next to the "No data" message. Such data now renders no items at all. It is still reported as a validation error as before. |
Member
There was a problem hiding this comment.
Minor: This bullet describes a fix for data violating the schema, which needs no action from users, so I would drop it. In general I would trim the entry to what users need to change, as it's quite long.
| UISchemaTester, | ||
| unsetReadonly, | ||
| } from '@jsonforms/core'; | ||
| import cloneDeep from 'lodash/cloneDeep'; |
Member
There was a problem hiding this comment.
We should get rid of the cloning in a follow up again. In all other renderer sets we make sure to never modify a given schema or ui schema, so eventually we should do the same in Angular.
…test order The array layout tests shared a single TEST_UISCHEMA object. The earlier tests hand it to the renderer directly, so with the old, mutating renderer the "pristine" copy taken by the later tests already contained the mutation, and those tests only failed when run in isolation. TEST_UISCHEMA is replaced by a factory, so every test - and every pristine copy - gets its own fresh literal.
… renderers The array layout, the object control and the list with detail each resolved their detail ui schema with the same memoized block: the same six dependencies, findUISchema, a defensive copy and setReadonly when disabled. This is moved into createDetailUiSchemaResolver next to depsChanged, so the three can't drift apart. MasterListComponent#detailUISchema is renamed to detailUiSchema to match the other two. The comments introduced alongside are trimmed to the density of the surrounding code; the reason for the copy is documented once, on the helper.
…trols ObjectControlRenderer set the type of its detail ui schema to VerticalLayout at the root and its label to the property name otherwise, even for a detail ui schema the user provided via options.detail or the uischemas registry. Such a detail is now used as-is and only the generated one is adjusted, as in the React and Vue object renderers.
Reduce the entry to what users need to act on. The bullet about schema violating non-array data rendering no items is dropped, as it describes a fix that requires no action.
Contributor
Author
This branch has not been deployed
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
3 participants
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.
Fixes #2343
The bug
With a
VerticalLayout→Categorization→ twoCategorytabs, each holding an array control with an inlineoptions.detail: switch to the second tab and click "add". The item is added, but the form jumps back to the first tab. Every subsequent "add" behaves correctly.Root cause
findUISchemareturns the inlineoptions.detailUI schema - or a UI schema from theuischemasregistry - as-is, i.e. the very object the application passed in (reducers.ts:78,:88).ArrayLayoutRenderer#getPropswas called from the template, so on every change detection cycle it ranunsetReadonly/setReadonlyon that object, and those writeoptions.readonlyinto every leaf control in place.While the array is empty
getPropsis never called, so the first "add" is the first timeoptions.readonlyis written into the application's UI schema. Ancestor layouts track their children withpath + JSON.stringify(uischema)(LayoutRenderer.trackElement), andNgForOf.ngDoCheckre-runs its differ on every check regardless of whether the iterated array changed. So that single write flips the track key of the wholeCategorizationbranch, Angular destroys and re-creates the embedded view, and the freshly createdMatTabGroupstarts at index 0 again. Later adds write the same value, the key is stable, and nothing happens - hence "only the first click".The same in-place mutation existed in the object control and the list-with-detail renderers.
Changes
Five commits, each self-contained:
mapAdditionalPropsinstead of the template, copy it before setting thereadonlyoption, and only recalculate it when one of its inputs changed. The items' props are precalculated too, and the template iterates them: previously it handed a new object tojsonforms-outleton every change detection cycle, and each of those made the outlet deep clone the entire form state, once per item. Adds an example reproducing the issue.typeandlabel.unsetReadonly- it only ever existed to undo the renderer's own write, which no longer happens.markForCheckin the array layout - it isOnPushbut never scheduled a check of its own, so state changes that did not originate from an event inside its own view never reached the template. Related rather than required: before this PR the array layout repainted as a side effect of the destroy/recreate that commits 1-4 remove.setReadonly/unsetReadonlywere only ever used byangular-material; React and Vue are untouched.Behaviour changes
Documented in
MIGRATION.mdunder 3.9:readonly, and for object controlstypeandlabel.readonlytofalseon the controls of an enabled array's detail. That option takes precedence over both the global config and the JSON schema, so the write suppressed all of them. Areadonly/readOnlyoption you set yourself, areadonly/readOnlyconfig entry, and areadOnly: truein the JSON schema now take effect for controls inside an array'soptions.detail. This is the most likely source of surprise: a field inside an array detail may now be readonly where it previously was not.setReadonly, so it stayed readonly forever.ArrayLayoutRendererexposesitemProps, which also drives how many items are rendered;getProps(index)reads from it and returnsundefinedout of range. Subclasses overridingmapAdditionalPropsmust callsuper.Testing
Twelve new tests across
array-layout.spec.ts,object-control.spec.tsandmaster-detail.spec.ts, all verified to fail on the pre-fix code. They cover: the given UI schema is not modified (inline detail and registry), the detail is a stable copy across state emissions, the item props are reused rather than rebuilt per cycle, readonly still lands on the copy when disabled and is cleared again when re-enabled, an explicitreadonly: trueinsideoptions.detailsurvives, non-array data renders no items, and both renderers schedule a change detection check.There is no test asserting the symptom itself (that
MatTabGroup.selectedIndexsurvives an "add"). Such a test needs a hand-tuned multi-passdetectChangessequence - theNgForOfdiffer observes the mutation one pass after it happens - and would silently pass if the pass count were wrong. The tests pin the invariant instead: the UI schema is never mutated, which is the actual root cause.Manual check: run the Angular Material example app, open Categorization - Issue 2343, switch to tab A2, click add. The item appears and the form stays on A2.
Follow-ups (not in this PR)
LayoutRenderer.trackElementkeys embedded views onpath + JSON.stringify(uischema). This PR removes JSON Forms' own in-place writers, but any application or third-party renderer that mutates its UI schema in place still reproduces this class of bug, and the serialization runs over every child's full subtree on every check. Worth its own issue - probably tracking by index or element identity, which is breaking for anyone relying on in-place mutation.CategorizationTabLayoutRendererkeeps no selection state of its own, so a rule-driven change to category visibility can still reset the selected tab. Fixing it properly needs selection state that outlives the component instance.JsonFormsAbstractControlinjects noChangeDetectorRef, so everyOnPushcontrol renderer has the problem commit 5 fixes locally for the array layout. The general fix belongs in@jsonforms/angularand affects all renderer sets.