Initial support for property & schema dependencies #659

mfulton26 · 2017-08-05T13:29:17Z

Reasons for making this change

JSON Schema supports property and schema dependencies which allow changing the schema for an object (even a single object within an array of objects) based on the presence or value of certain properties.

This change brings initial support for property dependencies and schema dependencies which allows developers to define conditionally required fields, conditionally present fields, and dynamic schemas (e.g. changing a property type based on the value of another field).

See #616 and #430 (see also #522).

Checklist

This is a work in progress but I am looking for some initial feedback as I have not contributed to this project before and I am looking for early feedback on the implementation, etc.

I'm updating documentation
- I've checked the rendering of the Markdown text I've added
- If I'm adding a new section, I've updated the Table of Content
I'm adding or updating code
- I've added and/or updated tests
- I've updated docs if needed
- I've run npm run cs-format on my branch to conform my code to prettier coding style
I'm adding a new feature
- I've updated the playground with an example use of the feature

ksjogo · 2017-08-16T16:47:42Z

Using that in my project now. Works great. Happy to provide any needed help for merging this.

faassen · 2017-08-18T11:20:18Z

I've seen it argued in other issues such as #430 that instead of using schema dependencies you can dynamically change the schema when data changes. This is indeed very powerful. But I've added a note there with some use cases for dependencies.

n1k0

I really like this, thanks for the effort put in this PR. Here are some preliminary remarks before getting further into the patch.

n1k0 · 2017-08-18T11:27:46Z

playground/samples/propertyDependencies.js

+        src:
+          "https://spacetelescope.github.io/understanding-json-schema/reference/object.html#dependencies",
+        description:
+          "In the following example, whenever a `credit_card` property is provided, a `billing_address` property must also be present.",


Markdown is not supported, so backticks won't turn into <code> blocks, though you could use JSX instead of a string here.

Oh interesting, I didn't think of that. I'll take a look. Thank you.

n1k0 · 2017-08-18T11:29:53Z

src/components/fields/ArrayField.js

