From 4571c0fc8610fc778af7ca1731ebde967f791696 Mon Sep 17 00:00:00 2001 From: Alexandra Buzila Date: Thu, 4 Nov 2021 15:13:37 +0100 Subject: [PATCH] Fix ref resolving for oneOf and array schemas - resolve items subschema refs for renderers and testers - make rootSchema a required parameter in resolveSchema, as it is needed while resolving - use resolved schemas in MaterialOneOfRenderer when computing default value - enable and update tests - remove 'Resolve Remote Schema' example as it's unsupported --- packages/core/src/generators/uischema.ts | 4 +- packages/core/src/testers/testers.ts | 15 +++- packages/core/src/util/combinators.ts | 4 +- packages/core/src/util/renderer.ts | 9 +- packages/core/src/util/resolvers.ts | 49 ++++------- packages/core/src/util/util.ts | 24 +++-- packages/core/test/testers.test.ts | 73 ++++++++++++++++ packages/core/test/util/resolvers.test.ts | 16 ++-- packages/examples/src/examples/resolve.ts | 87 ------------------- packages/examples/src/index.ts | 2 - .../src/complex/MaterialAllOfRenderer.tsx | 8 +- .../src/complex/MaterialAnyOfRenderer.tsx | 6 +- .../src/complex/MaterialOneOfRenderer.tsx | 8 +- .../renderers/MaterialOneOfRenderer.test.tsx | 19 ++-- .../react/test/renderers/JsonForms.test.tsx | 11 +-- 15 files changed, 160 insertions(+), 175 deletions(-) delete mode 100644 packages/examples/src/examples/resolve.ts diff --git a/packages/core/src/generators/uischema.ts b/packages/core/src/generators/uischema.ts index 865259392b..be1164efcf 100644 --- a/packages/core/src/generators/uischema.ts +++ b/packages/core/src/generators/uischema.ts @@ -122,7 +122,7 @@ const generateUISchema = ( ): UISchemaElement => { if (!isEmpty(jsonSchema) && jsonSchema.$ref !== undefined) { return generateUISchema( - resolveSchema(rootSchema, jsonSchema.$ref), + resolveSchema(rootSchema, jsonSchema.$ref, rootSchema), schemaElements, currentRef, schemaName, @@ -164,7 +164,7 @@ const generateUISchema = ( let value = jsonSchema.properties[propName]; const ref = `${nextRef}/${encode(propName)}`; if (value.$ref !== undefined) { - value = resolveSchema(rootSchema, value.$ref); + value = resolveSchema(rootSchema, value.$ref, rootSchema); } generateUISchema( value, diff --git a/packages/core/src/testers/testers.ts b/packages/core/src/testers/testers.ts index 6790b61932..ba710a6741 100644 --- a/packages/core/src/testers/testers.ts +++ b/packages/core/src/testers/testers.ts @@ -86,7 +86,10 @@ export const schemaMatches = ( } let currentDataSchema = schema; if (hasType(schema, 'object')) { - currentDataSchema = resolveSchema(schema, schemaPath); + currentDataSchema = resolveSchema(schema, schemaPath, schema); + } + if (hasType(currentDataSchema, 'array')) { + currentDataSchema = { ...currentDataSchema, items: (resolveSchema(currentDataSchema, 'items', schema)) } as JsonSchema } if (currentDataSchema === undefined) { return false; @@ -105,7 +108,10 @@ export const schemaSubPathMatches = ( const schemaPath = uischema.scope; let currentDataSchema: JsonSchema = schema; if (hasType(schema, 'object')) { - currentDataSchema = resolveSchema(schema, schemaPath); + currentDataSchema = resolveSchema(schema, schemaPath, schema); + } + if (hasType(currentDataSchema, 'array')) { + currentDataSchema = { ...currentDataSchema, items: (resolveSchema(currentDataSchema, 'items', schema)) } as JsonSchema } currentDataSchema = get(currentDataSchema, subPath); @@ -425,7 +431,7 @@ export const isObjectArrayWithNesting = ( return false; } const schemaPath = (uischema as ControlElement).scope; - const resolvedSchema = resolveSchema(schema, schemaPath); + const resolvedSchema = resolveSchema(schema, schemaPath, schema); const wantedNestingByType: { [key: string]: number } = { object: 2, array: 1 @@ -437,6 +443,9 @@ export const isObjectArrayWithNesting = ( if (val === schema) { return false; } + if (val.$ref !== undefined) { + return false; + } // we don't support multiple types if (typeof val.type !== 'string') { return true; diff --git a/packages/core/src/util/combinators.ts b/packages/core/src/util/combinators.ts index c89496f5db..a34c17ed75 100644 --- a/packages/core/src/util/combinators.ts +++ b/packages/core/src/util/combinators.ts @@ -24,7 +24,7 @@ */ import { ControlElement, JsonSchema, UISchemaElement } from '../models'; -import { resolveSchema } from './resolvers'; +import { resolveArraySchema, resolveSchema } from './resolvers'; import { findUISchema, JsonFormsUISchemaRegistryEntry } from '../reducers'; export interface CombinatorSubSchemaRenderInfo { @@ -58,7 +58,7 @@ export const resolveSubSchemas = ( return { ...schema, [keyword]: (schema[keyword] as any[]).map(e => - e.$ref ? resolveSchema(rootSchema, e.$ref) : e + e.$ref ? resolveArraySchema(resolveSchema(rootSchema, e.$ref, rootSchema), rootSchema) : e ) }; } diff --git a/packages/core/src/util/renderer.ts b/packages/core/src/util/renderer.ts index 2be9bd9bc4..ef17ecb4e6 100644 --- a/packages/core/src/util/renderer.ts +++ b/packages/core/src/util/renderer.ts @@ -439,11 +439,10 @@ export const mapStateToControlProps = ( const required = controlElement.scope !== undefined && isRequired(ownProps.schema, controlElement.scope, rootSchema); - const resolvedSchema = Resolve.schema( - ownProps.schema || rootSchema, - controlElement.scope, - rootSchema - ); + const resolvedSchema = Resolve.arraySchema( + Resolve.schema(ownProps.schema || rootSchema, controlElement.scope, rootSchema), + rootSchema); + const errors = getErrorAt(path, resolvedSchema)(state); const description = diff --git a/packages/core/src/util/resolvers.ts b/packages/core/src/util/resolvers.ts index 5a4c62d70f..761078dbf2 100644 --- a/packages/core/src/util/resolvers.ts +++ b/packages/core/src/util/resolvers.ts @@ -59,6 +59,17 @@ export const resolveData = (instance: any, dataPath: string): any => { }, instance); }; +export const resolveArraySchema = (schema: JsonSchema, + rootSchema: JsonSchema):JsonSchema => { + if (schema?.items && (schema.items as any).$ref) { + return { + ...schema, + items: resolveSchema(schema, 'items', rootSchema) + } as JsonSchema + } + return schema; +} + /** * Finds all references inside the given schema. * @@ -111,7 +122,7 @@ const invalidSegment = (pathSegment: string) => export const resolveSchema = ( schema: JsonSchema, schemaPath: string, - rootSchema?: JsonSchema + rootSchema: JsonSchema ): JsonSchema => { if (isEmpty(schema)) { return undefined; @@ -123,7 +134,7 @@ export const resolveSchema = ( resultSchema = resultSchema === undefined || resultSchema.$ref === undefined ? resultSchema - : resolveSchema(schema, resultSchema.$ref); + : resolveSchema(schema, resultSchema.$ref, rootSchema); if (invalidSegment(pathSegment)) { // skip invalid segments continue; @@ -137,7 +148,7 @@ export const resolveSchema = ( resultSchema?.anyOf ?? [] ); for (let item of schemas) { - curSchema = resolveSchema(item, validPathSegments.slice(i).map(encode).join('/')); + curSchema = resolveSchema(item, validPathSegments.slice(i).map(encode).join('/'), rootSchema); if (curSchema) { break; } @@ -155,36 +166,10 @@ export const resolveSchema = ( // with absolute paths here, so that we don't need to keep two different // schemas around if (resultSchema !== undefined && resultSchema.$ref !== undefined) { - try { - return retrieveResolvableSchema(schema, resultSchema.$ref); - } catch (e) { - return retrieveResolvableSchema(rootSchema, resultSchema.$ref); - } + return resolveSchema(schema, resultSchema.$ref, rootSchema) + ?? resolveSchema(rootSchema, resultSchema.$ref, rootSchema) + ?? schema; } return resultSchema; }; - -/** - * Normalizes the schema and resolves the given ref. - * - * @param {JsonSchema} full the JSON schema to resolved the reference against - * @param {string} reference the reference to be resolved - * @returns {JsonSchema} the resolved sub-schema - */ -// disable rule because resolve is mutually recursive -// tslint:disable:only-arrow-functions -function retrieveResolvableSchema( - full: JsonSchema, - reference: string -): JsonSchema { - // tslint:enable:only-arrow-functions - const child = resolveSchema(full, reference); - const allRefs = findAllRefs(child); - const innerSelfReference = allRefs[reference]; - if (innerSelfReference !== undefined) { - innerSelfReference.$ref = '#'; - } - - return child; -} diff --git a/packages/core/src/util/util.ts b/packages/core/src/util/util.ts index 946d5b54d0..873d842e5b 100644 --- a/packages/core/src/util/util.ts +++ b/packages/core/src/util/util.ts @@ -28,7 +28,7 @@ import isArray from 'lodash/isArray'; import includes from 'lodash/includes'; import find from 'lodash/find'; import { JsonSchema, Scopable, UISchemaElement } from '..'; -import { resolveData, resolveSchema } from './resolvers'; +import { resolveArraySchema, resolveData, resolveSchema } from './resolvers'; import { composePaths, toDataPathSegments } from './path'; import { isEnabled, isVisible } from './runtime'; import Ajv from 'ajv'; @@ -96,15 +96,21 @@ export const deriveTypes = (jsonSchema: JsonSchema): string[] => { * Convenience wrapper around resolveData and resolveSchema. */ export const Resolve: { - schema( - schema: JsonSchema, - schemaPath: string, - rootSchema?: JsonSchema - ): JsonSchema; - data(data: any, path: string): any; + schema( + schema: JsonSchema, + schemaPath: string, + rootSchema?: JsonSchema + ): JsonSchema; + arraySchema( + schema: JsonSchema, + rootSchema?: JsonSchema + ): JsonSchema; + data(data: any, path: string): any; + } = { - schema: resolveSchema, - data: resolveData + schema: resolveSchema, + arraySchema: resolveArraySchema, + data: resolveData }; // Paths -- diff --git a/packages/core/test/testers.test.ts b/packages/core/test/testers.test.ts index 85906946d0..a8df003473 100644 --- a/packages/core/test/testers.test.ts +++ b/packages/core/test/testers.test.ts @@ -230,6 +230,30 @@ test('schemaMatches should return false for control pointing to invalid subschem t.false(schemaMatches(() => false)(uischema, schema)); }); +test('schemaMatches should check type sub-schema of array items ref', t => { + const schema: JsonSchema = { + definitions: { + order: { + type: 'string' + } + }, + type: 'object', + properties: { + foo: { type: 'array', + items: { + $ref: '#/definitions/order' + } } + } + }; + const uischema: ControlElement = { + type: 'Control', + scope: '#/properties/foo' + }; + t.true( + schemaMatches(subSchema => (subSchema.items as any).type === 'string')(uischema, schema) + ); +}); + test('scopeEndsWith checks whether the ref of a control ends with a certain string', t => { const uischema: ControlElement = { type: 'Control', @@ -423,6 +447,28 @@ test('tester isObjectArrayControl', t => { } }; t.true(isObjectArrayControl(control, schema_innerAllOf)); + + const schemaWithRefs = { + definitions: { + order: { + type: 'object', + properties: { + x: { type: 'integer' }, + y: { type: 'integer' } + } + } + }, + type: 'object', + properties: { + foo: { + type: 'array', + items: { + $ref: '#/definitions/order' + } + } + } + } + t.true(isObjectArrayControl(control, schemaWithRefs)); }); test('isBooleanControl', t => { @@ -771,12 +817,39 @@ test('tester schemaSubPathMatches', t => { type: 'Control', scope: '#' }; + + const schemaWithRef = { + definitions: { + foobar: { type: 'number' } + }, + properties: { + test: { + type: 'array', + items: { + $ref: "#/definitions/foobar" + } + } + } + } + + const refUiSchema: ControlElement = { + type: 'Control', + scope: '#/properties/test' + }; + t.true( schemaSubPathMatches('items', items => items.type === 'number')( uischema, schema ) ); + + t.true( + schemaSubPathMatches('items', items => items.type === 'number')( + refUiSchema, + schemaWithRef + ) + ); }); test('tester not', t => { diff --git a/packages/core/test/util/resolvers.test.ts b/packages/core/test/util/resolvers.test.ts index 80f1095368..e95ecdbe1a 100644 --- a/packages/core/test/util/resolvers.test.ts +++ b/packages/core/test/util/resolvers.test.ts @@ -56,13 +56,13 @@ test('resolveSchema - resolves schema with any ', t => { } }; // test backward compatibility - t.deepEqual(resolveSchema(schema, '#/properties/description/oneOf/0/properties/name'), {type: 'string'}); - t.deepEqual(resolveSchema(schema, '#/properties/description/oneOf/1/properties/index'), {type: 'number'}); + t.deepEqual(resolveSchema(schema, '#/properties/description/oneOf/0/properties/name', schema), {type: 'string'}); + t.deepEqual(resolveSchema(schema, '#/properties/description/oneOf/1/properties/index', schema), {type: 'number'}); // new simple approach - t.deepEqual(resolveSchema(schema, '#/properties/description/properties/name'), {type: 'string'}); - t.deepEqual(resolveSchema(schema, '#/properties/description/properties/index'), {type: 'number'}); - t.deepEqual(resolveSchema(schema, '#/properties/description/properties/exist'), {type: 'boolean'}); - t.is(resolveSchema(schema, '#/properties/description/properties/notfound'), undefined); + t.deepEqual(resolveSchema(schema, '#/properties/description/properties/name', schema), {type: 'string'}); + t.deepEqual(resolveSchema(schema, '#/properties/description/properties/index', schema), {type: 'number'}); + t.deepEqual(resolveSchema(schema, '#/properties/description/properties/exist', schema), {type: 'boolean'}); + t.is(resolveSchema(schema, '#/properties/description/properties/notfound', schema), undefined); }); test('resolveSchema - resolves schema with encoded characters', t => { @@ -74,6 +74,6 @@ test('resolveSchema - resolves schema with encoded characters', t => { } } }; - t.deepEqual(resolveSchema(schema, '#/properties/foo ~1 ~0 bar'), {type: 'integer'}); - t.is(resolveSchema(schema, '#/properties/foo / bar'), undefined); + t.deepEqual(resolveSchema(schema, '#/properties/foo ~1 ~0 bar', schema), {type: 'integer'}); + t.is(resolveSchema(schema, '#/properties/foo / bar', schema), undefined); }); diff --git a/packages/examples/src/examples/resolve.ts b/packages/examples/src/examples/resolve.ts deleted file mode 100644 index 90ac48c891..0000000000 --- a/packages/examples/src/examples/resolve.ts +++ /dev/null @@ -1,87 +0,0 @@ -/* - The MIT License - - Copyright (c) 2017-2019 EclipseSource Munich - https://github.com/eclipsesource/jsonforms - - Permission is hereby granted, free of charge, to any person obtaining a copy - of this software and associated documentation files (the "Software"), to deal - in the Software without restriction, including without limitation the rights - to use, copy, modify, merge, publish, distribute, sublicense, and/or sell - copies of the Software, and to permit persons to whom the Software is - furnished to do so, subject to the following conditions: - - The above copyright notice and this permission notice shall be included in - all copies or substantial portions of the Software. - - THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE - AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, - OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN - THE SOFTWARE. -*/ -import { registerExamples } from '../register'; - -export const schema = { - definitions: { - address: { - type: 'object', - properties: { - street_address: { type: 'string' }, - city: { type: 'string' }, - state: { type: 'string' }, - coord: { $ref: '#/definitions/coord' } - }, - required: ['street_address', 'city', 'state'] - }, - coord: { - $ref: 'geographical-location.schema.json#' - } - }, - type: 'object', - properties: { - address: { $ref: '#/definitions/address' } - } -}; - -export const uischema = { - type: 'VerticalLayout', - elements: [ - { - type: 'Control', - scope: '#/properties/address/properties/street_address' - }, - { - type: 'Control', - scope: '#/properties/address/properties/city' - }, - { - type: 'Control', - scope: '#/properties/address/properties/state' - }, - { - type: 'Control', - scope: '#/properties/address/properties/coord' - } - ] -}; - -export const data = { - address: { - street_address: 'Agnes-Pockels Bogen 1', - city: 'Munich', - state: 'Bavaria' - } -}; - -registerExamples([ - { - name: 'resolve', - label: 'Resolve Remote Schema', - data, - schema, - uischema - } -]); diff --git a/packages/examples/src/index.ts b/packages/examples/src/index.ts index fc173b5163..320444a1d5 100644 --- a/packages/examples/src/index.ts +++ b/packages/examples/src/index.ts @@ -51,7 +51,6 @@ import * as layout from './examples/layout'; import * as person from './examples/person'; import * as rule from './examples/rule'; import * as ruleInheritance from './examples/ruleInheritance'; -import * as resolve from './examples/resolve'; import * as config from './examples/config'; import * as text from './examples/text'; import * as numbers from './examples/numbers'; @@ -112,7 +111,6 @@ export { ruleInheritance, dates, dynamic, - resolve, config, text, numbers, diff --git a/packages/material/src/complex/MaterialAllOfRenderer.tsx b/packages/material/src/complex/MaterialAllOfRenderer.tsx index ea833ea926..8c4c79bbf7 100644 --- a/packages/material/src/complex/MaterialAllOfRenderer.tsx +++ b/packages/material/src/complex/MaterialAllOfRenderer.tsx @@ -47,9 +47,9 @@ export const MaterialAllOfRenderer = ({ uischemas, uischema }: StatePropsOfCombinator) => { - const _schema = resolveSubSchemas(schema, rootSchema, 'allOf'); + const resolvedSchema = resolveSubSchemas(schema, rootSchema, 'allOf'); const delegateUISchema = findMatchingUISchema(uischemas)( - _schema, + resolvedSchema, uischema.scope, path ); @@ -57,7 +57,7 @@ export const MaterialAllOfRenderer = ({ return ( diff --git a/packages/material/src/complex/MaterialOneOfRenderer.tsx b/packages/material/src/complex/MaterialOneOfRenderer.tsx index 497d083e94..f8158dca10 100644 --- a/packages/material/src/complex/MaterialOneOfRenderer.tsx +++ b/packages/material/src/complex/MaterialOneOfRenderer.tsx @@ -66,9 +66,9 @@ export const MaterialOneOfRenderer = const cancel = useCallback(() => { setOpen(false); }, [setOpen]); - const _schema = resolveSubSchemas(schema, rootSchema, 'oneOf'); + const resolvedSchema = resolveSubSchemas(schema, rootSchema, 'oneOf'); const oneOfRenderInfos = createCombinatorRenderInfos( - (_schema as JsonSchema).oneOf, + (resolvedSchema as JsonSchema).oneOf, rootSchema, 'oneOf', uischema, @@ -79,7 +79,7 @@ export const MaterialOneOfRenderer = const openNewTab = (newIndex: number) => { handleChange( path, - createDefaultValue(schema.oneOf[newIndex]) + createDefaultValue(resolvedSchema.oneOf[newIndex]) ); setSelectedIndex(newIndex); } @@ -101,7 +101,7 @@ export const MaterialOneOfRenderer = return ( diff --git a/packages/material/test/renderers/MaterialOneOfRenderer.test.tsx b/packages/material/test/renderers/MaterialOneOfRenderer.test.tsx index e7b4b7facf..f9a20279ac 100644 --- a/packages/material/test/renderers/MaterialOneOfRenderer.test.tsx +++ b/packages/material/test/renderers/MaterialOneOfRenderer.test.tsx @@ -309,7 +309,7 @@ describe('Material oneOf renderer', () => { }, 1000); }); - it.skip('should add an item within an array', async () => { + it('should add an item within an array', async () => { const schema = { type: 'object', properties: { @@ -366,7 +366,7 @@ describe('Material oneOf renderer', () => { expect(nrOfRowsAfterAdd.length).toBe(3); }); - it.skip('should add an object within an array', async () => { + it('should add an object within an array', async () => { const schema = { type: 'object', properties: { @@ -424,7 +424,6 @@ describe('Material oneOf renderer', () => { await waitForAsync(); - // expect(wrapper.state()) wrapper.update(); selectOneOfTab(wrapper, 1, false); @@ -441,7 +440,7 @@ describe('Material oneOf renderer', () => { }); }); - it.skip('should switch to array based oneOf subschema, then switch back, then edit', async () => { + it('should switch to array based oneOf subschema, then switch back, then edit', async (done) => { const schema = { type: 'object', properties: { @@ -511,9 +510,15 @@ describe('Material oneOf renderer', () => { const input = wrapper.find('input').first(); input.simulate('change', { target: { value: 'test' } }); wrapper.update(); - expect(onChangeData.data).toEqual({ - thingOrThings: { thing: 'test' } - }); + + setTimeout(() => { + expect(onChangeData.data).toEqual({ + thingOrThings: { thing: 'test' } + }); + done(); + }, 1000); + + }); it('should show confirm dialog when data is not an empty object', async () => { diff --git a/packages/react/test/renderers/JsonForms.test.tsx b/packages/react/test/renderers/JsonForms.test.tsx index 46b50783ed..f2c460dd27 100644 --- a/packages/react/test/renderers/JsonForms.test.tsx +++ b/packages/react/test/renderers/JsonForms.test.tsx @@ -310,7 +310,7 @@ test('render schema with $ref', () => { wrapper.unmount(); }); -test.skip('updates schema with ref', () => { +test('updates schema with ref', () => { const schemaWithRef = { definitions: { n: { @@ -338,10 +338,8 @@ test.skip('updates schema with ref', () => { } }; - const tester1 = (_uischema: UISchemaElement, s: JsonSchema) => - s.properties.foo.type === 'string' ? 1 : -1; - const tester2 = (_uischema: UISchemaElement, s: JsonSchema) => - s.properties.foo.type === 'number' ? 1 : -1; + const tester1 = rankWith(1, schemaMatches(schema => schema.type === 'string')) + const tester2 = rankWith(1, schemaMatches(schema => schema.type === 'number')) const renderers = [ { @@ -354,7 +352,7 @@ test.skip('updates schema with ref', () => { } ]; - const wrapper = shallow( + const wrapper = mount( { wrapper.setProps({ schema: schemaWithRef }); wrapper.update(); - expect(wrapper.state()).toHaveProperty('resolving', false); expect(wrapper.find(CustomRenderer2).length).toBe(1); wrapper.unmount(); });