Optimizations to improve performance on complex conditional schemas #2466

nickgros · 2021-07-12T19:11:51Z

~~I've picked up from where #1666 has left off, hoping that the work I've done so far will be enough to add this feature.~~

~~In addition to the changes outlined in #1666, I've tried to address changes requested in that review, and fix a few other issues as well.~~

~~Here's a link to just the changes that I've personally made since I branched from the work in #1666~~

Since conditional support was added in #2700, this branch only exists to preserve the optimizations I've done to improve performance in complex schemas. We struggle with performance on complex conditional schemas because we re-run validation on every input change to determine if the condition has changed. Presently, I've worked around this with lots of memoization and attempts to get ajv to use its internal cache, but we could probably do this better/smarter than what I've done so far.

Checklist

I'm updating documentation
- I've checked the rendering of the Markdown text I've added
I'm adding or updating code
- I've added and/or updated tests
- I've updated docs if needed
I'm adding a new feature
- I've updated the playground with an example use of the feature

Fixed issue with arrays Removed only from tests better allOf support better allOf support mend If Then Else , Allof Fixed typo and included .lock.json added support for in if/then/else Added support for const as per rjsf-team#1627 Moved handling of if then / else from SchemaField to utils and tidied up things

…a-form into conditional-support

nickgros · 2021-07-12T19:18:05Z

packages/core/src/validate.js

