Improve reference resolving #1829

AlexandraBuzila · 2021-11-04T14:14:08Z

resolve items subschema refs for renderers and testers
make rootSchema a required parameter in resolveSchema, as it is needed
while resolving
add rootSchema as an optional parameter to Testers, so we can
correclty resolve subschemas in testers
use resolved schemas in MaterialOneOfRenderer when computing default
value
enable and update tests
remove 'Resolve Remote Schema' example as it's unsupported

coveralls · 2021-11-04T14:30:52Z

Coverage remained the same at 84.29% when pulling 6a32016 on AlexandraBuzila:ref-resolving-fixes into d04e851 on eclipsesource:master.

packages/core/src/util/resolvers.ts

packages/material/src/complex/MaterialOneOfRenderer.tsx

AlexandraBuzila · 2021-12-13T15:24:49Z

Open points that should be integrated in the PR:

we are currently resolving the refs for the items property directly in the resolveSchema method (and thus modifying the schema). This should be removed from the method and directly happen in the testers and renderers.
the rootSchema property in the resolveSchema method should be mandatory
check that all examples are working

packages/core/src/util/resolvers.ts

sdirix

Already works much better than before. Please see my detailed comments for further improvements. Please also test against the schema and uischema from:

packages/core/src/testers/testers.ts

packages/core/src/util/resolvers.ts

AlexandraBuzila · 2022-03-14T13:07:26Z

Please also test against the schema and uischema from:

Unable to use recursive schema #1491

https://github.com/jamesbmorris92/jsonforms-react-seed

Both examples work. #1491 is covered by the oneOf-recursive example.

sdirix

Looks good! Added some comments and questions.

packages/core/src/testers/testers.ts

packages/core/src/util/renderer.ts

sdirix

Thanks for the update! I have some more comments and questions

packages/core/src/testers/testers.ts

packages/core/src/util/renderer.ts

packages/core/src/util/resolvers.ts

AlexandraBuzila · 2022-04-29T16:52:53Z

Thank you for for the review, Stefan! I updated the code and integrated your comments.

sdirix

Looks good ;) Found one more small detail.

sdirix · 2022-05-05T15:17:08Z

packages/core/src/testers/testers.ts

@@ -47,15 +47,17 @@ import { deriveTypes, hasType, resolveSchema } from '../util';
 export const NOT_APPLICABLE = -1;
 /**
 * A tester is a function that receives an UI schema and a JSON schema and returns a boolean.
+ * Optionally, a root JSON schema can be provided, which can be used for resolving refs in the schema.


Suggested change

* Optionally, a root JSON schema can be provided, which can be used for resolving refs in the schema.

* The rootSchema is handed over as context. Can be used to resolve references.

sdirix · 2022-05-05T15:24:16Z

packages/core/src/testers/testers.ts

+  const resolvedSchema = resolveSchema(schema, schemaPath, rootSchema ?? schema);
+  if (resolvedSchema?.items && (resolvedSchema.items as any).$ref){
+    resolvedSchema.items = resolveSchema(resolvedSchema, 'items', rootSchema ?? schema);
+  }


Here we are modifying the returned schema and therefore the overall schema stored in the JSON Forms store. Instead of building up a resolved schema we could adapt the checking logic to being able to follow refs.

- resolve items subschema refs for renderers and testers - make rootSchema a required parameter in resolveSchema, as it is needed while resolving - add rootSchema as an optional parameter to Testers and predicates used by Testers, so we can correclty resolve subschemas - use resolved schemas in MaterialOneOfRenderer when computing default value - enable and update tests - remove 'Resolve Remote Schema' example as it's unsupported

- make rootSchema parameter required in testers - update tests - use rootSchema where applicable when resolving

sdirix

Great work! That's a great improvement to JSON Forms!

AlexandraBuzila requested a review from sdirix November 4, 2021 14:14

TheZoker mentioned this pull request Nov 4, 2021

Remove redux from example package #1828

Merged

sdirix reviewed Nov 9, 2021

View reviewed changes

packages/core/src/util/resolvers.ts Outdated Show resolved Hide resolved

packages/material/src/complex/MaterialOneOfRenderer.tsx Outdated Show resolved Hide resolved

AlexandraBuzila force-pushed the ref-resolving-fixes branch from c280793 to d9e9004 Compare December 9, 2021 12:10

AlexandraBuzila commented Dec 13, 2021

View reviewed changes

packages/core/src/util/resolvers.ts Outdated Show resolved Hide resolved

AlexandraBuzila force-pushed the ref-resolving-fixes branch from d9e9004 to 4571c0f Compare February 8, 2022 16:47

sdirix requested changes Feb 9, 2022

View reviewed changes

packages/core/src/testers/testers.ts Outdated Show resolved Hide resolved

packages/core/src/util/resolvers.ts Outdated Show resolved Hide resolved

AlexandraBuzila force-pushed the ref-resolving-fixes branch from 4571c0f to 3d07957 Compare March 14, 2022 13:02

AlexandraBuzila requested a review from sdirix March 14, 2022 13:09

sdirix reviewed Apr 6, 2022

View reviewed changes

packages/core/src/testers/testers.ts Outdated Show resolved Hide resolved

packages/core/src/testers/testers.ts Outdated Show resolved Hide resolved

packages/core/src/util/renderer.ts Show resolved Hide resolved

AlexandraBuzila force-pushed the ref-resolving-fixes branch from 3d07957 to dada0c0 Compare April 13, 2022 14:03

sdirix reviewed Apr 27, 2022

View reviewed changes

AlexandraBuzila force-pushed the ref-resolving-fixes branch from 0f5846a to 735ce93 Compare April 29, 2022 16:51

sdirix requested changes May 6, 2022

View reviewed changes

AlexandraBuzila and others added 4 commits May 6, 2022 14:33

Fix ref resolving for oneOf and array schemas

c154e4a

- make rootSchema parameter required in testers - update tests - use rootSchema where applicable when resolving

Don't modify schema in testers

8214203

Fix Angular Material tests

9f2c9f7

sdirix force-pushed the ref-resolving-fixes branch from 82d0d1e to 9f2c9f7 Compare May 6, 2022 15:14

sdirix approved these changes May 6, 2022

View reviewed changes

sdirix added 3 commits May 6, 2022 18:02

Add migration text for the testers interface

e3535bf

futher migration changes

fa5c165

more changes

6a32016

sdirix merged commit c5e6703 into eclipsesource:master May 6, 2022

sdirix changed the title ~~Fix ref resolving for oneOf schemas~~ Improve reference resolving May 6, 2022

sdirix linked an issue May 6, 2022 that may be closed by this pull request

Remove external ref resolving #1750

Closed

This was referenced May 6, 2022

Remove external ref resolving #1750

Closed

undefined rootSchema in resolveSchema() causes: Cannot read properties of undefined (reading 'properties') #1855

Closed

golinski mentioned this pull request Aug 5, 2022

indexOfFittingSchema for oneOf doesn't work anymore if $ref is used #1991

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve reference resolving #1829

Improve reference resolving #1829

AlexandraBuzila commented Nov 4, 2021 •

edited

Loading

coveralls commented Nov 4, 2021 •

edited

Loading

AlexandraBuzila commented Dec 13, 2021

sdirix left a comment

AlexandraBuzila commented Mar 14, 2022

sdirix left a comment

sdirix left a comment

AlexandraBuzila commented Apr 29, 2022

sdirix left a comment

sdirix May 5, 2022

sdirix May 5, 2022

sdirix left a comment

	* Optionally, a root JSON schema can be provided, which can be used for resolving refs in the schema.
	* The rootSchema is handed over as context. Can be used to resolve references.

Improve reference resolving #1829

Improve reference resolving #1829

Conversation

AlexandraBuzila commented Nov 4, 2021 • edited Loading

coveralls commented Nov 4, 2021 • edited Loading

AlexandraBuzila commented Dec 13, 2021

sdirix left a comment

Choose a reason for hiding this comment

AlexandraBuzila commented Mar 14, 2022

sdirix left a comment

Choose a reason for hiding this comment

sdirix left a comment

Choose a reason for hiding this comment

AlexandraBuzila commented Apr 29, 2022

sdirix left a comment

Choose a reason for hiding this comment

sdirix May 5, 2022

Choose a reason for hiding this comment

sdirix May 5, 2022

Choose a reason for hiding this comment

sdirix left a comment

Choose a reason for hiding this comment

AlexandraBuzila commented Nov 4, 2021 •

edited

Loading

coveralls commented Nov 4, 2021 •

edited

Loading