+        const itemSchema = retrieveSchema(
+          schema.items,
+          definitions,
+          formData[index]


Nit: here and in some other places, this could be just item.

Good point, I felt like I was "hacking" a bit as I am still familiarizing myself with the code base. Thank you for the feedback. I will update.

n1k0 · 2017-08-18T11:33:29Z

src/utils.js

+      definitions,
+      formData
+    );
+  } else if (schema.hasOwnProperty("dependencies")) {


How about extracting dependency handling logic into its own dedicated function? This whole retrieveSchema one is getting huge.

I was just thinking of this this morning when I started thinking about this pull request again due to others commenting on the pull request. I will see if I can have some updates up today or tomorrow. Thank you.

n1k0 · 2017-08-18T11:33:53Z

src/utils.js

+    // Drop the dependencies from the source schema.
+    let { dependencies, ...localSchema } = schema;
+    // Process dependencies updating the local schema properties as appropriate.
+    for (const key of Object.keys(dependencies)) {


Nit: why not just for (const key in dependencies)?

Because I'm an EcmaScript noob? 😄

n1k0 · 2017-08-18T11:39:49Z

src/utils.js

+            true
+          )
+        );
+        if (Array.isArray(value.oneOf)) {


Hmm so this introduces partial support for oneOf while fields are still not handled at all, I'd have preferred introducing this after we get proper support for oneOf.

Yeah, I'm still not certain how oneOf would work in general but I kind of view the usage here as special. The idea is that depending on values in formData the overall schema will dynamically update to one of several options but this still doesn't support a user selecting one of several schemas to interact with as one would need if using oneOf elsewhere in a schema. I have started updating README.md locally and I am planning on explaining just this as implementing support for this kind of oneOf is very independent of general support for oneOf (and vice versa I believe although I could be wrong).

Right, we may just want to document this specific behavior here.

n1k0 · 2017-08-18T11:40:31Z

src/utils.js

+              };
+              const { errors } = jsonValidate(formData, conditionSchema);
+              if (errors.length === 0) {
+                const dependentSchema = { ...subschema };


Beware as cloning this way isn't recursive.

Correct. I am just trying to not mutate the incoming schema at all so I make copies as needed in the tree instead of recursively coping the entire tree (although I suppose the later could be done for readability/maintainability reasons although I'm not sure how to do that off the top of my head).

Deep copying objects is hard, Json serial/deserialization is known to be quite efficient but maybe not enough here as this operation may be executed on each keystroke in a field with this lib.

n1k0 · 2017-08-18T11:45:27Z

src/utils.js

+      // Skip this dependency if its trigger property is not present.
+      if (!formData[key]) {
+        // fixme: a falsey check may not be appropriate here, I'm not sure
+        continue;


Yeah, maybe just check for undefined I think.

Hmm, I'll try that but I may already have done so. If a field is left empty does it come in as undefined in the formData or as an empty string ""? I guess I will find out. :-)

AFAICR it depends if the field value has been edited already or not; by default a schema string field value is set to undefined (property is not preset), but as soon as users manipulate the widget, eg. typing then erasing the entered value, it's set to an empty string. So yeah, it might be a little tricky.

n1k0 · 2017-08-18T11:45:58Z

test/utils_test.js

+          });
+        });
+      });
+      describe("true condition", () => {


Nit: empty line between describe statements.

It can be done! 😄

FYI: I appreciate the "Nit" comments. Every little bit counts.

mfulton26

Thank you for the comments so far @n1k0. I very much appreciate it.

mfulton26 · 2017-08-05T13:32:45Z

src/utils.js

+    // Process dependencies updating the local schema properties as appropriate.
+    for (const [key, value] of Object.entries(dependencies)) {
+      // Skip this dependency if its trigger property is not present.
+      if (!formData[key]) {


I looked but I have not yet found a more proper way to determine if a field is defined. This is a "falsey" check which may not be appropriate. If someone could give me a suggestion on what to use instead that would be very much appreciated. Thank you.

mfulton26 · 2017-08-18T12:35:49Z

playground/samples/propertyDependencies.js

+        src:
+          "https://spacetelescope.github.io/understanding-json-schema/reference/object.html#dependencies",
+        description:
+          "In the following example, whenever a `credit_card` property is provided, a `billing_address` property must also be present.",


Oh interesting, I didn't think of that. I'll take a look. Thank you.

mfulton26 · 2017-08-18T12:37:04Z

src/components/fields/ArrayField.js

+        const itemSchema = retrieveSchema(
+          schema.items,
+          definitions,
+          formData[index]


Good point, I felt like I was "hacking" a bit as I am still familiarizing myself with the code base. Thank you for the feedback. I will update.

mfulton26 · 2017-08-18T12:38:06Z

src/utils.js

+      definitions,
+      formData
+    );
+  } else if (schema.hasOwnProperty("dependencies")) {


I was just thinking of this this morning when I started thinking about this pull request again due to others commenting on the pull request. I will see if I can have some updates up today or tomorrow. Thank you.

mfulton26 · 2017-08-18T12:39:05Z

src/utils.js

+    // Drop the dependencies from the source schema.
+    let { dependencies, ...localSchema } = schema;
+    // Process dependencies updating the local schema properties as appropriate.
+    for (const key of Object.keys(dependencies)) {


Because I'm an EcmaScript noob? 😄

mfulton26 · 2017-08-18T12:40:00Z

src/utils.js

+      // Skip this dependency if its trigger property is not present.
+      if (!formData[key]) {
+        // fixme: a falsey check may not be appropriate here, I'm not sure
+        continue;


Hmm, I'll try that but I may already have done so. If a field is left empty does it come in as undefined in the formData or as an empty string ""? I guess I will find out. :-)

mfulton26 · 2017-08-18T12:43:50Z

src/utils.js

+            true
+          )
+        );
+        if (Array.isArray(value.oneOf)) {


Yeah, I'm still not certain how oneOf would work in general but I kind of view the usage here as special. The idea is that depending on values in formData the overall schema will dynamically update to one of several options but this still doesn't support a user selecting one of several schemas to interact with as one would need if using oneOf elsewhere in a schema. I have started updating README.md locally and I am planning on explaining just this as implementing support for this kind of oneOf is very independent of general support for oneOf (and vice versa I believe although I could be wrong).

mfulton26 · 2017-08-18T12:45:41Z

src/utils.js

+              };
+              const { errors } = jsonValidate(formData, conditionSchema);
+              if (errors.length === 0) {
+                const dependentSchema = { ...subschema };


Correct. I am just trying to not mutate the incoming schema at all so I make copies as needed in the tree instead of recursively coping the entire tree (although I suppose the later could be done for readability/maintainability reasons although I'm not sure how to do that off the top of my head).

mfulton26 · 2017-08-18T12:46:04Z

test/utils_test.js

+          });
+        });
+      });
+      describe("true condition", () => {


It can be done! 😄

mfulton26 · 2017-08-18T12:46:24Z

test/utils_test.js

+          });
+        });
+      });
+      describe("true condition", () => {


FYI: I appreciate the "Nit" comments. Every little bit counts.

n1k0

This looks generally very good to me, though I fear missing something important. I'd like more feedback and reviews from people like @olzraiti @mfulton26 @faassen or @glasserc before giving final approval.

n1k0 · 2017-08-19T06:12:14Z

src/utils.js

  } else {
    // No $ref or dependencies attribute found, returning the original schema.
    return schema;
  }
 }

+function resolveDependencies(schema, definitions, formData) {
+  // Drop the dependencies from the source schema.
+  let { dependencies, ...resolvedSchema } = schema;


We should probably default dependencies to an empty object to avoid throwing on Object.keys(undefined), what do you think?

Sounds good to me.

n1k0 · 2017-08-19T06:12:57Z

src/utils.js

+  // Drop the dependencies from the source schema.
+  let { dependencies, ...resolvedSchema } = schema;
+  // Process dependencies updating the local schema properties as appropriate.
+  for (const key of Object.keys(dependencies)) {


Nit: this could still be for..in

Yep, I missed that when I moved things around before.

n1k0 · 2017-08-19T06:35:52Z

src/utils.js

+    return schema;
+  }
+  schema = { ...schema };
+  schema.required = Array.isArray(schema.required) ? [...schema.required] : [];


Nit: Would be so much nicer to have a way to properly clone the whole schema in a single call... maybe just introduce a simple clone function in utils leveraging JSON.parse(JSON.stringify(obj)), so we'll have a single generic reference impl. we'll be able to tweak whenever required.

I don't really feel that cloning is necessary here, especially if you replace the for loop below with a filter call. You could even take it a step further and use a Set.

I think using a Set makes for a good solution. I'll try that for now.

n1k0 · 2017-08-19T06:50:48Z

src/utils.js

+      continue;
+    }
+    schema.required.push(property);
+  }


Nit: this for loop looks like it could be more legibly replaced by a Array#filter call. Or maybe not, we should at least try.

With the Set implementation this actually goes away. 👍

glasserc

Hi, thanks for tagging me in on this @n1k0 ! It's a great PR, thoroughly documented and adding tests, both of which are great signs.

My biggest concern here is that before this PR, rjsf doesn't support oneOf at all, but after this PR, rjsf "kind of, sort of" supports oneOf under certain very restrictive conditions. Those conditions aren't really expressed anywhere, either. I imagine this behavior is important to you because the PR would be much simpler without it. On the other hand, this lets us explore oneOf in a simpler way without having any questions of what UI would be necessary to let the user change to an alternative schema. I guess I'm OK including this behavior if we document exactly what it does and when. (Something explaining that you can use oneOf to handle more complicated cases, and how we examine oneOf alternatives one at a time, comparing just the initial property against the definition of that property in each alternative, and adding the rest of the schema if it matches.)

I'm a little concerned about weird recursive cases, like what do we do with

{
  "type": "object",
  "properties": {
    "a": {"enum": ["a1", "a2"]},
    "b": {"enum": ["b1", "b2"]}
  },
  "dependencies": {
    "a": {
      "oneOf": [
        {"properties": {
          "a": {"enum": ["a1"]}, 
          "b": {"enum": ["b1"]}
        }},
        {"properties": {
          "a": {"enum": ["a2"]}, 
          "b": {"enum": ["b2"]}
        }}
      ],
    "b": {
      "oneOf": [
        {"properties": {
          "b": {"enum": ["b1"]},
          "a": {"enum": ["a2"]}
        }},
        {"properties": {
          "b": {"enum": ["b2"]},
          "a": {"enum": ["a1"]}
        }}
      ]
    }
  }
}

... but maybe this isn't a fair criticism, because there are certainly an infinite number of invalid JSON schemas that we can't really expect to detect or handle.

I nitpicked a few of your function names. To me resolve carries a meaning similar to reduce -- to take one thing and simplify some aspect of it somehow. But I haven't really been involved with the maintenance of this project much lately so feel free to disregard my opinion on this subject. 😳

glasserc · 2017-08-21T21:23:16Z

src/utils.js

+    return schema;
+  }
+  schema = { ...schema };
+  schema.required = Array.isArray(schema.required) ? [...schema.required] : [];


I don't really feel that cloning is necessary here, especially if you replace the for loop below with a filter call. You could even take it a step further and use a Set.

glasserc · 2017-08-21T21:25:04Z

src/utils.js

