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: add condition to pick correct schemas for fixed array items in topathschemainternal util #3916

Merged
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ should change the heading of the (upcoming) version to include a major version b
-->
# 5.13.3

## @rjsf/utils

- Updated `toPathSchemaInternal()` util to generate correct path schemas for fixed arrays by picking up individual schemas in the `items` array, fixing [#3909](https://github.com/rjsf-team/react-jsonschema-form/issues/3909)

## @rjsf/core

- Replaced the deprecated `UNSAFE_componentWillReceiveProps()` method in the Form.tsx component with an improved solution utilizing the React lifecycle methods: `getSnapshotBeforeUpdate()` and `componentDidUpdate()`. Fixing [#1794](https://github.com/rjsf-team/react-jsonschema-form/issues/1794)
Expand Down
48 changes: 38 additions & 10 deletions packages/utils/src/schema/toPathSchema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,16 +73,44 @@ function toPathSchemaInternal<T = any, S extends StrictRJSFSchema = RJSFSchema,
}

if (ITEMS_KEY in schema && Array.isArray(formData)) {
formData.forEach((element, i: number) => {
pathSchema[i] = toPathSchemaInternal<T, S, F>(
validator,
schema.items as S,
`${name}.${i}`,
rootSchema,
element,
_recurseList
);
});
const { items: schemaItems, additionalItems: schemaAdditionalItems } = schema;

if (Array.isArray(schemaItems)) {
formData.forEach((element, i: number) => {
if (schemaItems[i]) {
pathSchema[i] = toPathSchemaInternal<T, S, F>(
validator,
schemaItems[i] as S,
`${name}.${i}`,
rootSchema,
element,
_recurseList
);
} else if (schemaAdditionalItems) {
pathSchema[i] = toPathSchemaInternal<T, S, F>(
validator,
schemaAdditionalItems as S,
`${name}.${i}`,
rootSchema,
element,
_recurseList
);
} else {
console.warn(`Unable to generate path schema for "${name}.${i}". No schema defined for it`);
}
Copy link
Member

Choose a reason for hiding this comment

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

For the moment, let's add a console.warn() here to let users know that they have unhandled formData. Which means we want to update the test below with a check that console.warn() was called. This can be done with a jest.spyOn()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Added a message. Let me know if there is a change required.

Copy link
Contributor Author

@Rozamo Rozamo Nov 4, 2023

Choose a reason for hiding this comment

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

@heath-freenome Should this log be moved up a level outside the for loop? We don't know how many items there will be in the array and if there are too many items, it might clutter the console.

});
} else {
formData.forEach((element, i: number) => {
pathSchema[i] = toPathSchemaInternal<T, S, F>(
validator,
schemaItems as S,
`${name}.${i}`,
rootSchema,
element,
_recurseList
);
});
}
} else if (PROPERTIES_KEY in schema) {
for (const property in schema.properties) {
const field = get(schema, [PROPERTIES_KEY, property]);
Expand Down
188 changes: 188 additions & 0 deletions packages/utils/test/schema/toPathSchemaTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,17 @@ import { TestValidatorType } from './types';

export default function toPathSchemaTest(testValidator: TestValidatorType) {
describe('toPathSchema()', () => {
let consoleWarnSpy: jest.SpyInstance;

beforeAll(() => {
// spy on console.warn() and make it do nothing to avoid making noise in the test
consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation();
});

afterAll(() => {
consoleWarnSpy.mockRestore();
});

it('should return a pathSchema for root field', () => {
const schema: RJSFSchema = { type: 'string' };

Expand Down Expand Up @@ -702,5 +713,182 @@ export default function toPathSchemaTest(testValidator: TestValidatorType) {
},
});
});

it('should handle fixed array items which are objects', () => {
const schema: RJSFSchema = {
type: 'object',
properties: {
str: { type: 'string' },
arr: {
type: 'array',
items: [
{
type: 'object',
properties: {
name: { type: 'string' },
},
},
],
},
},
};

const formData = {
str: 'string',
arr: [{ name: 'name1' }],
};

expect(toPathSchema(testValidator, schema, '', schema, formData)).toEqual({
$name: '',
arr: {
$name: 'arr',
'0': {
$name: 'arr.0',
name: {
$name: 'arr.0.name',
},
},
},
str: {
$name: 'str',
},
});
});

it('should handle fixed array items which are objects, which have additionalItems in the schema and have additional items in the form data', () => {
const schema: RJSFSchema = {
type: 'object',
properties: {
str: { type: 'string' },
arr: {
type: 'array',
items: [
{
type: 'object',
properties: {
name: { type: 'string' },
},
},
],
additionalItems: {
type: 'string',
},
},
},
};

const formData = {
str: 'string',
arr: [{ name: 'name1' }, 'name2'],
};

expect(toPathSchema(testValidator, schema, '', schema, formData)).toEqual({
$name: '',
arr: {
$name: 'arr',
'0': {
$name: 'arr.0',
name: {
$name: 'arr.0.name',
},
},
'1': {
$name: 'arr.1',
},
},
str: {
$name: 'str',
},
});
});

it("should handle fixed array items which are objects, which have additionalItems in the schema but don't have additional items in the form data", () => {
const schema: RJSFSchema = {
type: 'object',
properties: {
str: { type: 'string' },
arr: {
type: 'array',
items: [
{
type: 'object',
properties: {
name: { type: 'string' },
},
},
],
additionalItems: {
type: 'string',
},
},
},
};

const formData = {
str: 'string',
arr: [{ name: 'name1' }],
};

expect(toPathSchema(testValidator, schema, '', schema, formData)).toEqual({
$name: '',
arr: {
$name: 'arr',
'0': {
$name: 'arr.0',
name: {
$name: 'arr.0.name',
},
},
},
str: {
$name: 'str',
},
});
});

it('should handle fixed array items which are objects, which have additionalItems as false in the schema but have additional items in the form data', () => {
const schema: RJSFSchema = {
type: 'object',
properties: {
str: { type: 'string' },
arr: {
type: 'array',
items: [
{
type: 'object',
properties: {
name: { type: 'string' },
},
},
],
additionalItems: false,
},
},
};

const formData = {
str: 'string',
arr: [{ name: 'name1' }, 'name2'],
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 wondering whether there should be a console.warn or console.error for this condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see that toPathSchema util is mostly used as a step to omit extra data. In that case should we warn the users about the omitted data?

};

expect(toPathSchema(testValidator, schema, '', schema, formData)).toEqual({
$name: '',
arr: {
$name: 'arr',
'0': {
$name: 'arr.0',
name: {
$name: 'arr.0.name',
},
},
},
str: {
$name: 'str',
},
});
expect(consoleWarnSpy).toHaveBeenCalledWith(
'Unable to generate path schema for ".arr.1". No schema defined for it'
);
});
});
}