-  } finally {
-    // make sure we remove the rootSchema from the global ajv instance
-    ajv.removeSchema(ROOT_SCHEMA_PREFIX);


Why is the root schema being removed (before my change)?

Since we now need to run the validator frequently to evaluate conditional subschemas, keeping the root schema in the cache is essential for performance, especially for large schemas.

I think I've figured it out: this function uses the same ajv instance as validateFormData, which has some advantages and some drawbacks. I think with this new implementation (not removing the root schema), validation breaks for certain schemas. I'm not happy with the performance of the current implementation, though, so I'll have to figure something else out.

One cause of performance issues here is that we can't take advantage of AJV's cache because withIdRefPrefix creates a new object every time. We see a substantial improvement in performance if we memoize withIdRefPrefix, because validate sees the same schema and can use the ajv cache instead of recompiling. I'm not yet sure that memoization is 100% safe here, but it seems like a possible path forward.

Another thing I've noticed: we get different behavior with the current implementation if the root schema has an $id. When an $id is present, I think AJV totally ignores the root schema prefix when we call addSchema(rootSchema, ROOT_SCHEMA_PREFIX), so the removeSchema call never works. Not 100% on this either, though.

As for $id issues, I've filed #2471, ideally would like to address it here (at the risk of increasing scope of this PR) since how RJSF uses AJV is closely related with adding if/then/else support

nickgros · 2021-07-12T19:21:38Z

packages/core/test/ifthenelse_test.js

+    });
+  });
+
+  it("handles nested if then else", () => {


#1666 didn't handle this case properly. A small change in the resolveSchema function fixed it (changing if "if" in schema to while "if" in schema), and here's a test to prove it.

nickgros · 2021-07-12T19:22:31Z

packages/core/test/ifthenelse_test.js

+    expect(node.querySelector("select[id=root_city]")).not.eql(null);
+  });
+
+  it("handles additionalProperties with if then else", () => {


Tests that we fixed the issue in this comment: #1666 (comment)

nickgros · 2021-07-15T18:21:59Z

Converting this PR to a draft because while I think my changes in 78fc62c (#2466) are a step forward, I think it's going to break some things and I don't want to indicate that this should be merged as-is without more tests (in addition to fixing the ones that fail).

jimmycallin · 2021-07-21T23:49:41Z

I tried it out a bit and I think this looks great! If we can get it in a mergeable state where only the if-then-else changes are included in this PR, I think we are pretty close.

A few comments:

Currently if you type a value in a conditional field, and then change the parent such that the field is removed, the value is still included in formData and will be submitted, unless additionalProperties: false is included. Is this the preferred behaviour? I can see pros and cons of both resetting the value of the formData and including it, but I'm leaning towards resetting the values.
The memoization changes seem to cause issues when omitExtraData is set to true - I'm getting "maximum call size exceeded". See attached screenshot.

jimmycallin · 2021-07-21T23:24:56Z

packages/core/src/utils.js

 import fill from "core-js-pure/features/array/fill";
 import union from "lodash/union";
 import jsonpointer from "jsonpointer";
 import fields from "./components/fields";
 import widgets from "./components/widgets";
 import validateFormData, { isValid } from "./validate";
+import _ from "lodash";


To keep the build size down, it's preferable to import just the necessary function similarly to line 4.

Suggested change

import _ from "lodash";

import memoize from "lodash/memoize";

jimmycallin · 2021-07-21T23:30:35Z

packages/core/src/utils.js

  if (schema.hasOwnProperty("$ref")) {
    return resolveReference(schema, rootSchema, formData);
  } else if (schema.hasOwnProperty("dependencies")) {
    const resolvedSchema = resolveDependencies(schema, rootSchema, formData);
    return retrieveSchema(resolvedSchema, rootSchema, formData);
-  } else if (schema.hasOwnProperty("allOf")) {
+  } else if (schema["allOf"]) {


what's the reason for this change?

allOf may be set to undefined here: https://github.com/nickgros/react-jsonschema-form/blob/5e7a08dce80c0c1c85293ac3465c63d65693c6d4/packages/core/src/utils.js#L767

I think I changed that per a comment on the previous PR. Could use delete instead and revert this.

Switching that line to delete actually fixed the liveOmit issue that I mentioned here. I'm not sure if it fixed the stack overflow error that you saw.

jimmycallin · 2021-07-21T23:52:16Z

docs/index.md

+
+For arrays this is not the case. Defining an array, when a parent also defines an array, will be overwritten. This is only true when arrays are used in the same level, for objects within these arrays, they will be deeply merged again.
+
+## Tips and tricks


Not sure what's happening here with these docs changes, they are unrelated to the PR and are also out commented. I guess this should be reverted?

jimmycallin · 2021-07-21T23:53:47Z

packages/core/src/components/fields/SchemaField.js

@@ -249,7 +249,8 @@ function SchemaFieldRender(props) {
  const FieldTemplate =
    uiSchema["ui:FieldTemplate"] || registry.FieldTemplate || DefaultTemplate;
  let idSchema = props.idSchema;
-  const schema = retrieveSchema(props.schema, rootSchema, formData);
+  var schema = retrieveSchema(props.schema, rootSchema, formData);


I'm assuming all const -> var related changes are due to the original PR being a bit old. In any case, const is highly preferable to var, so please revert all of these changes.

nickgros · 2021-07-22T13:51:51Z

Thanks for taking a look @jimmycallin!

Currently if you type a value in a conditional field, and then change the parent such that the field is removed, the value is still included in formData and will be submitted, unless additionalProperties: false is included. Is this the preferred behaviour? I can see pros and cons of both resetting the value of the formData and including it, but I'm leaning towards resetting the values.

For my use case, I need the option to keep data as an additional property if its field is removed. As our users fill out the form, we want to be as nondestructive as possible with their data, and give them the opportunity to 'backtrack' without having to re-fill fields that they may have already completed.

Shouldn't this concern be handled by omitExtraData and liveOmit? It seems like liveOmit is working properly (except for your concern) ~~, but nothing is being omitted on submit if liveOmit is false, so that will need to be fixed (I'm actually seeing this issue in the current playground version 😬)~~ . I'm also not actually seeing a difference in behavior when I change additionalProperties like you note.

The memoization changes seem to cause issues when omitExtraData is set to true - I'm getting "maximum call size exceeded". See attached screenshot.

Good catch, and thanks for attaching the stack trace! I'm having trouble reproducing this, but you guided me to find that I can't actually change the data in the "All Of If Then Else" example if liveOmit is true. Possible that these are related issues.

Edit: I realize now that unless liveOmit is enabled, omitExtraData applies only to the submitted data and not the formData.

…a-form into conditional-support

jeremypele · 2021-08-04T11:56:04Z

Thanks for that PR @nickgros 🎉

Is there anything I can help you with to be able to complete this feature? Would love to this merged sooner than later

Added if then else logic to resolve schemas

dieseldjango · 2022-02-09T22:07:01Z

I need support for if-then-else and it's great to see there's a PR, but appears stalled. Anything I can help with?

…a-form into conditional-support

…chema-form into conditional-support

nickgros · 2022-02-10T17:23:11Z

I don't yet want to close this because I think there's still some value in my changes, but I think #2506 is a better foundation. I'll see if I can integrate those changes into this PR (and in doing so, see if there's a meaningful difference in behavior between how these handle if/then/else)

…schema-form into conditional-support

* Added tests Added if then else logic to resolve schemas * Changed resolve method's name * Added $ref tests * Code review changes * Remove only on test, add integration tests from #2466 * Fixed merge order * Update tests method names * Code review changes #2700 Co-authored-by: Juansasa <[email protected]> Co-authored-by: Quang Vu <[email protected]>

…a-form into conditional-support

heath-freenome · 2022-09-07T18:05:55Z

@nickgros Does it make sense to rethink these changes on top of the v5 beta's @rjsf/validator-ajv8?

nickgros · 2023-02-10T16:52:03Z

This is probably not the right way to optimize validation. Closing

Saulzi and others added 7 commits June 23, 2021 09:20

Fix playground example

51387a0

Fix resolveSchema to handle more cases

fca3592

Don't remove the root schema after validating

ba15fd3

Warn when encountering error in validation

75665ca

Add tests to cover error cases

23288ae

Merge branch 'master' of https://github.com/rjsf-team/react-jsonschem…

4cf5f7f

…a-form into conditional-support

nickgros commented Jul 12, 2021

View reviewed changes

nickgros changed the title ~~Implementation of If Then Else~~ Implementation of If Then Else, better support for const Jul 12, 2021

This was referenced Jul 12, 2021

Implementation of If Then Else / Const v2 #1666

Closed

ANNOUNCEMENT: Breaking changes planned for v4 #2437

Closed

nickgros force-pushed the conditional-support branch from 39c012b to 4cf5f7f Compare July 13, 2021 21:04

Memoize withIdRefPrefix and prevent recompiling during form validation

78fc62c

nickgros marked this pull request as draft July 15, 2021 18:22

nickgros added 2 commits July 15, 2021 16:47

Fix tests

58d3335

Memoize all the things

5e7a08d

jimmycallin reviewed Jul 21, 2021

View reviewed changes

Import only memoize from lodash

667903b

nickgros added 2 commits July 22, 2021 10:11

Revert outdated docs reorganization

ab36d1e

Revert const changes

b00f49f

nickgros changed the title ~~Implementation of If Then Else, better support for const~~ Implementation of If Then Else Jul 22, 2021

nickgros added 3 commits July 22, 2021 10:42

Remove default value from enum example

833006a

Delete allOf rather than set to undefined. Fixes liveOmit issue.

8796c9d

Merge branch 'master' of https://github.com/rjsf-team/react-jsonschem…

b6c53c8

…a-form into conditional-support

nickgros marked this pull request as ready for review July 22, 2021 14:47

Added tests

f72a62c

Added if then else logic to resolve schemas

Juansasa mentioned this pull request Aug 6, 2021

Support for if, else, then rules #2506

Closed

7 tasks

Changed resolve method's name

75c4d84

epicfaace self-requested a review August 11, 2021 14:30

Juansasa and others added 2 commits August 13, 2021 10:51

Added $ref tests

a08c563

Merge branch 'master' into conditional-support

07a43c8

nickgros added 2 commits February 10, 2022 12:19

Merge branch 'master' of https://github.com/rjsf-team/react-jsonschem…

52747fb

…a-form into conditional-support

Merge branch 'conditional-support' of github.com:nickgros/react-jsons…

123c480

…chema-form into conditional-support

Merge branch 'if_then_else' of https://github.com/stakater/react-json…

964f537

…schema-form into conditional-support

nickgros added a commit to nickgros/react-jsonschema-form that referenced this pull request Feb 10, 2022

Remove only on test, add integration tests from rjsf-team#2466

76ea7ba

nickgros mentioned this pull request Feb 10, 2022

If/Then/Else - Code Review changes from #2506 and tests from #2466 #2700

Merged

8 tasks

Merge branch 'master' of https://github.com/rjsf-team/react-jsonschem…

544133b

…a-form into conditional-support

nickgros marked this pull request as draft February 20, 2022 13:39

nickgros changed the title ~~Implementation of If Then Else~~ Optimizations to improve performance on complex conditional schemas Feb 20, 2022

nickgros mentioned this pull request May 4, 2022

Performance Issue: Frequently re-compiling (sub)schemas in AJV #1961

Closed

heath-freenome added awaiting response v5 refactor Needs refactor due to v5 breaking changes labels Sep 7, 2022

nickgros closed this Feb 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimizations to improve performance on complex conditional schemas #2466

Optimizations to improve performance on complex conditional schemas #2466

nickgros commented Jul 12, 2021 •

edited

Loading

nickgros Jul 12, 2021

nickgros Jul 13, 2021

nickgros Jul 14, 2021 •

edited

Loading

nickgros Jul 15, 2021 •

edited

Loading

nickgros Jul 12, 2021

nickgros Jul 12, 2021 •

edited

Loading

nickgros commented Jul 15, 2021

jimmycallin commented Jul 21, 2021

jimmycallin Jul 21, 2021

jimmycallin Jul 21, 2021

nickgros Jul 22, 2021

nickgros Jul 22, 2021

jimmycallin Jul 21, 2021

jimmycallin Jul 21, 2021

nickgros commented Jul 22, 2021 •

edited

Loading

jeremypele commented Aug 4, 2021 •

edited

Loading

dieseldjango commented Feb 9, 2022

nickgros commented Feb 10, 2022

heath-freenome commented Sep 7, 2022

nickgros commented Feb 10, 2023

	import _ from "lodash";
	import memoize from "lodash/memoize";


		For arrays this is not the case. Defining an array, when a parent also defines an array, will be overwritten. This is only true when arrays are used in the same level, for objects within these arrays, they will be deeply merged again.

		## Tips and tricks

Optimizations to improve performance on complex conditional schemas #2466

Optimizations to improve performance on complex conditional schemas #2466

Conversation

nickgros commented Jul 12, 2021 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickgros Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

nickgros Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickgros Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

nickgros commented Jul 15, 2021

jimmycallin commented Jul 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickgros commented Jul 22, 2021 • edited Loading

jeremypele commented Aug 4, 2021 • edited Loading

dieseldjango commented Feb 9, 2022

nickgros commented Feb 10, 2022

heath-freenome commented Sep 7, 2022

nickgros commented Feb 10, 2023

nickgros commented Jul 12, 2021 •

edited

Loading

nickgros Jul 14, 2021 •

edited

Loading

nickgros Jul 15, 2021 •

edited

Loading

nickgros Jul 12, 2021 •

edited

Loading

nickgros commented Jul 22, 2021 •

edited

Loading

jeremypele commented Aug 4, 2021 •

edited

Loading