+  return resolvedSchema;
+}
+
+function resolvePropertyDependency(schema, properties) {


I don't really like this name for this function. How about addRequired ?

I don't like it either but I couldn't think of something better at the time. 😄

I'm actually wonder if withDependentProperties would make sense. That way it is clear that this function returns a new schema object with the additional dependent properties (in the required property which is how property dependencies work).

Thoughts?

glasserc · 2017-08-21T21:26:57Z

src/utils.js

+  return schema;
+}
+
+function resolveConditionalSchema(schema, definitions, formData, value) {


I don't like this function name either. How about mergeSchemas ? (Changing the value parameter to something like additionalSchema or even conditionalSchema.)

glasserc · 2017-08-21T21:35:24Z

src/utils.js

+    const {
+      [key]: conditionProperty,
+      ...dependentProperties
+    } = subschema.properties;


I had to stare at this for a little while trying to figure it out.. maybe rename dependentProperties to propertiesToAdd, and/or add a comment like Remove key from the properties we will merge in. Instead, use it as a condition?

glasserc · 2017-08-21T21:36:40Z

src/utils.js

+      const { errors } = jsonValidate(formData, conditionSchema);
+      if (errors.length === 0) {
+        const dependentSchema = { ...subschema };
+        dependentSchema.properties = dependentProperties;


You can do this in one line: const dependentSchema = { ...subschema, properties: dependentProperties };

glasserc · 2017-08-21T21:37:42Z

src/utils.js

+function resolveConditionalSchema(schema, definitions, formData, value) {
+  return retrieveSchema(
+    mergeObjects(schema, retrieveSchema(value, definitions, formData), true)
+  );


Why is the outer retrieveSchema necessary? (A comment might help.)

It might not be... I'l do some testing and see. I think I was too liberal with calling retrieveSchema

glasserc · 2017-08-21T21:38:07Z

src/utils.js

+        schema = mergeObjects(
+          schema,
+          retrieveSchema(dependentSchema, definitions, formData)
+        );


Is an outer retrieveSchema necessary here? Alternately, should we use mergeSchema (above)?

(Oops, I posted this comment on the wrong line before...)

I added a test justifying the need for a call to retrieveSchema, plus fixed a minor bug that such a test uncovered.

glasserc · 2017-08-21T21:38:57Z

src/utils.js

+        schema = mergeObjects(
+          schema,
+          retrieveSchema(dependentSchema, definitions, formData)
+        );


Should we break once we find a oneOf that matches?

This probably actually needs to filter all of the subschemas and confirm that only one is valid. I will update.

glasserc · 2017-08-21T21:45:19Z

src/utils.js

+  );
+  if (Array.isArray(oneOf)) {
+    schema = resolveDynamicSchema(schema, definitions, formData, key, oneOf);
+  }


If oneOf isn't an array, what is the correct behavior? I don't think silent failure is correct..

Hmm, I agree. I haven't looked yet throughout this project. Do we ever display to the user that they have created a bad JSON schema? Or do we log warnings for this kind of thing in the console?

I've changed this to throw, which we appear to do for some schemas that are too dysfunctional to work. (For example, getDefaultFormState throws on non-object schemas.)

mfulton26 · 2017-08-25T21:20:13Z

@glasserc I took a quick glance at your input and I totally agree I've just been pretty busy this week with other things. I should be able to make some updates soon though. Thanks!

mfulton26 · 2017-09-02T03:41:22Z

Things I still need to do:

update README.md to explain special oneOf usage in dependencies only
address "Warning: Failed prop type: The prop id is marked as required in BaseInput, but its value is undefined."
re-review previous comments and code and see what else can be cleaned up

Thank you for the feedback, please feel free to reiterate and/or provide additional feedback as some of the decisions I've made in an attempt to "improve" things might not have been the best decisions. 😄

mfulton26 · 2017-09-02T03:43:01Z

Hmmm, I'm not sure why the build is working but I'm not going to look at this again until at least tomorrow so it can wait until at least then.

- Property dependencies: Make additional properties required whenever a trigger property is specified. - Schema dependencies: Dynamically add schema properties based on data entered.

mfulton26 · 2017-10-23T12:31:21Z

Okay, I think this ready to be reviewed again.

glasserc · 2017-10-23T16:01:26Z

I took the liberty of adding a little bit of documentation to the README explaining in the vaguest possible terms how this works and fixed up a few stylistic nits that bothered me. The tests seem to pass, so I'm merging it. Thanks so much for your work on this!

mfulton26 · 2017-10-23T19:50:13Z

src/utils.js

@@ -422,7 +422,7 @@ function resolveDependencies(schema, definitions, formData) {
  // Process dependencies updating the local schema properties as appropriate.
  for (const dependencyKey in dependencies) {
    // Skip this dependency if its trigger property is not present.
-    if (formData[dependencyKey] === undefined) {
+    if (!formData.hasOwnProperty(dependencyKey)) {


@glasserc Unfortunately it looks like this change broke properties and schema dependencies. With the change conditionally required properties are always marked as required (with an asterisk (*)) and conditionally declared properties are always shown. See the playground for the examples.

Note that the dynamic schema still works correctly with the specific oneOf case.

I'm surprised this passed the tests... I must not have written very good ones. 😄

Yeah, that's unfortunate.. the way we computeDefaults fills missing fields with undefineds. I don't like that design but it's outside the scope of this PR. Thanks for letting me know that I broke it :)

mfulton26 · 2017-10-23T21:36:08Z

Awesome. I'm happy you've made the additions. This is really great and thank you all for supporting the change!

mfulton26 · 2017-10-23T21:37:36Z

I think the hasOwnProperty change may have caused a hiccup though. Please see my comment on your commit. Thank you.

mfulton26 · 2017-10-25T22:38:17Z

Just out of curiosity, how soon might we be able to get the latest version deployed to the playground? Thanks!

cdtinguria · 2017-10-28T03:30:23Z

Guys, very nice feature to have. Any idea when this will be released?

mfulton26 · 2017-11-01T15:45:45Z

@glasserc This appears to be published to npm. I'm curious how we get the playground updated. Do you know? Thanks.

glasserc · 2017-11-01T20:29:15Z

Sorry, I forgot to push it to the playground. It should be up now.

mfulton26 · 2017-11-02T01:07:06Z

Thanks!

mfulton26 force-pushed the dependencies branch from 23686e8 to 6fc5633 Compare August 6, 2017 01:39

n1k0 reviewed Aug 18, 2017

View reviewed changes

mfulton26 commented Aug 18, 2017

View reviewed changes

n1k0 reviewed Aug 21, 2017

View reviewed changes

glasserc reviewed Aug 21, 2017

View reviewed changes

mfulton26 force-pushed the dependencies branch from 2433f70 to 306dcce Compare September 14, 2017 20:11

glasserc mentioned this pull request Sep 22, 2017

Easier Bootstrap Grid usage and basic BS4 compatibility #623

Closed

9 tasks

mfulton26 force-pushed the dependencies branch 3 times, most recently from 70479e6 to a5d81a7 Compare October 21, 2017 00:35

Add initial dependencies support

9235bac

- Property dependencies: Make additional properties required whenever a trigger property is specified. - Schema dependencies: Dynamically add schema properties based on data entered.

mfulton26 force-pushed the dependencies branch from a5d81a7 to 9235bac Compare October 21, 2017 00:50

README.md: explain usage in schema dependencies

c708a75

glasserc added 5 commits October 23, 2017 11:21

Add documentation explaining how oneOf works here

b3b9557

Throw error on wildly invalid schema

b0dc5c3

Use hasOwnProperty to check for possibly-missing key

39519e3

Tests that demonstrate a need for a retrieveSchema

d41ef3a

retrieveSchema needs to happen before checking for oneOf

4b3766c

glasserc merged commit 132a6cc into rjsf-team:master Oct 23, 2017

mfulton26 commented Oct 23, 2017

View reviewed changes

mfulton26 deleted the dependencies branch October 24, 2017 01:38

Initial support for property & schema dependencies #659

Initial support for property & schema dependencies #659

Conversation

mfulton26 commented Aug 5, 2017 • edited Loading

Reasons for making this change

Checklist

ksjogo commented Aug 16, 2017

faassen commented Aug 18, 2017

n1k0 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1k0 Aug 18, 2017 • edited Loading

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

Choose a reason for hiding this comment

mfulton26 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

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

n1k0 left a comment • edited Loading

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

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

glasserc left a comment • edited Loading

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

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

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

mfulton26 commented Aug 25, 2017

mfulton26 commented Sep 2, 2017

mfulton26 commented Sep 2, 2017

mfulton26 commented Oct 23, 2017 • edited Loading

glasserc commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfulton26 commented Aug 5, 2017 •

edited

Loading

n1k0 Aug 18, 2017 •

edited

Loading

n1k0 left a comment •

edited

Loading

glasserc left a comment •

edited

Loading

mfulton26 commented Oct 23, 2017 •

edited

Loading