Skip to content
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

Jump to bottom

fix(@rjsf/core): Nested allOf blocks with multiple if/then/else statements fail to render correctly #2839

Merged
merged 8 commits into from
May 6, 2022
Merged

fix(@rjsf/core): Nested allOf blocks with multiple if/then/else statements fail to render correctly #2839

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d9c2c84
Support allOf blocks within other allOf blocks as described here http…
stuartwilkes Apr 28, 2022
173dd16
Add test for allOf at multiple levels
stuartwilkes Apr 29, 2022
4d4a05d
Update CHANGELOG.md
stuartwilkes Apr 29, 2022
e025609
Remove uneeded test
stuartwilkes Apr 29, 2022
3511d4b
Remove unnecessary test
stuartwilkes May 4, 2022
515ff2e
Add comment for new code block
stuartwilkes May 4, 2022
7ae0e20
Merge branch 'master' into fix-nested-allof-if-then-else
stuartwilkes May 5, 2022
4c76f16
Remove trailing space on comment
stuartwilkes May 5, 2022
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ should change the heading of the (upcoming) version to include a major version b

## @rjsf/core
- Feature for ui:submitButtonOptions on the submit button for forms (https://github.com/rjsf-team/react-jsonschema-form/pull/2640)
- Fix for nested allOf blocks with multiple if/then/else statements failing to render correctly (https://github.com/rjsf-team/react-jsonschema-form/pull/2839)

## Dev / docs / playground
- Enable ui options in playground, to demonstrate submit button options (https://github.com/rjsf-team/react-jsonschema-form/pull/2640)
Expand Down
27 changes: 27 additions & 0 deletions packages/core/src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,33 @@ export function retrieveSchema(schema, rootSchema = {}, formData = {}) {
return resolveCondition(schema, rootSchema, formData);
}

// For each level of the dependency, we need to recursively determine the appropriate resolved schema given the current state of formData.
// Otherwise, nested allOf subschemas will not be correctly displayed.
if (resolvedSchema.properties) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment clarifying why we have to do this new step, and what exactly we're doing? It's hard to follow exactly what's going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like "For each level of the dependency, we need to recursively determine the appropriate resolved schema given the current state of formData, otherwise nested allOf subschemas will not be correctly displayed"?

@AlimovSV wrote the original code here, so may want to word this more appropriately?

Copy link
Contributor

Choose a reason for hiding this comment

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

finding the right words is not my best side :) I like your description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nickgros Hopefully this comment resolves your query...

const properties = {};

Object.entries(resolvedSchema.properties).forEach(entries => {
const propName = entries[0];
const propSchema = entries[1];
nickgros marked this conversation as resolved.
Show resolved Hide resolved
const rawPropData = formData && formData[propName];
const propData = isObject(rawPropData) ? rawPropData : {};
const resolvedPropSchema = retrieveSchema(
propSchema,
rootSchema,
propData
);

properties[propName] = resolvedPropSchema;

if (
propSchema !== resolvedPropSchema &&
resolvedSchema.properties !== properties
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like properties will always be a different object than resolvedSchema.properties, since you declared a new properties object on line 771. So I think this would always be true.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this is possible ONLY when the recursive call of retrieveSchema returns a modified schema for the specific property. Otherwise resolvedSchema will be returned unchanged.

Copy link
Contributor

@nickgros nickgros May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I follow that retrieveSchema could return the value referenced by propSchema under certain conditions. I think we need a deep equality check for the second condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea of this check is to reduce creation of shallow copy for the retrieved schemas. So if a schema isn't computed schema (doesn't have any if/then/else) it will be returned unchanged (and reference equality check will be enough for this condition)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if I'm being unclear, but essentially I don't think you will ever have referential equality between resolvedSchema.properties and properties because properties is always a new object.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, let me explain. Before loop we initialize properties variable as a new object, you right. And during the iteration (first time when we meet propSchema !== resolvedPropSchema) the condition resolvedSchema.properties !== properties will be truthy, you right too. But after we make a shallow copy of the schema with new properties container resolvedSchema = { ...resolvedSchema, properties }; during any next iteration resolvedSchema.properties !== properties will be falsy and we will not make shallow copy. The bonuse is: when resolvedSchema doesn't contain properties with computed schema we do not make shallow copy at all and return the unmodified object.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤦‍♂️ I finally see it now! Thank you for this explanation (and for bearing with me)!

) {
resolvedSchema = { ...resolvedSchema, properties };
}
});
}

if ("allOf" in schema) {
try {
resolvedSchema = mergeAllOf({
Expand Down
145 changes: 145 additions & 0 deletions packages/core/test/utils_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2540,6 +2540,151 @@ describe("utils", () => {
required: ["animal", "food"],
});
});
it("should resolve multiple conditions in nested allOf blocks", () => {
const schema = {
type: "object",
properties: {
Animal: {
default: "Cat",
enum: ["Cat", "Dog"],
title: "Animal",
type: "string",
},
},
allOf: [
{
if: {
required: ["Animal"],
properties: {
Animal: {
const: "Cat",
},
},
},
then: {
properties: {
Tail: {
default: "Long",
enum: ["Long", "Short", "None"],
title: "Tail length",
type: "string",
},
},
required: ["Tail"],
},
},
{
if: {
required: ["Animal"],
properties: {
Animal: {
const: "Dog",
},
},
},
then: {
properties: {
Breed: {
title: "Breed",
properties: {
BreedName: {
default: "Alsatian",
enum: ["Alsatian", "Dalmation"],
title: "Breed name",
type: "string",
},
},
allOf: [
{
if: {
required: ["BreedName"],
properties: {
BreedName: {
const: "Alsatian",
},
},
},
then: {
properties: {
Fur: {
default: "brown",
enum: ["black", "brown"],
title: "Fur",
type: "string",
},
},
required: ["Fur"],
},
},
{
if: {
required: ["BreedName"],
properties: {
BreedName: {
const: "Dalmation",
},
},
},
then: {
properties: {
Spots: {
default: "small",
enum: ["large", "small"],
title: "Spots",
type: "string",
},
},
required: ["Spots"],
},
},
],
required: ["BreedName"],
},
},
},
},
],
required: ["Animal"],
};
const definitions = {};
const formData = {
Animal: "Dog",
Breed: {
BreedName: "Dalmation",
},
};

expect(retrieveSchema(schema, { definitions }, formData)).eql({
type: "object",
properties: {
Animal: {
default: "Cat",
enum: ["Cat", "Dog"],
title: "Animal",
type: "string",
},
Breed: {
properties: {
BreedName: {
default: "Alsatian",
enum: ["Alsatian", "Dalmation"],
title: "Breed name",
type: "string",
},
Spots: {
default: "small",
enum: ["large", "small"],
title: "Spots",
type: "string",
},
},
required: ["BreedName", "Spots"],
title: "Breed",
},
},
required: ["Animal"],
});
});
it("should resolve $ref", () => {
const schema = {
type: "object",
Expand Down