Handle circular references #557

eggyal · 2017-04-22T02:33:55Z

Reasons for making this change

Fixes #556

Checklist

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

glasserc · 2017-04-24T17:26:59Z

Hi, this looks good, but I'd like to handle #554 before merging this -- I think it will make review easier.

Also, I saw "Schemas SHOULD NOT make use of infinite recursive nesting like this, the behavior is undefined." in http://json-schema.org/latest/json-schema-core.html, does this mean we can punt on handling the infinite recursion case? If we can't work on Node, that seems like a pretty big limitation...

eggyal · 2017-04-24T17:55:12Z

@glasserc: I agree with handling #554 first—as you can see, I actually rebased to include that fix in here because they're dependent.

Anyway, I fixed the Node issue in 6ebb3e4—per this comment on my bug report, the problem was that I hadn't defined getOwnPropertyDescriptor(). However, whilst the existing tests are now passing I still need to add a circular reference test case because I think it's still getting stuck somewhere else.

As regards the spec, I did wonder about that... but am not 100% sure whether it's talking about the same thing? I can't see how circularities can be avoided in many real world scenarios e.g. where the schema for "person" has a "children" property that is an array of "person" objects. I ran into exactly this problem with the (draft) UBL JSON schema (albeit with Locations that have AlternativeLocation children). So, even if the JSON Schema spec currently discourages circularities I suspect that will eventually have to be changed? json-ref-schema-parser, which I mentioned previously, happily handles a variety of different types of circularity.

eggyal · 2017-04-25T20:46:28Z

src/components/fields/ObjectField.js

@@ -45,7 +45,7 @@ class ObjectField extends Component {
      onBlur,
    } = this.props;
    const { definitions, fields, formContext } = this.props.registry;
-    const { SchemaField, TitleField, DescriptionField } = fields;
+    const { ObjectPropertyField, TitleField, DescriptionField } = fields;


With circular schema, we must avoid rendering everything: object properties should only be rendered when necessary. I define a new component, ObjectPropertyField that only renders the underlying SchemaField when necessary—i.e. if the schema requires the property to be present, if the formData contains an explicit value for it, or if the user explicitly requests the property to be shown.

Consider putting this comment as JSDoc on the ObjectPropertyField class.

eggyal · 2017-04-25T20:50:11Z

src/components/fields/ObjectField.js

+          const isDefault =
+            !(name in formData) ||
+            typeof Object.getOwnPropertyDescriptor(formData, name).get ===
+              "function";


Here I'm trying determine whether the value in formData was given explicitly, or if it's just a default value from the lazily-evaluated Proxy object (this is used in the ObjectPropertyField component to determine whether the underlying SchemaField should be rendered). It may be better to pass explicit formData separately from default data?

Yes, I have to say this is super ugly, and passing explicit formData and defaults would be better in a perfect world. On the other hand, I'm not sure I want to separate them as part of this PR. I think extracting this into a variable called isDefault is good enough, although maybe you could add a FIXME comment explaining this.

eggyal · 2017-04-25T20:51:15Z

src/components/fields/ObjectPropertyField.js

+        </div>
+      );
+    } else {
+      return <button onClick={this.show}>ADD {title}</button>;


No doubt these <button/> elements should be replaced with a better UI?

I'm pretty nervous about the UX in general. I know that @n1k0 took a swing at collapsibility before in #268 and wasn't able to find a UX that he found felt right for most use cases, and this feels like a similar situation to me. Can we add this only if we are in a circular-schema situation? (N.B. I haven't actually tried running the code so this may not be as intrusive as I imagine it being.)

…ence of a schema with [circular references](https://github.com/BigstickCarpet/json-schema-ref-parser/blob/master/docs/README.md#circular-refs). NB this approach uses ES2015 Proxies, which can't be polyfilled and therefore it requires native support from the JS engine. V8 currently has [a bug](https://bugs.chromium.org/p/v8/issues/detail?id=6290) which causes test suite to fail under Node. No tests written yet.

…hen explicitly necessary. Tests updated to make mandatory object properties that were previously optional but assumed to be mandatory.

glasserc

Hi, sorry for taking so long to get to this again. 😞

I agree with your analysis, that recursive schemas are likely to be important as JSON Schema becomes more utilized. I'm really grateful that you're working on it. I'm a little nervous it may turn out to be a bigger idea than can be contained in just one PR. In particular, I agree with your comment in #557 (comment) that trying to determine whether something is a default might be easier if we change how form data and defaults are given to components. If we do decide to make that change, that might be easier as a separate PR.

However, my biggest concern with this PR is the addition of expand/collapse buttons on everything. (Expand/collapse buttons, of course, are another idea that is big enough that it could be its own PR. See e.g. #268 and #271.) My interpretation of the history of the RJSF project is that we tend to follow the structure of the schema as much as possible, and that more generally, we try to take as simplistic an approach as possible, hoping that it yields a better UX. I'm hesitant to jeopardize the common case in the pursuit of the complete case.

I'd be much happier with this PR if collapsibility were only introduced where the schema is circular, and leaves the existing, simplistic UX for the common case. Of course, you might also want to extract the idea of collapsibility and try to get it reviewed separately, but I understand if that isn't exactly relevant to your goal.

glasserc · 2017-06-20T19:42:23Z

src/components/fields/ObjectField.js

@@ -45,7 +45,7 @@ class ObjectField extends Component {
      onBlur,
    } = this.props;
    const { definitions, fields, formContext } = this.props.registry;
-    const { SchemaField, TitleField, DescriptionField } = fields;
+    const { ObjectPropertyField, TitleField, DescriptionField } = fields;


Consider putting this comment as JSDoc on the ObjectPropertyField class.

glasserc · 2017-06-20T19:46:10Z

src/components/fields/ObjectField.js

+          const isDefault =
+            !(name in formData) ||
+            typeof Object.getOwnPropertyDescriptor(formData, name).get ===
+              "function";


Yes, I have to say this is super ugly, and passing explicit formData and defaults would be better in a perfect world. On the other hand, I'm not sure I want to separate them as part of this PR. I think extracting this into a variable called isDefault is good enough, although maybe you could add a FIXME comment explaining this.

glasserc · 2017-06-20T19:47:33Z

src/components/fields/index.js

@@ -14,6 +15,7 @@ export default {
  DescriptionField,
  NumberField,
  ObjectField,
+  ObjectPropertyField,


I'm not sure we really need to expose ObjectPropertyField?

glasserc · 2017-06-20T20:02:32Z

src/components/fields/ObjectPropertyField.js

+        </div>
+      );
+    } else {
+      return <button onClick={this.show}>ADD {title}</button>;


I'm pretty nervous about the UX in general. I know that @n1k0 took a swing at collapsibility before in #268 and wasn't able to find a UX that he found felt right for most use cases, and this feels like a similar situation to me. Can we add this only if we are in a circular-schema situation? (N.B. I haven't actually tried running the code so this may not be as intrusive as I imagine it being.)

glasserc · 2017-06-20T20:05:36Z

test/Form_test.js

            properties: {
              qux: { type: "string" },
            },
          },
        },
        type: "object",
+        required: ["foo"],


These are great examples of what I don't think should be necessary.

epicfaace · 2019-01-21T18:41:00Z

@eggyal now that the actual problem in #556 has been fixed, I guess the question is whether circular references should be supported.

How do you envision a form should look like when it's created from a schema such as this?

{
  "definitions": {
    "bar": {
      "type": "object",
      "properties": {
        "qux": {
          "$ref": "#/definitions/baz"
        }
      }
    },
    "baz": {
      "type": "object",
      "properties": {
        "quz": {
          "$ref": "#/definitions/bar"
        }
      }
    }
  },
  "type": "object",
  "properties": {
    "foo": {
      "$ref": "#/definitions/bar"
    }
  }
}

eggyal · 2019-01-22T08:02:43Z

@epicfaace:

How do you envision a form should look like when it's created from a schema such as this?

An object instance is valid even if properties are omitted (unless they are explicitly required by the schema). A circular schema is only sane if it can terminate, which will occur when (at some depth) a property in the instance is so omitted.

So, the UI must provide some means of omitting optional properties.

Moreover, optional properties within a circular path must not be rendered until they actually exist in the document instance else the rendering will never terminate: so the UI must provide some means of adding optional properties that do not yet exist in the document. I provided in my original PR a UI through which all properties (whether on circular paths or otherwise) would be omitted from the form until actually required (whether explicitly by schema or implicitly by document instance), but @glasserc (quite rightly) felt it would impair the UX for simple cases.

Taking that into consideration, perhaps RJSF needs to:

Identify all circular paths within the schema;
Only render properties that are on circular paths if required (by schema or by instance);
Provide a "delete" button by which any rendered property that is not required by the schema can be removed (whether on circular path or not); and
Provide an "add" button by which any not rendered property permitted by the schema can be added (whether on circular path or not).

This would still render entire forms in the simple cases (whilst also allowing optional properties within them to be properly deleted/omitted from the document, rather than merely left empty) but would furthermore support circularities.

Items 3 and 4 were implement in this PR, albeit could undoubtedly be made prettier. Item 2 was half implemented, insofar as this PR only rendered a property if it was required irrespective of whether it was on a circular path. So, the main suggestion here is really to identify the circular paths and add that as a discriminant in whether a field is rendered by default or not.

epicfaace · 2019-12-22T08:20:30Z

Closing this PR due to inactivity. It would be best to continue discussion of this issue in a new issue.

eggyal commented Apr 25, 2017

View reviewed changes

eggyal added 6 commits April 26, 2017 08:50

Handle recursive references

778fb91

Fixes failing tests

6ebb3e4

Evaluate idSchema lazily and only render optional object properties w…

132bc60

…hen explicitly necessary. Tests updated to make mandatory object properties that were previously optional but assumed to be mandatory.

Tests updated from rebasing.

114fb5e

Limit ArrayField item addition/deletion to minItems and maxItems

9d0b741

eggyal mentioned this pull request Apr 28, 2017

Corrected defaulting & non-required empty property handling #542

Closed

9 tasks

glasserc reviewed Jun 20, 2017

View reviewed changes

n1k0 mentioned this pull request Aug 16, 2017

Recursive references and uiSchema #670

Closed

epicfaace changed the title ~~Handle recursive references~~ Handle circular references Jan 21, 2019

epicfaace added the awaiting changes label Jan 21, 2019

epicfaace assigned eggyal Jan 21, 2019

glasserc mentioned this pull request Feb 4, 2019

Infinite $ref recursive error via computeDefaults #1160

Closed

3 tasks

epicfaace closed this Dec 22, 2019

Handle circular references #557

Handle circular references #557

Uh oh!

Conversation

eggyal commented Apr 22, 2017

Reasons for making this change

Checklist

Uh oh!

glasserc commented Apr 24, 2017

Uh oh!

eggyal commented Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eggyal Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glasserc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epicfaace commented Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eggyal commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epicfaace commented Dec 22, 2019

Uh oh!

Uh oh!

eggyal commented Apr 24, 2017 •

edited

Loading

eggyal Apr 25, 2017 •

edited

Loading

epicfaace commented Jan 21, 2019 •

edited

Loading

eggyal commented Jan 22, 2019 •

edited

Loading