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

Add support for 'then/else' paths #1909

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/core/src/util/path.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@ export { compose as composePaths };
*/
export const toDataPathSegments = (schemaPath: string): string[] => {
const s = schemaPath
.replace(/anyOf\/[\d]\//g, '')
.replace(/allOf\/[\d]\//g, '')
.replace(/oneOf\/[\d]\//g, '');
.replace(/anyOf\/[\d]\/((then|else)\/)?/g, '')
.replace(/allOf\/[\d]\/((then|else)\/)?/g, '')
.replace(/oneOf\/[\d]\/((then|else)\/)?/g, '');
Copy link
Member

Choose a reason for hiding this comment

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

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, '');

const segments = s.split('/');

const decodedSegments = segments.map(decode);
Expand Down
20 changes: 18 additions & 2 deletions packages/core/src/util/resolvers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,13 +134,29 @@ export const resolveSchema = (
const schemas = [].concat(
resultSchema?.oneOf ?? [],
resultSchema?.allOf ?? [],
resultSchema?.anyOf ?? []
resultSchema?.anyOf ?? [],
// also add root level schema composition entries
schema?.oneOf ?? [],
schema?.allOf ?? [],
schema?.anyOf ?? []
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the use cases where this is necessary?

Copy link
Contributor Author

@nmaier95 nmaier95 May 5, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

);
for (let item of schemas) {
for (const item of schemas) {
curSchema = resolveSchema(item, validPathSegments.slice(i).map(encode).join('/'));
if (curSchema) {
break;
}
if (!curSchema) {
Copy link
Member

Choose a reason for hiding this comment

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

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

const conditionalSchemas = [].concat(
item.then ?? [],
item.else ?? [],
);
for (const condiItem of conditionalSchemas) {
curSchema = resolveSchema(condiItem, schemaPath);
Copy link
Member

Choose a reason for hiding this comment

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

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

if (curSchema) {
break;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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

}
}
if (curSchema) {
// already resolved rest of the path
Expand Down
27 changes: 27 additions & 0 deletions packages/core/test/util/path.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,39 @@ test('toDataPath ', t => {
test('toDataPath replace anyOf', t => {
t.is(toDataPath('/anyOf/1/properties/foo/anyOf/1/properties/bar'), 'foo.bar');
});
test('toDataPath replace anyOf in combination with conditional schema compositions', t => {
t.is(toDataPath('/anyOf/1/then/properties/foo'), 'foo');
});
test('toDataPath replace multiple directly nested anyOf in combination with conditional schema compositions', t => {
t.is(toDataPath('/anyOf/1/then/anyOf/0/then/properties/foo'), 'foo');
});
test('toDataPath replace multiple nested properties with anyOf in combination with conditional schema compositions', t => {
t.is(toDataPath('/anyOf/1/properties/foo/anyOf/0/then/properties/bar'), 'foo.bar');
});
test('toDataPath replace allOf', t => {
t.is(toDataPath('/allOf/1/properties/foo/allOf/1/properties/bar'), 'foo.bar');
});
test('toDataPath replace allOf in combination with conditional schema compositions', t => {
t.is(toDataPath('/allOf/1/then/properties/foo'), 'foo');
});
test('toDataPath replace multiple directly nested allOf in combination with conditional schema compositions', t => {
t.is(toDataPath('/allOf/1/then/allOf/0/then/properties/foo'), 'foo');
});
test('toDataPath replace multiple nested properties with allOf in combination with conditional schema compositions', t => {
t.is(toDataPath('/allOf/1/properties/foo/allOf/0/then/properties/bar'), 'foo.bar');
});
test('toDataPath replace oneOf', t => {
t.is(toDataPath('/oneOf/1/properties/foo/oneOf/1/properties/bar'), 'foo.bar');
});
test('toDataPath replace oneOf in combination with conditional schema compositions', t => {
t.is(toDataPath('/oneOf/1/then/properties/foo'), 'foo');
});
test('toDataPath replace multiple directly nested oneOf in combination with conditional schema compositions', t => {
t.is(toDataPath('/oneOf/1/then/oneOf/0/then/properties/foo'), 'foo');
});
test('toDataPath replace multiple nested properties with oneOf in combination with conditional schema compositions', t => {
t.is(toDataPath('/oneOf/1/properties/foo/oneOf/0/then/properties/bar'), 'foo.bar');
});
test('toDataPath replace all combinators', t => {
t.is(
toDataPath(
Expand Down
45 changes: 44 additions & 1 deletion packages/core/test/util/resolvers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,58 @@ test('resolveSchema - resolves schema with any ', t => {
}
}]
}
}
},
anyOf: [
{
if: {
properties: {
exist: {
const: true
}
}
},
then: {
properties: {
lastname: {
type: 'string'
}
}
},
else: {
properties: {
firstname: {
type: 'string'
},
address: {
type: 'object',
anyOf: [
{
properties: {
street: {
type: 'string'
}
}
}
]
}
}
}
}
]
};
// 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, '#/anyOf/0/then/properties/lastname'), {type: 'string'});
t.deepEqual(resolveSchema(schema, '#/anyOf/0/else/properties/firstname'), {type: 'string'});
t.deepEqual(resolveSchema(schema, '#/anyOf/0/else/properties/address/anyOf/0/properties/street'), {type: 'string'});
// 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.deepEqual(resolveSchema(schema, '#/properties/lastname'), {type: 'string'});
t.deepEqual(resolveSchema(schema, '#/properties/firstname'), {type: 'string'});
t.deepEqual(resolveSchema(schema, '#/properties/address/properties/street'), {type: 'string'});
t.is(resolveSchema(schema, '#/properties/description/properties/notfound'), undefined);
});

Expand Down
118 changes: 118 additions & 0 deletions packages/examples/src/examples/conditional-schema-compositions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/*
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 = {
type: 'object',
properties: {
name: {
type: 'string',
minLength: 1,
description: 'The task\'s name',
},
recurrence: {
type: 'string',
enum: ['Never', 'Daily', 'Weekly', 'Monthly'],
},
},
anyOf: [
{
if: {
properties: {
recurrence: {
const: 'Never'
}
}
},
then: {
properties: {
lastname: {
type: 'string'
},
age: {
type: 'number'
}
}
}
},
]
};

export const uischema = {
type: 'HorizontalLayout',
elements: [
{
type: 'VerticalLayout',
elements: [
{
type: 'Control',
scope: '#/properties/name',
},
{
type: 'Control',
scope: '#/properties/recurrence',
},
{
type: 'Control',
scope: '#/anyOf/0/then/properties/lastname',
rule: {
effect: 'SHOW',
condition: {
scope: '#/properties/recurrence',
schema: {
const: 'Never'
}
}
}
},
{
type: 'Control',
scope: '#/properties/age',
rule: {
effect: 'SHOW',
condition: {
scope: '#/properties/recurrence',
schema: {
const: 'Never'
}
}
}
},
],
},
],
};

const data = {};

registerExamples([
{
name: 'conditional-schema-compositions',
label: 'Conditional Schema Compositions',
data,
schema,
uischema
}
]);
4 changes: 3 additions & 1 deletion packages/examples/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ import * as enumInArray from './examples/enumInArray';
import * as readonly from './examples/readonly';
import * as bug_1779 from './examples/1779';
import * as bug_1645 from './examples/1645';
import * as conditionalSchemaComposition from './examples/conditional-schema-compositions';
export * from './register';
export * from './example';

Expand Down Expand Up @@ -136,5 +137,6 @@ export {
enumInArray,
readonly,
bug_1779,
bug_1645
bug_1645,
conditionalSchemaComposition
};