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

fix: fixed many issues in oneOf/anyOf functions #3392

Merged
merged 8 commits into from
Jan 24, 2023

Conversation

heath-freenome
Copy link
Member

@heath-freenome heath-freenome commented Jan 20, 2023

Reasons for making this change

Fixes #3236, #2978, #2944, #2202, #2183, #2086, #2069, #1661 and possibly others
Fixes #2538 by dealing with additionalProperties with anyOf/oneOf
Fixes #1654 by inferring types from the oneOf/anyOf

  • In @rjsf/utils, added new getClosestMatchingOption(), getFirstMatchingOption() and sanitizeDataForNewSchema() schema-based utility functions
    • Deprecated getMatchingOption() and updated all calls to it in other utility functions to use getFirstMatchingOption()
    • Added 100% unit tests for all new functions, renaming the old getMatchingOptionsTest.ts file to getFirstMatchingOptionsTest.ts
    • Updated createSchemaUtils() and it's associated type to add the three new functions
    • Updated stubExistingAdditionalProperties to deal with additionalProperties with anyOf/oneOf
    • Updated getSchemaType() to grab the type of the first element of a oneOf/anyOf
  • In @rjsf/validator-ajv6 and @rjsf/validator-ajv8, updated the schema.tests.ts to add the new tests for the new schema-based utility functions
  • In @rjsf/core, updated the MultiSchemaField to use the new getClosestMatchingOption() and sanitizeDataForNewSchema() utility functions
    • Also updated the render to properly pass props to the widget and the schema field
    • Updated ObjectField to deal with additionalProperties with anyOf/oneOf
  • In @rjsf/playground, updated onFormDataEdited() to only change the formData in the state if the JSON.stringify() of the old and new values are different
    • Also updated the npm start command to add the --force option to avoid issues where changes made to other packages weren't getting picked up due to vite caching
  • Updated the utility-functions.md file to document the new schema-based functions and to fix up incorrect strike-through caused by the unescaped <S> generic
  • Updated the 5.x upgrade guide.md file to document the new utility functions and the deprecation of getMatchingOption()

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests. I've run npm run test:update to update snapshots, if needed.
    • I've updated docs if needed
    • I've updated the changelog with a description of the PR
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

