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	`f1f81dc`
🔍 Latest deploy log	https://app.netlify.com/sites/jsonforms-examples/deploys/66ced72fd6d0e00008b8c53b
😎 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

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ gatanasov-asteasolutions
❌ Kaloyan Manolov

Kaloyan Manolov seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

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

coverage: 84.255% (+1.0%) from 83.285%
when pulling f1f81dc on gatanasov-asteasolutions:master
into bd11754 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.

…nto backmerge-jsonforms-base

…ms-base Backmerge jsonforms base

…-for-array-handling Additional checks for array handling

gatanasov-asteasolutions · 2024-08-28T09:22:55Z

Hi @sdirix can you please review these changes again? The fork is updated to include the latest changes of the master branch and new changes were introduced in order to support conditionally required validations for array controls. I am also looking forward to your team's decision on the future of this proposal and if I can assist you in any way.

sdirix · 2024-09-09T10:16:53Z

Hi @gatanasov-asteasolutions,

Conceptually this feature is not a good fit for the code base as it is right now because:

The dynamic required use cases are hard coded to some basic scenarios. I guess these are the ones you need to support right now, however once they are in certainly many more requests / bugs will be opened to cover more. The upper ceiling is very high because at the end of the line we would need to replicate all of JSON Schema validation.
At the moment these "required evaluations" will run with every single rerender. Therefore this will slow the framework down for everyone, even if they are not interested in the feature.

What we would like to see is a generic extension mechanism for JSON Forms which allows to easily bring in additional functionality like this dynamic required use case.

At the moment it's already possible to integrate a middleware which allows to inject arbitrary functionality into JSON Form's state handling, allowing to implement all kinds of powerful use cases. It would be interesting to add something similar for the rendering process, e.g. to allow to provide a rendering-middleware which allows to modify all props handed over to the renderers before they are invoked. With that in place you could add generic functionality like "dynamic required analysis" without re-registering all renderers with custom bindings.

Ideally it should be introduced in a way which allows to memo the result, so it's not executed for every rerender. However conceptually this will be difficult as any data change might affect the result of the dynamic validation. So not rerunning the analysis with every data change might be difficult.

Does this make sense to you?

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.

Kaloyan Manolov and others added 7 commits February 21, 2024 09:53

Modify conditionallyRequired logic to handle arrays

a94ef35

Remove redundant comments

0cc6dda

Add additional check to prevent accessing undefined data

ad4614a

Remove comments from renderer.ts

a6f0d7b

Merge branch 'master' of https://github.com/eclipsesource/jsonforms i…

2b9dad3

…nto backmerge-jsonforms-base

Merge branch 'master' into backmerge-jsonforms-base

f963f79

Merge pull request #5 from gatanasov-asteasolutions/backmerge-jsonfor…

c3d333f

…ms-base Backmerge jsonforms base

gatanasov-asteasolutions and others added 2 commits August 27, 2024 14:15

Merge branch 'master' into additional-checks-for-array-handling

e53b121

Merge pull request #3 from gatanasov-asteasolutions/additional-checks…

f1f81dc

…-for-array-handling Additional checks for array handling

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

Loading

CLAassistant commented Dec 13, 2023 •

edited

Loading

coveralls commented Dec 13, 2023 •

edited

Loading

sdirix commented Dec 14, 2023

sdirix left a comment •

edited

Loading

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

Loading

sdirix commented Jan 24, 2024

gatanasov-asteasolutions commented Aug 28, 2024

sdirix commented Sep 9, 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 Loading

✅ Deploy Preview for jsonforms-examples ready!

CLAassistant commented Dec 13, 2023 • edited Loading

coveralls commented Dec 13, 2023 • edited Loading

sdirix commented Dec 14, 2023

sdirix left a comment • edited Loading

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 Loading

sdirix commented Jan 24, 2024

gatanasov-asteasolutions commented Aug 28, 2024

sdirix commented Sep 9, 2024

netlify bot commented Dec 13, 2023 •

edited

Loading

CLAassistant commented Dec 13, 2023 •

edited

Loading

coveralls commented Dec 13, 2023 •

edited

Loading

sdirix left a comment •

edited

Loading

gatanasov-asteasolutions commented Jan 8, 2024 •

edited

Loading