Add support for 'then/else' paths #1909

nmaier95 · 2022-03-31T09:37:16Z

If properties of a schema were added conditionally inside
an anyof|oneof|allof list their paths were not resolved
correctly.
For reference see second example in:
https://json-schema.org/understanding-json-schema/reference/conditionals.html#if-then-else

resolves #1907

If properties of a schema were added conditionally inside an anyof|oneof|allof list their paths were not resolved correctly. For reference see second example in: https://json-schema.org/understanding-json-schema/reference/conditionals.html#if-then-else

sdirix · 2022-04-06T10:35:57Z

Thanks for the PR. We'll have a look soon!

coveralls · 2022-04-06T10:45:49Z

Coverage remained the same at 84.29% when pulling 8dd4852 on nmaier95:feature/support-conditional-schemas-inside-anyof-oneof-allof into 1410b58 on eclipsesource:master.

sdirix

Hi @nmaier95!

Thanks a lot for the PR, it makes a lot of sense to me!

Note that we also support skipping oneOf/allOf/anyOf within the UI Schema scopes, see here. Would you be interested in something similar for the if/then/else parts? If yes, would you be willing to also contribute this?

sdirix · 2022-04-07T14:49:01Z

Can you also add an example to the examples so this use case can be immediately tried?

nmaier95 · 2022-04-08T10:11:44Z

Hi @sdirix,

thank you for your feedback! I'll add an example.

Furthermore I'll have a look at the support of skipping the if/then/else part inside the scope of an ui-schema element.

CLAassistant · 2022-04-17T13:58:01Z

All committers have signed the CLA.

sdirix

In general this looks very good and I would like to merge this soon. However there is one detail I would like to have explained.

sdirix · 2022-05-05T10:33:56Z

packages/core/src/util/resolvers.ts

+        schema?.oneOf ?? [],
+        schema?.allOf ?? [],
+        schema?.anyOf ?? []


Can you explain the use cases where this is necessary?

I'll try to explain this in english. I hope it is comprehensible.

The resultSchema variable already holds the first resolved path entry. Which mostyl is properties.
If we take for example, the example given in the corresponding test-file resolvers.test.ts there I defined an anyOf in the root of the schema. The problem was that this code would now search further inside properties for the scopes defined inside anyOf - where it would not find those. That is why we need to also add the root level compositions here.

It feels weird to me that we single out the case that the anyOf/allOf/oneOf is attached to the start where we want to resolve from. The problem you mention can happen at any point of the resolving, so we only solve this for the one special case here.

…onditional-schemas-inside-anyof-oneof-allof

sdirix

Looking at the code in more detail I think we could do better by generalizing the code a bit. Also the resolving could easier handle all cases by transforming it to a depth-first search.

sdirix · 2022-05-11T13:03:42Z

packages/core/src/util/path.ts

+    .replace(/anyOf\/[\d]\/((then|else)\/)?/g, '')
+    .replace(/allOf\/[\d]\/((then|else)\/)?/g, '')
+    .replace(/oneOf\/[\d]\/((then|else)\/)?/g, '');


Shouldn't this just be another line? e.g. .replace(/(then|else)\//g, '')?
I don't see why we should restrict ourselves purely to the case that the then or else is within a combinator. We also want to remove them when they are outside of them.

Actually we could then just simplify to this, right?

Suggested change

.replace(/anyOf\/[\d]\/((then|else)\/)?/g, '')

.replace(/allOf\/[\d]\/((then|else)\/)?/g, '')

.replace(/oneOf\/[\d]\/((then|else)\/)?/g, '');

.replace(/(anyOf|allOf|oneOf)\/[\d]\//g, '')

.replace(/(then|else)\//g, '');

sdirix · 2022-05-11T14:31:22Z

packages/core/src/util/resolvers.ts

+        schema?.oneOf ?? [],
+        schema?.allOf ?? [],
+        schema?.anyOf ?? []


It feels weird to me that we single out the case that the anyOf/allOf/oneOf is attached to the start where we want to resolve from. The problem you mention can happen at any point of the resolving, so we only solve this for the one special case here.

sdirix · 2022-05-11T14:32:00Z

packages/core/src/util/resolvers.ts

        curSchema = resolveSchema(item, validPathSegments.slice(i).map(encode).join('/'));
        if (curSchema) {
          break;
        }
+        if (!curSchema) {


This condition must be true, otherwise we would not be executed. see the break above

sdirix · 2022-05-11T14:32:49Z

packages/core/src/util/resolvers.ts

+          const conditionalSchemas = [].concat(
+              item.then ?? [],
+              item.else ?? [],
+          );
+          for (const condiItem of conditionalSchemas) {
+            curSchema = resolveSchema(condiItem, schemaPath);
+            if (curSchema) {
+              break;
+            }
+          }


This only works if the then/else is directly below a combinator. If they are more nested, this will not resolve.

sdirix · 2022-05-11T14:33:38Z

packages/core/src/util/resolvers.ts

+              item.else ?? [],
+          );
+          for (const condiItem of conditionalSchemas) {
+            curSchema = resolveSchema(condiItem, schemaPath);


By handing over the whole schemaPath instead of the current segment list this will only ever be correct for the special root case.

sdirix · 2022-05-11T15:18:35Z

@nmaier95 I have a fix for my requests ready. Please allow edits by maintainers so I can add them to this PR.

Otherwise I'll have to close this one and reopen a new PR.

nmaier95 · 2022-05-11T15:33:53Z

@nmaier95 I have a fix for my requests ready. Please allow edits by maintainers so I can add them to this PR.

Otherwise I'll have to close this one and reopen a new PR.

It should be allowed now.

AlexandraBuzila

Thank you for the changes, they look good to me.

There's only one small comment in one of the tests that should be addressed.

AlexandraBuzila · 2022-05-12T08:04:45Z

packages/core/test/util/resolvers.test.ts

@@ -25,7 +25,7 @@
 import { resolveSchema } from '../../src/util/resolvers';
 import test from 'ava';

-test('resolveSchema - resolves schema with any ', t => {
+test.only('resolveSchema - resolves schema with any ', t => {


only should be removed from here 😄

AlexandraBuzila

Thanks, changes look good now!

sdirix reviewed Apr 7, 2022

View reviewed changes

nmaier95 added 2 commits April 17, 2022 15:28

Add support to ommit oneOf|allOf|anyOf + then/else inside scope

6706e50

Add example

33eb959

Remove obsolete test data

7ce3e05

nmaier95 requested a review from sdirix April 20, 2022 11:47

sdirix requested a review from AlexandraBuzila April 27, 2022 11:09

sdirix reviewed May 5, 2022

View reviewed changes

sdirix added 2 commits May 11, 2022 16:24

Merge remote-tracking branch 'upstream/master' into feature/support-c…

eda008a

…onditional-schemas-inside-anyof-oneof-allof

Rewrite resolver

7d543eb

sdirix requested changes May 11, 2022

View reviewed changes

Add sanity check in resolveSchema

85a8fbc

AlexandraBuzila requested changes May 12, 2022

View reviewed changes

Remove testing artifact

8dd4852

AlexandraBuzila approved these changes May 12, 2022

View reviewed changes

sdirix approved these changes May 12, 2022

View reviewed changes

sdirix merged commit 11de281 into eclipsesource:master May 12, 2022

sdirix changed the title ~~Add support for conditional schema compositions~~ Add support for 'then/else' paths May 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for 'then/else' paths #1909

Add support for 'then/else' paths #1909

nmaier95 commented Mar 31, 2022

sdirix commented Apr 6, 2022

coveralls commented Apr 6, 2022 •

edited

Loading

sdirix left a comment

sdirix commented Apr 7, 2022

nmaier95 commented Apr 8, 2022

CLAassistant commented Apr 17, 2022 •

edited

Loading

sdirix left a comment

sdirix May 5, 2022

nmaier95 May 5, 2022 •

edited

Loading

sdirix May 11, 2022

sdirix left a comment

sdirix May 11, 2022

sdirix May 11, 2022

sdirix May 11, 2022

sdirix May 11, 2022

sdirix May 11, 2022

sdirix commented May 11, 2022

nmaier95 commented May 11, 2022

AlexandraBuzila left a comment

AlexandraBuzila May 12, 2022

sdirix May 12, 2022

AlexandraBuzila left a comment

Add support for 'then/else' paths #1909

Add support for 'then/else' paths #1909

Conversation

nmaier95 commented Mar 31, 2022

sdirix commented Apr 6, 2022

coveralls commented Apr 6, 2022 • edited Loading

sdirix left a comment

Choose a reason for hiding this comment

sdirix commented Apr 7, 2022

nmaier95 commented Apr 8, 2022

CLAassistant commented Apr 17, 2022 • edited Loading

sdirix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmaier95 May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdirix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdirix commented May 11, 2022

nmaier95 commented May 11, 2022

AlexandraBuzila left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandraBuzila left a comment

Choose a reason for hiding this comment

coveralls commented Apr 6, 2022 •

edited

Loading

CLAassistant commented Apr 17, 2022 •

edited

Loading

nmaier95 May 5, 2022 •

edited

Loading