docs/5.x upgrade guide.md Outdated Show resolved Hide resolved
@@ -231,6 +231,7 @@ render((
In version 5, all the utility functions that were previously accessed via `import { utils } from '@rjsf/core';` are now available via `import utils from '@rjsf/utils';`.
Because of the decoupling of validation from `@rjsf/core` there is a breaking change for all the [validator-based utility functions](https://react-jsonschema-form.readthedocs.io/en/stable/api-reference/utiltity-functions#validator-based-utility-functions), since they now require an additional `ValidatorType` parameter.
More over, one previously exported function `resolveSchema()` is no longer exposed in the `@rjsf/utils`, so use `retrieveSchema()` instead.
Finally, one previously exported function `getMatchingOption()` has been deprecated in favor of `getFirstMatchingOption()`.
Copy link
Member

@epicfaace epicfaace Jan 20, 2023

Choose a reason for hiding this comment

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

What's the reason for renaming it to getFirstMatchingOption? I think getMatchingOption is pretty clear ("option" not "options") and not sure if "first" is worth the rename? of course there's some ambiguity as to whether it would be the first, last, or an arbitrary one, but I think first could be a sane and intuitive default. If it really got the last matching option instead, for example, then that would be a stronger case for renaming the function to clarity.

Copy link
Member Author

Choose a reason for hiding this comment

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

the getMatchingOption() function logic loops over all of the options in a list and return the first option that is considered valid for a schema. This means that it stops looking for any other options. Sometimes there is actually an option that has a closer match to the formData that the first one. Which is why I added the getClosestMatchingOption() function. The rename better suggests the behaviour

Copy link

@cdaringe cdaringe Jan 23, 2023

Choose a reason for hiding this comment

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

i have a big problem right now where i have anyOf where two schemas have an intersecting key. if my form is empty except that common key, sometimes, the form just changes views, which is extremely disruptive.

that is:

{ type: "object", properties: { "foo": ..., ...A } }
{ type: "object", properties: { "foo": ..., ...B } }

...the form can toggle on edit, even if the anyOf option <select /> was not touched. hopefully this changeset will make it clear that if a current subschema is selected, it should not change the active view

Copy link
Member Author

Choose a reason for hiding this comment

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

@cdaringe I plan on cutting beta-18 with this change within a day. Let me know if does not fix it, but from what you shared I believe it would work properly. What is this key's type? (i.e. what is A and B defined to be?

Choose a reason for hiding this comment

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

hey @heath-freenome , thanks! i'll install the beta as soon as it's out and test if it's fixed. A & B are objects of other properties that are non-insersecting. in the above, i was poorly trying to communicate that that I have anyOf with 2 schemas, which share the field foo but share no other field names.

@heath-freenome heath-freenome force-pushed the fix-many-oneofs branch 3 times, most recently from cf1ef2b to a5a2dc5 Compare January 22, 2023 00:51
@@ -156,7 +156,7 @@ export function computeDefaults<
) as T[];
} else if (ONE_OF_KEY in schema) {
schema = schema.oneOf![
getMatchingOption<T, S, F>(
getFirstMatchingOption<T, S, F>(
Copy link
Member Author

Choose a reason for hiding this comment

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

@nickgros I'm wondering if, because of value defaulting, we actually want to use getClosestMatchingOption() here instead?? Is the slower performance for the best choice worth it? I'm on the fence but leaning towards best vs fast

Copy link
Contributor

@nickgros nickgros Jan 23, 2023

Choose a reason for hiding this comment

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

I personally think that this is so useful that I think getClosestMatchingOption should be the default behavior.

Couple of thoughts:

  • If users are relying on the existing behavior, we'll need an opt-in for the old behavior
  • If the main performance hit is from calculating scores of complex schemas, we'll probably need an opt-out.
  • If the main performance hit is because we make multiple validation checks by repeatedly calling getFirstMatchingOption, then let's see if we can optimize that (e.g. use the Ajv cache)

We can wait and see what feedback we get from users if we're not sure if any of these will be real problems.

@heath-freenome heath-freenome changed the title fix: fixed several issue in oneOf/anyOf functions fix: fixed many issues in oneOf/anyOf functions Jan 22, 2023
options: S[],
rootSchema: S
): number {
return getMatchingOption<T, S, F>(validator, formData, options, rootSchema);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer this fn to contain the implementation and getMatchingOption calls getFirstMatchingOption

Copy link
Member Author

@heath-freenome heath-freenome Jan 23, 2023

Choose a reason for hiding this comment

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

I went back and forth on this and decided to keep the choice that would simply maintaining the test coverage and the overall diff. Switching it meant duplicating the tests for getMatchingOption(). The way it is now, I was able to rename those tests to be getFirstMatchingOptionTests.

docs/api-reference/utility-functions.md Outdated Show resolved Hide resolved
docs/api-reference/utility-functions.md Outdated Show resolved Hide resolved
* @returns - The new form data, with all the fields uniquely associated with the old schema set
* to `undefined`. Will return `undefined` if the new schema is not an object containing properties.
*/
export default function sanitizeDataForNewSchema<
Copy link
Contributor

Choose a reason for hiding this comment

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

Something we should think about: what makes this different from omitExtraData? If the behavior should be the same, maybe we can use this instead of whatever the current logic is.

Copy link
Member Author

@heath-freenome heath-freenome Jan 23, 2023

Choose a reason for hiding this comment

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

I feel like this is different than omitExtraData in that when we change from one schema to another we are trying to remove anything that doesn't match the old schema AND keep anything that does. The old implementation just removed things, especially objects since the nesting of data wasn't checked. Plus omitExtraData is only for when submitting

Copy link
Member Author

Choose a reason for hiding this comment

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

It is different as omitExtraData only happens on submit

// any non-matching value lower
newScore += formValue === value.const ? 1 : -1;
}
// TODO eventually, deal with enums/arrays
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with handling this case later, too.

@@ -156,7 +156,7 @@ export function computeDefaults<
) as T[];
} else if (ONE_OF_KEY in schema) {
schema = schema.oneOf![
getMatchingOption<T, S, F>(
getFirstMatchingOption<T, S, F>(
Copy link
Contributor

@nickgros nickgros Jan 23, 2023

Choose a reason for hiding this comment

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

I personally think that this is so useful that I think getClosestMatchingOption should be the default behavior.

Couple of thoughts:

  • If users are relying on the existing behavior, we'll need an opt-in for the old behavior
  • If the main performance hit is from calculating scores of complex schemas, we'll probably need an opt-out.
  • If the main performance hit is because we make multiple validation checks by repeatedly calling getFirstMatchingOption, then let's see if we can optimize that (e.g. use the Ajv cache)

We can wait and see what feedback we get from users if we're not sure if any of these will be real problems.

(newOptionDefault && newOptionDefault !== formValue) ||
(newOptionConst && newOptionConst !== formValue)
) {
removeOldSchemaData[key] = newOptionDefault;
Copy link
Contributor

Choose a reason for hiding this comment

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

Trying to think through if this will be forward-compatible with #2980 if/when we get to that. Can we make this undefined in the default case? Then MultiSchemaField should call getDefaultFormState, and that would fill in the default value based on the flag that we would add in the change for #2980.

Copy link
Member Author

Choose a reason for hiding this comment

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

Making this undefined actually didn't fix one or two of the issues. In this case two schemas had a single named property but each had a different default value. When you changed from the first to the second, clearing the value here kept the first selected because getDefaultFormState's use of getFirstMatchingOption would choose the first option again. By switching from one default to another, it picked the second.

packages/utils/test/schema/sanitizeDataForNewSchemaTest.ts Outdated Show resolved Hide resolved
packages/utils/src/schema/sanitizeDataForNewSchema.ts Outdated Show resolved Hide resolved
Fixes rjsf-team#2944, rjsf-team#3236, rjsf-team#2978 and possibly others
- In `@rjsf/utils`, added new `getClosestMatchingOption()`, `getFirstMatchingOption()` and `sanitizeDataForNewSchema()` schema-based utility functions
  - Deprecated `getMatchingOption()` and updated all calls to it in other utility functions to use `getFirstMatchingOption()`
  - Added 100% unit tests for all new functions, renaming the old `getMatchingOptionsTest.ts` file to `getFirstMatchingOptionsTest.ts`
  - Updated `createSchemaUtils()` and it's associated type to add the three new functions
- In `@rjsf/validator-ajv6` and `@rjsf/validator-ajv8`, updated the `schema.tests.ts` to add the new tests for the new schema-based utility functions
- In `@rjsf/core`, updated the `MultiSchemaField` to use the new `getClosestMatchingOption()` and `sanitizeDataForNewSchema()` utility functions
  - Also updated the render to properly pass props to the widget and the schema field
- In `@rjsf/playground`, updated `onFormDataEdited()` to only change the formData in the state if the `JSON.stringify()` of the old and new values are different
  - Also updated the `npm start` command to add the `--force` option to avoid issues where changes made to other packages weren't getting picked up due to `vite` caching
- Updated the `utility-functions.md` file to document the new schema-based functions and to fix up incorrect strike-through caused by the unescaped `<S>` generic
- Updated the `5.x upgrade guide.md` file to document the new utility functions and the deprecation of `getMatchingOption()`
…jsf-team#2375

- Also updated the `CHANGELOG.md` to include all of the fixed issues
Comment on lines +33 to +37
## @rjsf/material-ui
- Fix shrinking of `SelectWidget` label only if value is not empty, fixing [#3369](https://github.com/rjsf-team/react-jsonschema-form/issues/3369)

## @rjsf/mui
- Fix shrinking of `SelectWidget` label only if value is not empty, fixing [#3369](https://github.com/rjsf-team/react-jsonschema-form/issues/3369)
Copy link
Member Author

Choose a reason for hiding this comment

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

Added these for #3388 so I could merge it without the CHANGELOG changes

… handle readOnly default values like const

- Updated some documentation in the types and createSchemaUtils
@heath-freenome heath-freenome merged commit ff431e4 into rjsf-team:main Jan 24, 2023
@heath-freenome heath-freenome deleted the fix-many-oneofs branch January 24, 2023 00:35
shijistar pushed a commit to shijistar/react-jsonschema-form that referenced this pull request Jun 8, 2023
* fix: fixed several issue in oneOf/anyOf functions
Fixes rjsf-team#2944, rjsf-team#3236, rjsf-team#2978 and possibly others
- In `@rjsf/utils`, added new `getClosestMatchingOption()`, `getFirstMatchingOption()` and `sanitizeDataForNewSchema()` schema-based utility functions
  - Deprecated `getMatchingOption()` and updated all calls to it in other utility functions to use `getFirstMatchingOption()`
  - Added 100% unit tests for all new functions, renaming the old `getMatchingOptionsTest.ts` file to `getFirstMatchingOptionsTest.ts`
  - Updated `createSchemaUtils()` and it's associated type to add the three new functions
- In `@rjsf/validator-ajv6` and `@rjsf/validator-ajv8`, updated the `schema.tests.ts` to add the new tests for the new schema-based utility functions
- In `@rjsf/core`, updated the `MultiSchemaField` to use the new `getClosestMatchingOption()` and `sanitizeDataForNewSchema()` utility functions
  - Also updated the render to properly pass props to the widget and the schema field
- In `@rjsf/playground`, updated `onFormDataEdited()` to only change the formData in the state if the `JSON.stringify()` of the old and new values are different
  - Also updated the `npm start` command to add the `--force` option to avoid issues where changes made to other packages weren't getting picked up due to `vite` caching
- Updated the `utility-functions.md` file to document the new schema-based functions and to fix up incorrect strike-through caused by the unescaped `<S>` generic
- Updated the `5.x upgrade guide.md` file to document the new utility functions and the deprecation of `getMatchingOption()`

* - Fixed a few small issues exposed by trying to use the playground in rjsf-team#2375
- Also updated the `CHANGELOG.md` to include all of the fixed issues

* - Fix rjsf-team#2538 by fixing additionalProperties to deal with allOf/anyOf/oneOf

* - Updated `getSchemaType()` to grab the type of the first element of a `oneOf`/`anyOf`

* - Allow `formData` in `getClosestMatchingOption()` to accept `undefined`

* - Responded to reviewer feedback

* - Deal with sanitizing data when both `array.items` elements are booleans and have the same value

* - Fixed issue with const being assigned default value incorrectly and handle readOnly default values like const
- Updated some documentation in the types and createSchemaUtils
shijistar pushed a commit to shijistar/react-jsonschema-form that referenced this pull request Jun 8, 2023
* fix: fixed several issue in oneOf/anyOf functions
Fixes rjsf-team#2944, rjsf-team#3236, rjsf-team#2978 and possibly others
- In `@rjsf/utils`, added new `getClosestMatchingOption()`, `getFirstMatchingOption()` and `sanitizeDataForNewSchema()` schema-based utility functions
  - Deprecated `getMatchingOption()` and updated all calls to it in other utility functions to use `getFirstMatchingOption()`
  - Added 100% unit tests for all new functions, renaming the old `getMatchingOptionsTest.ts` file to `getFirstMatchingOptionsTest.ts`
  - Updated `createSchemaUtils()` and it's associated type to add the three new functions
- In `@rjsf/validator-ajv6` and `@rjsf/validator-ajv8`, updated the `schema.tests.ts` to add the new tests for the new schema-based utility functions
- In `@rjsf/core`, updated the `MultiSchemaField` to use the new `getClosestMatchingOption()` and `sanitizeDataForNewSchema()` utility functions
  - Also updated the render to properly pass props to the widget and the schema field
- In `@rjsf/playground`, updated `onFormDataEdited()` to only change the formData in the state if the `JSON.stringify()` of the old and new values are different
  - Also updated the `npm start` command to add the `--force` option to avoid issues where changes made to other packages weren't getting picked up due to `vite` caching
- Updated the `utility-functions.md` file to document the new schema-based functions and to fix up incorrect strike-through caused by the unescaped `<S>` generic
- Updated the `5.x upgrade guide.md` file to document the new utility functions and the deprecation of `getMatchingOption()`

* - Fixed a few small issues exposed by trying to use the playground in rjsf-team#2375
- Also updated the `CHANGELOG.md` to include all of the fixed issues

* - Fix rjsf-team#2538 by fixing additionalProperties to deal with allOf/anyOf/oneOf

* - Updated `getSchemaType()` to grab the type of the first element of a `oneOf`/`anyOf`

* - Allow `formData` in `getClosestMatchingOption()` to accept `undefined`

* - Responded to reviewer feedback

* - Deal with sanitizing data when both `array.items` elements are booleans and have the same value

* - Fixed issue with const being assigned default value incorrectly and handle readOnly default values like const
- Updated some documentation in the types and createSchemaUtils
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants