Fix conditionally required property not applying #2228

gatanasov-asteasolutions · 2023-12-13T12:15:20Z

This PR fixes the long lasting problem of having conditionally required properties in the schema that do not show as required in the UI. It addresses issue #1390.

Fix the isRequired function's required check when having conditional subschemas - whether it is a single if-the-else or multiple if-then-else inside allOf combinator.

Make the if condition work with pattern and fix the check to work with nested properties

…equired Fix conditional required not applying

netlify · 2023-12-13T12:15:26Z

✅ Deploy Preview for jsonforms-examples ready!

Name	Link
🔨 Latest commit	`ec4273a`
🔍 Latest deploy log	https://app.netlify.com/sites/jsonforms-examples/deploys/659e7f31b463c60009a9121c
😎 Deploy Preview	https://deploy-preview-2228--jsonforms-examples.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

CLAassistant · 2023-12-13T12:15:26Z

All committers have signed the CLA.

coveralls · 2023-12-13T16:02:51Z

coverage: 85.591% (+2.5%) from 83.078%
when pulling ec4273a on gatanasov-asteasolutions:master
into b00754c on eclipsesource:master.

sdirix · 2023-12-14T13:43:23Z

Thank you very much for the contribution ❤️. We will soon take a look.

sdirix

Thanks for this interesting contribution!!

Technically speaking, what this PR is missing is a set of unit tests, testing the new functionality in detail.

Conceptionally this is breaking new grounds for JSON Forms as this introduces dynamic evaluation mechanism which we do not do yet. This is potentially expensive and will be executed with every render call. I'm wondering whether we should

make this behavior optional (default: false), and/or
introduce a generic prop-adaption mechanism. Then this "advanced" required analysis could be manually configured by the user if they need it.
- Of course then we could already provide this adapter for them.

Once we introduce this, there will be a lot of follow up requests. For example it's a bit arbitrary that we only do this for required and not for other attributes too, e.g. title. So I'm leaning a bit to the generic prop-adaption mechanism which would allow for arbitrary behavior while keeping JSON Forms itself slim.

What do you think?

sdirix · 2024-01-03T15:36:06Z

packages/core/src/util/renderer.ts

+ 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])
+ );


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.

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.

sdirix · 2024-01-03T15:43:22Z

packages/core/src/util/renderer.ts

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


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

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.

sdirix · 2024-01-03T15:56:45Z

packages/core/src/util/renderer.ts

+import { has } from 'lodash';
+import { all, any } from 'lodash/fp';


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

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

gatanasov-asteasolutions · 2024-01-08T09:21:43Z

I'm wondering whether we should

make this behavior optional (default: false), and/or

introduce a generic prop-adaption mechanism. Then this "advanced" required analysis could be manually configured by the user if they need it.

Of course then we could already provide this adapter for them.

Once we introduce this, there will be a lot of follow up requests. For example it's a bit arbitrary that we only do this for required and not for other attributes too, e.g. title. So I'm leaning a bit to the generic prop-adaption mechanism which would allow for arbitrary behavior while keeping JSON Forms itself slim.

What do you think?

The most appropriate place for me to add this toggle of these dynamic checks is the config property of the JsonForms component. It is an easy way to provide the users with the option to enable them only on specific schemas, just like they can use it to disable the asterisk. Also they can use the same property to enable future dynamic checks for other fields, if there appear to be any. I have added this functionality in my latest commit.

…ditionally-required Add tests for conditionally required logic. Also make the whole dynamic check optional based on the `allowDynamicChecks` property of the `config` parameter of the `JsonForms` component.

sdirix · 2024-01-24T12:56:16Z

Hi @gatanasov-asteasolutions, we were currently busy with the 3.2 release. We will discuss this proposal and what we think what the best way forward is within the team in the upcoming weeks and let you know.

In the mean time:

If you would like to contribute the "prop-adaption" mechanism to accomplish this feature for your renderers in a convenient way, then feel free to do so
If you would like to already consume your contributed mechanism, then you can do so via custom renderers. In there you can call the current bindings of JSON Forms and then just apply the "conditionally required property" evaluation on top.

