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 conditionally required property not applying #2228

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
9746c81
Add new example for conditional required
gatanasov-asteasolutions Dec 5, 2023
67ab199
Fix required not showing with conditional subschemas
gatanasov-asteasolutions Dec 5, 2023
9afddab
Add pattern support and nested checks
gatanasov-asteasolutions Dec 6, 2023
2cf5824
Add support for nested allOf and if-then-else
gatanasov-asteasolutions Dec 6, 2023
4455ed2
Address comments about nested structures
gatanasov-asteasolutions Dec 11, 2023
ea425a6
Fix wrong calling of condition
gatanasov-asteasolutions Dec 11, 2023
1e18a5e
Add check for required in parent schema
gatanasov-asteasolutions Dec 12, 2023
2fcb0f7
Merge pull request #1 from gatanasov-asteasolutions/fix_conditional_r…
gatanasov-asteasolutions Dec 13, 2023
9119c78
Merge branch 'eclipsesource:master' into master
gatanasov-asteasolutions Dec 13, 2023
3e466c8
Merge branch 'eclipsesource:master' into master
gatanasov-asteasolutions Jan 8, 2024
d11e2b6
Add tests for conditionally required logic
gatanasov-asteasolutions Jan 9, 2024
0d3f614
Add more tests and make conditionally required check optional
gatanasov-asteasolutions Jan 10, 2024
ec4273a
Merge pull request #2 from gatanasov-asteasolutions/add-tests-for-con…
gatanasov-asteasolutions Jan 10, 2024
a94ef35
Modify conditionallyRequired logic to handle arrays
KManolov3 Feb 15, 2024
0cc6dda
Remove redundant comments
KManolov3 Feb 15, 2024
ad4614a
Add additional check to prevent accessing undefined data
KManolov3 Feb 19, 2024
a6f0d7b
Remove comments from renderer.ts
gatanasov-asteasolutions Aug 27, 2024
2b9dad3
Merge branch 'master' of https://github.com/eclipsesource/jsonforms i…
gatanasov-asteasolutions Aug 27, 2024
f963f79
Merge branch 'master' into backmerge-jsonforms-base
gatanasov-asteasolutions Aug 27, 2024
c3d333f
Merge pull request #5 from gatanasov-asteasolutions/backmerge-jsonfor…
gatanasov-asteasolutions Aug 27, 2024
e53b121
Merge branch 'master' into additional-checks-for-array-handling
gatanasov-asteasolutions Aug 27, 2024
f1f81dc
Merge pull request #3 from gatanasov-asteasolutions/additional-checks…
gatanasov-asteasolutions Aug 28, 2024
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
311 changes: 305 additions & 6 deletions packages/core/src/util/renderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ import {
ControlElement,
isLabelable,
JsonSchema,
JsonSchema4,
JsonSchema7,
LabelElement,
UISchemaElement,
} from '../models';
Expand Down Expand Up @@ -58,7 +60,7 @@ import type { CombinatorKeyword } from './combinators';
import { moveDown, moveUp } from './array';
import type { AnyAction, Dispatch } from './type';
import { Resolve, convertDateToString, hasType } from './util';
import { composePaths, composeWithUi } from './path';
import { composePaths, composeWithUi, toDataPath } from './path';
import { CoreActions, update } from '../actions';
import type { ErrorObject } from 'ajv';
import type { JsonFormsState } from '../store';
Expand All @@ -80,11 +82,284 @@ import {
} from '../i18n/arrayTranslations';
import { resolveSchema } from './resolvers';
import cloneDeep from 'lodash/cloneDeep';
import { has } from 'lodash';
import { all, any } from 'lodash/fp';
Copy link
Member

Choose a reason for hiding this comment

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

We use the modular imports of lodash, see line 83,84.

Choose a reason for hiding this comment

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

I see, I have changed it in my latest commit.


const checkDataCondition = (
propertyCondition: unknown,
property: string,
data: Record<string, unknown>
) => {
if (has(propertyCondition, 'const')) {
return (
has(data, property) && data[property] === get(propertyCondition, 'const')
);
} else if (has(propertyCondition, 'enum')) {
return (
has(data, property) &&
(get(propertyCondition, 'enum') as unknown[]).includes(data[property])
);
Copy link
Member

Choose a reason for hiding this comment

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

These will only work when values are primitives. If a value is an object these comparisons will fail. We should probably use lodash's isEqual here to cover the more complex use cases too.

Choose a reason for hiding this comment

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

You are right, there were other places as well where it was needed to compare the values with isEqual. It is fixed in my latest commit.

} else if (has(propertyCondition, 'pattern')) {
const pattern = new RegExp(get(propertyCondition, 'pattern'));

return (
has(data, property) &&
typeof data[property] === 'string' &&
pattern.test(data[property] as string)
);
}

return false;
};

const checkPropertyCondition = (
propertiesCondition: Record<string, unknown>,
property: string,
data: Record<string, unknown>
): boolean => {
if (has(propertiesCondition[property], 'not')) {
return !checkDataCondition(
get(propertiesCondition[property], 'not'),
property,
data
);
}

if (has(propertiesCondition[property], 'properties')) {
const nextPropertyConditions = get(
propertiesCondition[property],
'properties'
);

return all(
(prop) =>
checkPropertyCondition(
nextPropertyConditions,
prop,
data[property] as Record<string, unknown>
),
Object.keys(nextPropertyConditions)
);
}

return checkDataCondition(propertiesCondition[property], property, data);
};

const evaluateCondition = (
schema: JsonSchema,
data: Record<string, unknown>
): boolean => {
if (has(schema, 'allOf')) {
return all(
(subschema: JsonSchema) => evaluateCondition(subschema, data),
get(schema, 'allOf')
);
}

if (has(schema, 'anyOf')) {
return any(
(subschema: JsonSchema) => evaluateCondition(subschema, data),
get(schema, 'anyOf')
);
}

if (has(schema, 'oneOf')) {
const subschemas = get(schema, 'oneOf');

let satisfied = false;

for (let i = 0; i < subschemas.length; i++) {
if (satisfied) {
return false;
}

satisfied = evaluateCondition(subschemas[i], data);
}

return satisfied;
}

let requiredProperties: string[] = [];
if (has(schema, 'required')) {
requiredProperties = get(schema, 'required');
}

const requiredCondition = all(
(property) => data[property],
requiredProperties
);

const propertiesCondition = get(schema, 'properties') as Record<
string,
unknown
>;

const valueCondition = all(
(property) => checkPropertyCondition(propertiesCondition, property, data),
Object.keys(propertiesCondition)
);

return requiredCondition && valueCondition;
};

/**
* Go through parent's properties untill the segment is found at the exact level it is defined and check if it is required
*/
const extractRequired = (
schema: JsonSchema,
segment: string,
prevSegments: string[]
) => {
let i = 0;
let currentSchema = schema;
while (
i < prevSegments.length &&
has(currentSchema, 'properties') &&
has(get(currentSchema, 'properties'), prevSegments[i])
) {
currentSchema = get(get(currentSchema, 'properties'), prevSegments[i]);
++i;
}

if (i < prevSegments.length) {
return false;
}

return (
has(currentSchema, 'required') &&
(get(currentSchema, 'required') as string[]).includes(segment)
);
};

/**
* Check if property's required attribute is set based on if-then-else condition
*
*/
const checkRequiredInIf = (
schema: JsonSchema,
segment: string,
prevSegments: string[],
data: Record<string, unknown>
): boolean => {
const propertiesConditionSchema = get(schema, 'if');

const condition = evaluateCondition(propertiesConditionSchema, data);

const ifInThen = has(get(schema, 'then'), 'if');
const ifInElse = has(get(schema, 'else'), 'if');
const allOfInThen = has(get(schema, 'then'), 'allOf');
const allOfInElse = has(get(schema, 'else'), 'allOf');
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit too hardcoded. I would prefer if the logic could be generalized. For example anyOf and oneOf are not checked here.

Choose a reason for hiding this comment

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

anyOf and oneOf are not checked, because they are not part of the conditionally required logic. We are only interested in required in the then and else clauses of if and allOf is used as a method of having multiples if-s in the schema.


return (
(has(schema, 'then') &&
condition &&
extractRequired(get(schema, 'then'), segment, prevSegments)) ||
(has(schema, 'else') &&
!condition &&
extractRequired(get(schema, 'else'), segment, prevSegments)) ||
(ifInThen &&
condition &&
checkRequiredInIf(get(schema, 'then'), segment, prevSegments, data)) ||
(ifInElse &&
!condition &&
checkRequiredInIf(get(schema, 'else'), segment, prevSegments, data)) ||
(allOfInThen &&
condition &&
conditionallyRequired(
get(schema, 'then'),
segment,
prevSegments,
data
)) ||
(allOfInElse &&
!condition &&
conditionallyRequired(get(schema, 'else'), segment, prevSegments, data))
);
};

