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.

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

To improve performance, skip validating subschemas in oneOf / anyOf if formData is undefined #2676

Merged
merged 9 commits into from
Mar 1, 2022

Conversation

KittyJose
Copy link
Contributor

Fixing getMatchingOption in the case of oneOf/anyOf by validating schema only when formData is available

Reasons for making this change

When there is a property with oneOf/anyOf - schema gets validated when there is not formData provided which leads to lot of load time. This fix will improves speed and performance.

@KittyJose
Copy link
Contributor Author

@epicfaace , I have added a test to utils_test.js, in which I pass undefined as formData to getMatchingOption(), and it returns 0. My point is, if we dont pass formData to getMatchingOption => then there is no need to validate data against the whole schema.

In my schema, I use anyOfs (100 to 200 anyOfs) , and the check in utils.js passes undefined as formData as shown in the below pic. I also have a very large schema and multiple checks to validate formData against schema is very slow.

image

Maybe this is not the right solution, I can pass you my huge schema and you can paste it in playground and see it takes lot of time to load, eventually it loads after 3 minutes.

https://github.com/terminusdb/terminusdb-documents-ui/blob/main/seshatSchema.json

Any help would be really appreciated, we can set up a call as well, but do let me know what has to be done.

Thank you so much :)

@KittyJose
Copy link
Contributor Author

Ok, thanks @epicfaace, I shall join the call on discord this Friday 11 am ET :)

@@ -1230,10 +1230,11 @@ export function getMatchingOption(formData, options, rootSchema) {
// been filled in yet, which will mean that the schema is not valid
delete augmentedSchema.required;

if (isValid(augmentedSchema, formData, rootSchema)) {
if (formData && isValid(augmentedSchema, formData, rootSchema)) {
Copy link
Member

Choose a reason for hiding this comment

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

At the beginning of the function -- let's add:

  // For performance, skip validating subschemas if formData is undefined. We just
  // want to get the first option in that case.
  if (formData === undefined) {
    return 0;
  }

Copy link
Member

Choose a reason for hiding this comment

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

And remove the other changes in this file.

Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

Thanks! Can you fix the lint so the CI passes?

@KittyJose
Copy link
Contributor Author

@epicfaace ok I have fixed the lint issues :)

@epicfaace
Copy link
Member

@Michal-Sta
Copy link

I have a question about that - is it fix that issue too? #1564

@rrooij
Copy link
Contributor

rrooij commented Feb 21, 2022

The linter issues should be fixed. 😄

@KittyJose
Copy link
Contributor Author

@epicfaace, Hello, Can you contact me on discord (Kitty Jose). I would like to discuss a matter with you. I cant seem to find ur username on discord.Thanks :D

@epicfaace
Copy link
Member

I'm not sure what your discord username is -- mine is Ashwin#0168.

Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

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

Can you update this branch with the latest from master and add an entry to CHANGELOG.md?

@rrooij rrooij force-pushed the fixing-getMatchingOption branch from 56dc455 to 3971c20 Compare February 25, 2022 15:43
@rrooij
Copy link
Contributor

rrooij commented Feb 25, 2022

We added an entry to the changelog and rebased to master. 😄

CHANGELOG.md Outdated
@@ -19,6 +19,10 @@ should change the heading of the (upcoming) version to include a major version b

# v4.0.2 (upcoming)

## @rjsf/core

- For performance, skip validating subschemas if formData is undefined (#2676)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- For performance, skip validating subschemas if formData is undefined (#2676)
- To improve performance, skip validating subschemas in oneOf / anyOf if formData is undefined (#2676)

Copy link
Member

Choose a reason for hiding this comment

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

Can you make this change? I don't have access to edit. Thanks!

CHANGELOG.md Outdated
@@ -19,6 +19,10 @@ should change the heading of the (upcoming) version to include a major version b

# v4.0.2 (upcoming)

## @rjsf/core

- For performance, skip validating subschemas if formData is undefined (#2676)
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this change? I don't have access to edit. Thanks!

@rrooij
Copy link
Contributor

rrooij commented Feb 28, 2022

Did the change 👍 Also merged with the latest master again.

KittyJose and others added 9 commits February 28, 2022 09:54
Fixing getMatchingOption in the case of oneOf/anyOf by validating schema only when formData is available
add proper return on formData check in getMatchingOption()
Test utils when formData is undefined
For performance, skip validating subschemas if formData is undefined. We just want to get the first option in that case.
moving formData check to beginning of getMatchingOption()
Add a test for getMatchingOption

where formData is null
Where option 2 is {type: null}
We expect getMatchingOption to be equal to 2
after fixing lint errors
@rrooij rrooij force-pushed the fixing-getMatchingOption branch from 0221c89 to 9b38d3a Compare February 28, 2022 08:54
@epicfaace epicfaace changed the title Fixing getMatchingOption To improve performance, skip validating subschemas in oneOf / anyOf if formData is undefined Feb 28, 2022
@epicfaace epicfaace merged commit 3052f5f into rjsf-team:master Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants