Skip to content

fix: MultiSchema getting incorrect id in arrays #2199

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

fix: MultiSchema getting incorrect id in arrays #2199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e83df1
fix: MultiSchema getting incorrect id in arrays
qingqi-spotify Jan 18, 2021
73039a4
tests: fix tests and update docs
QingqiShi Apr 21, 2021
129b6bc
Update 3.x upgrade guide.md
epicfaace Apr 21, 2021
533c47c
Update 3.x upgrade guide.md
epicfaace Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions docs/3.x upgrade guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,45 @@ From `@babel/preset-env`'s docs
Before an option could include a `$ref`.

Now any option with a reference will be resolved/dereferenced when given as props for `MultiSchemaField`.

### Generate correct ids when arrays are combined with `anyOf`/`oneOf`

In v2, with arrays inside `anyOf` or `oneOf`, the parent name
was not used to generate ids. For example, given a schema such as

```json
{
"type": "object",
"properties": {
"items": {
"type": "array",
"items": {
"type": "object",
"anyOf": [
{
"properties": {
"foo": {
"type": "string",
},
},
},
{
"properties": {
"bar": {
"type": "string",
},
},
},
],
},
},
},
}
```

We would get fields with id `root_foo` and `root_bar`. As you can imagine, we
could end up with duplicated ids if there's actually a `foo` or a `bar` in the
root of the schema.

From v3, the child fields will correctly use the parent id when generating
its own id, generating ids such as `root_items_0_foo`.
2 changes: 1 addition & 1 deletion packages/core/src/components/fields/SchemaField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ function SchemaFieldRender(props) {
let idSchema = props.idSchema;
const schema = retrieveSchema(props.schema, rootSchema, formData);
idSchema = mergeObjects(
toIdSchema(schema, null, rootSchema, formData, idPrefix),
toIdSchema(schema, idSchema.$id, rootSchema, formData, idPrefix),
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused as to what exactly this change does. Do you mind explaining a bit as to how this change in the logic fixes the issue? @QingqiShi

Depending on how complex it is, it might be good for us to add a comment so future users of this code can more easily understand, as well!

Copy link
Author

@QingqiShi QingqiShi Apr 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Gladly.

However I'm not sure if we need much explaining here. The call signature for toIdSchema is toIdSchema(schema, id, rootSchema, formData, idPrefix), so I believe passing null to the id argument is incorrect to begin with.

More specifically, in the ArrayField each item gets an itemIdSchema with the id idSchema.$id + "_" + index, this id then gets passed to each item rendered bySchemaField. The logic after that is a bit fuzzy to me, but I imagine if we don't use it during the generation of new ids, it defaults to 'root' prefix and this wrong id eventually get passed to the MultiSchema component.

🙏 Thanks again for spending the time to maintain this package.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks for the explanation! I'm still a bit confused as to how this change would have only affected the ids in arrays in MultiSchemaField. I would have expected that such a change to SchemaField.js would have had a significant effect on all / many more ids generated through idSchema. Do you know why that's not the case?

idSchema
);
const FieldComponent = getFieldComponent(schema, uiSchema, idSchema, fields);
Expand Down
30 changes: 21 additions & 9 deletions packages/core/test/anyOf_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,9 @@ describe("anyOf", () => {

expect(node.querySelectorAll("select")).to.have.length.of(1);

expect(node.querySelectorAll("input#root_foo")).to.have.length.of(1);
expect(node.querySelectorAll("input#root_items_0_foo")).to.have.length.of(
1
);
});

it("should not change the selected option when switching order of items for anyOf inside array items", () => {
Expand Down Expand Up @@ -995,8 +997,12 @@ describe("anyOf", () => {
target: { value: $select.options[1].value },
});

expect(node.querySelectorAll("input#root_foo")).to.have.length.of(1);
expect(node.querySelectorAll("input#root_bar")).to.have.length.of(1);
expect(node.querySelectorAll("input#root_items_0_foo")).to.have.length.of(
1
);
expect(node.querySelectorAll("input#root_items_0_bar")).to.have.length.of(
1
);
});

it("should correctly infer the selected option based on value", () => {
Expand Down Expand Up @@ -1075,13 +1081,19 @@ describe("anyOf", () => {
},
});

const idSelects = node.querySelectorAll("select#root_id");
const rootId = node.querySelector("select#root_id");
expect(rootId.value).eql("chain");

expect(idSelects).to.have.length(4);
expect(idSelects[0].value).eql("chain");
expect(idSelects[1].value).eql("map");
expect(idSelects[2].value).eql("transform");
expect(idSelects[3].value).eql("to_absolute");
const componentId = node.querySelector("select#root_components_0_id");
expect(componentId.value).eql("map");

const fnId = node.querySelector("select#root_components_0_fn_id");
expect(fnId.value).eql("transform");

const transformerId = node.querySelector(
"select#root_components_0_fn_transformer_id"
);
expect(transformerId.value).eql("to_absolute");
});
});
});
28 changes: 19 additions & 9 deletions packages/core/test/oneOf_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ describe("oneOf", () => {

expect($select.value).eql("1");

Simulate.change(node.querySelector("input#root_bar"), {
Simulate.change(node.querySelector("input#root_items_bar"), {
target: { value: "Lorem ipsum dolor sit amet" },
});

Expand Down Expand Up @@ -584,8 +584,12 @@ describe("oneOf", () => {
target: { value: $select.options[1].value },
});

expect(node.querySelectorAll("input#root_foo")).to.have.length.of(1);
expect(node.querySelectorAll("input#root_bar")).to.have.length.of(1);
expect(node.querySelectorAll("input#root_items_0_foo")).to.have.length.of(
1
);
expect(node.querySelectorAll("input#root_items_0_bar")).to.have.length.of(
1
);
});
});

Expand Down Expand Up @@ -767,12 +771,18 @@ describe("oneOf", () => {
},
});

const idSelects = node.querySelectorAll("select#root_id");
const rootId = node.querySelector("select#root_id");
expect(rootId.value).eql("chain");

expect(idSelects).to.have.length(4);
expect(idSelects[0].value).eql("chain");
expect(idSelects[1].value).eql("map");
expect(idSelects[2].value).eql("transform");
expect(idSelects[3].value).eql("to_absolute");
const componentId = node.querySelector("select#root_components_0_id");
expect(componentId.value).eql("map");

const fnId = node.querySelector("select#root_components_0_fn_id");
expect(fnId.value).eql("transform");

const transformerId = node.querySelector(
"select#root_components_0_fn_transformer_id"
);
expect(transformerId.value).eql("to_absolute");
});
});