/**
* Check if property becomes required based on some if-then-else condition
* that is part of allOf combinator
*/
const conditionallyRequired = (
schema: JsonSchema,
segment: string,
prevSegments: string[],
data: any
) => {
const nestedAllOfSchema = get(schema, 'allOf');

return any((subschema: JsonSchema4 | JsonSchema7): boolean => {
return (
(has(subschema, 'if') &&
checkRequiredInIf(subschema, segment, prevSegments, data)) ||
conditionallyRequired(subschema, segment, prevSegments, data)
);
}, nestedAllOfSchema);
};

/**
* Check if property is being required in the parent schema
*/
const isRequiredInParent = (
schema: JsonSchema,
rootSchema: JsonSchema,
path: string,
segment: string,
prevSegments: string[],
data: Record<string, unknown>
): boolean => {
const pathSegments = path.split('/');
const lastSegment = pathSegments[pathSegments.length - 3];
const nextHigherSchemaSegments = pathSegments.slice(
0,
pathSegments.length - 4
);

if (!nextHigherSchemaSegments.length) {
return false;
}

const nextHigherSchemaPath = nextHigherSchemaSegments.join('/');

const nextHigherSchema = Resolve.schema(
schema,
nextHigherSchemaPath,
rootSchema
);

const currentData = Resolve.data(data, toDataPath(nextHigherSchemaPath));

return (
conditionallyRequired(
nextHigherSchema,
segment,
[lastSegment, ...prevSegments],
currentData
) ||
(has(nextHigherSchema, 'if') &&
checkRequiredInIf(
nextHigherSchema,
segment,
[lastSegment, ...prevSegments],
currentData
)) ||
isRequiredInParent(
schema,
rootSchema,
nextHigherSchemaPath,
segment,
[lastSegment, ...prevSegments],
currentData
)
);
};

const isRequired = (
schema: JsonSchema,
schemaPath: string,
rootSchema: JsonSchema
rootSchema: JsonSchema,
data: any
): boolean => {
const pathSegments = schemaPath.split('/');
const lastSegment = pathSegments[pathSegments.length - 1];
Expand All @@ -99,11 +374,35 @@ const isRequired = (
nextHigherSchemaPath,
rootSchema
);
const currentData = Resolve.data(data, toDataPath(nextHigherSchemaPath));

const requiredInIf =
has(nextHigherSchema, 'if') &&
checkRequiredInIf(nextHigherSchema, lastSegment, [], currentData);

const requiredConditionally = conditionallyRequired(
nextHigherSchema,
lastSegment,
[],
currentData
);

const requiredConditionallyInParent = isRequiredInParent(
rootSchema,
rootSchema,
schemaPath,
lastSegment,
[],
data
);

return (
nextHigherSchema !== undefined &&
nextHigherSchema.required !== undefined &&
nextHigherSchema.required.indexOf(lastSegment) !== -1
(nextHigherSchema !== undefined &&
nextHigherSchema.required !== undefined &&
nextHigherSchema.required.indexOf(lastSegment) !== -1) ||
requiredInIf ||
requiredConditionally ||
requiredConditionallyInParent
);
};

Expand Down Expand Up @@ -500,7 +799,7 @@ export const mapStateToControlProps = (
const rootSchema = getSchema(state);
const required =
controlElement.scope !== undefined &&
isRequired(ownProps.schema, controlElement.scope, rootSchema);
isRequired(ownProps.schema, controlElement.scope, rootSchema, rootData);
const resolvedSchema = Resolve.schema(
ownProps.schema || rootSchema,
controlElement.scope,
Expand Down
Loading