gatanasov-asteasolutions and others added 9 commits December 5, 2023 16:11

Add new example for conditional required

9746c81

Fix required not showing with conditional subschemas

67ab199

Fix the isRequired function's required check when having conditional subschemas - whether it is a single if-the-else or multiple if-then-else inside allOf combinator.

Add pattern support and nested checks

9afddab

Make the if condition work with pattern and fix the check to work with nested properties

Add support for nested allOf and if-then-else

2cf5824

Address comments about nested structures

4455ed2

Fix wrong calling of condition

ea425a6

Add check for required in parent schema

1e18a5e

Merge pull request #1 from gatanasov-asteasolutions/fix_conditional_r…

2fcb0f7

…equired Fix conditional required not applying

Merge branch 'eclipsesource:master' into master

9119c78

gatanasov-asteasolutions changed the title ~~Fix conditionally required property not applying #1390~~ Fix conditionally required property not applying Dec 13, 2023

sdirix force-pushed the master branch 2 times, most recently from 8bd14c2 to e9946ef Compare December 15, 2023 16:07

sdirix reviewed Jan 3, 2024

View reviewed changes

gatanasov-asteasolutions and others added 4 commits January 8, 2024 11:57

Merge branch 'eclipsesource:master' into master

3e466c8

Add tests for conditionally required logic

d11e2b6

Add more tests and make conditionally required check optional

0d3f614

Merge pull request #2 from gatanasov-asteasolutions/add-tests-for-con…

ec4273a

…ditionally-required Add tests for conditionally required logic. Also make the whole dynamic check optional based on the `allowDynamicChecks` property of the `config` parameter of the `JsonForms` component.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix conditionally required property not applying #2228

Fix conditionally required property not applying #2228

gatanasov-asteasolutions commented Dec 13, 2023

netlify bot commented Dec 13, 2023 •

edited

CLAassistant commented Dec 13, 2023 •

edited

coveralls commented Dec 13, 2023 •

edited

sdirix commented Dec 14, 2023

sdirix left a comment •

edited

sdirix Jan 3, 2024

gatanasov-asteasolutions Jan 10, 2024

sdirix Jan 3, 2024

gatanasov-asteasolutions Jan 8, 2024

sdirix Jan 3, 2024

gatanasov-asteasolutions Jan 10, 2024

gatanasov-asteasolutions commented Jan 8, 2024 •

edited

sdirix commented Jan 24, 2024

		import { has } from 'lodash';
		import { all, any } from 'lodash/fp';

Fix conditionally required property not applying #2228

Are you sure you want to change the base?

Fix conditionally required property not applying #2228

Conversation

gatanasov-asteasolutions commented Dec 13, 2023

netlify bot commented Dec 13, 2023 • edited

✅ Deploy Preview for jsonforms-examples ready!

CLAassistant commented Dec 13, 2023 • edited

coveralls commented Dec 13, 2023 • edited

sdirix commented Dec 14, 2023

sdirix left a comment • edited

Choose a reason for hiding this comment

sdirix Jan 3, 2024

Choose a reason for hiding this comment

gatanasov-asteasolutions Jan 10, 2024

Choose a reason for hiding this comment

sdirix Jan 3, 2024

Choose a reason for hiding this comment

gatanasov-asteasolutions Jan 8, 2024

Choose a reason for hiding this comment

sdirix Jan 3, 2024

Choose a reason for hiding this comment

gatanasov-asteasolutions Jan 10, 2024

Choose a reason for hiding this comment

gatanasov-asteasolutions commented Jan 8, 2024 • edited

sdirix commented Jan 24, 2024

netlify bot commented Dec 13, 2023 •

edited

CLAassistant commented Dec 13, 2023 •

edited

coveralls commented Dec 13, 2023 •

edited

sdirix left a comment •

edited

gatanasov-asteasolutions commented Jan 8, 2024 •

edited