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

Add support for anyOf #417

Closed
wants to merge 11 commits into from
Closed

Add support for anyOf #417

wants to merge 11 commits into from

Conversation

stathismor
Copy link

@stathismor stathismor commented Nov 23, 2016

Reasons for making this change

This change adds support for anyOf. The logic is built in the ArrayField component and handles all the various supported types (number, string, array etc).

Areas that need improvement:

  • The layout is maybe not the prettiest.
  • UISchema is not supported for anyOf. The challenge here, as far as I understand it is that UISchema needs specific types defined in the JSONSchema, whereas with anyOf, the type is determined on the fly.
  • Array with anyOf, cannot also have fixed items at the moment.
  • anyOf item of type object is not supported. null is also not supported, but I saw there is a pull request (Initial implementation of nullable type #329) for that.

Checklist

  • 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'm adding a new feature
    • I've updated the playground with an example use of the feature

@cyxou
Copy link

cyxou commented Jan 11, 2017

Why it hasn't been merged yet?

@stathismor
Copy link
Author

@cyxou I am wondering this myself... I am happy to re-work on it, if there are any comments, and update the branch (there's quite a few conflicts as ArrayField has been updated since my PR).

@inostia
Copy link

inostia commented Jan 26, 2017

+1, my project requires this and is on hold until this gets merged.

@Natim
Copy link
Contributor

Natim commented Jan 27, 2017

+1, my project requires this and is on hold until this gets merged.

Don't hesitate to use this branch in your project in the meantime.

@kkarimi
Copy link

kkarimi commented Jan 27, 2017

+1 for merging

@Natim
Copy link
Contributor

Natim commented Jan 27, 2017

I guess the most important missing feature is the support of uiSchema. The anyOf feature is important to support but this PR doesn't support it yet at the standard we want for that lib.

We need at least to improve the UX of using anyOf as well as supporting the uiSchema.

Merging is easy but supporting an incomplete implementation of the spec in the lib is a path we are not prepared to take.

However this work is a great start and we are highly interesting in this, feel free to keep iterating and improving this Pull-Request and we will merge it as soon as we feel comfortable with maintaining it on the long term.

@stathismor
Copy link
Author

Thanks for the detailed response @Natim! I wasn't aware what was wrong with it, but after your explanation, it makes sense. I remember updating the uiSchema was quite challenging, as it does not support dynamically added types... But now that I see there's some interest in this PR, I will keep working on it, first by updating it with master, then trying to add support for uiSchema

};

anyOfOptions(anyOfItems) {
return anyOfItems.map(item => ({value: item.type, label: item.type}));
Copy link

Choose a reason for hiding this comment

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

Should it be label: item.title?

Copy link
Author

Choose a reason for hiding this comment

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

Good spot, thanks! I think it should, yes. Maybe item.title || item.type, in case it's not defined...

Copy link

Choose a reason for hiding this comment

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

Sounds good to me!

@exchgr
Copy link

exchgr commented Feb 9, 2017

Will this be able to support inheritance? Suppose I have an array of widgets. A widget can be of several types: Button, Slider, and Select. Button and Slider have the same attributes, but Select allows the user to define what's in its list.

The issue I'm running into is that they all need to be "object"s, and having multiple items in an anyOf array with the same type causes it to just select the first item with that type.

Here's an example of what I'm trying to do:

items: {
  anyOf: [
    {
      type: "object",
      title: "Button",

      properties: {
        name: {
          type: "string",
          title: "Name"
        }
      }
    },

    {
      type: "object",
      title: "Slider",

      properties: {
        name: {
          type: "string",
          title: "Name"
        }
      }
    },

    {
      type: "object",
      title: "Select",

      properties: {
        name: {
          type: "string",
          title: "Name",
        },

        fields: {
          "$ref": "#/definitions/select_fields"
        }
      }
    }
  ]
}

@stathismor
Copy link
Author

stathismor commented Feb 10, 2017

@exchgr I see what you're saying. This was deliberate as I did not have a use case for multiple items with same type. I think that part of code responsible is this line, that tries to find the selected item in the anyOf schema.

It could be easily changed so that title is preferred instead type. But title is not an obligatory property and is not required by object. And it would break for cases where an anyOf schema has multiple types with the same title. I think there is a simple custom change you can make if you want. If the SelectWidget uses the title as the identifier instead of the type, then it should just work...

I guess the problem lies in the fact that there is no unique identifier between anyOf items. Ideally I think it should not reference the items by type, but also by the order in the schema. Or maybe, now that I think about it, it could be just the order... Thanks for the comment, I will check whether order can be used, next time I have a look at it!

@exchgr
Copy link

exchgr commented Feb 13, 2017

@jManji Thanks for the quick response. Going by the array index may solve the issue. I was also thinking that it'd be useful to have a way for the dropdown to influence an attribute that indicates the type on the backend.

Building on my example, suppose we have a few classes on the backend (in this case, Rails). Here's an example with polymorphic associations emulating inheritance across multiple tables (aka Multiple Table Inheritance):

class DeviceType < ApplicationRecord
  has_many :controls
  # we're also able to work with each control's "controllable" association directly, through some methods not defined in this example
end

class Control < ApplicationRecord
  belongs_to :device_type
  belongs_to :controllable, polymorphic: true
end

class Button < ApplicationRecord
  has_one :control, as: :controllable
end

class Slider < ApplicationRecord
  has_one :control, as: :controllable
end

class Select < ApplicationRecord
  has_one :control, as: :controllable
end

Then in the JSON schema:

items: {
  anyOf: [
    {
      type: "object",
      title: "Button",

      properties: {
        name: {
          type: "string",
          title: "Name"
        },

        controllable_type: {
          enum: ["Button"]
        }
      }
    },

    {
      type: "object",
      title: "Slider",

      properties: {
        name: {
          type: "string",
          title: "Name"
        },

        controllable_type: {
          enum: ["Slider"]
        }
      }
    },

    {
      type: "object",
      title: "Select",

      properties: {
        name: {
          type: "string",
          title: "Name",
        },

        controllable_type: {
          enum: ["Select"]
        }

        fields: {
          "$ref": "#/definitions/select_fields"
        }
      }
    }
  ]
}

Since I actually need this for something I'm working on, I'm going to take a stab at it today.

Copy link
Contributor

@glasserc glasserc left a comment

Choose a reason for hiding this comment

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

Impressive!

Sorry for the delay in reviewing this -- @n1k0 has been busy on other projects and I'm only just coming up to speed on this project. Here are my thoughts on this approach.

How come the logic for this is part of the ArrayField class? Just to simplify, I guess? I think we should come up with a clear understanding of what cases we want to cover, and I don't think this includes all of them. As an example, this seems like something we'd want to support:

{
  "type": "object",
  "anyOf": [{
    "properties": {"foo": {"type": "number"}},
    "additionalProperties": false
  }, {
    "properties": {"bar": {"type": "string"}},
    "additionalProperties": false
  }],
}

[i.e. an object with two possible cases.]

Here's another one:

{
  "type": "object",
  "properties": {
    "userId": {
      "anyOf": [
        {"type": "number"},
        {"type": "string"}
      ]
    }
  }
}

[i.e. an object with a field that has two possible types; this particular example could be implemented using "type", but more complicated examples using objects can be found in the wild, per @exchgr's comment]

I'm not sure about this:

{
  "type": "object",
  "properties": {"version": {"type": "string"}},
  "anyOf": [{
    "properties": {"username": {"type": "string"}}
  }, {
    "properties": {"email": {"type": "string"}}
  }]
}

[i.e. an object with one mandatory property and two possible cases]

I'd say we should not bother even trying to support something like:

{
  "type": "object",
  "anyOf": [
    {"type": "integer"},
    {"type": "string"}
  ]
}

I'm not averse to accepting a PR that only supports some of these cases, of course, so long as it's maintainable and it supports implementing the other cases. I'm not sure this PR does, since it puts the logic in arrays rather than in items, where they would be both more powerful and I think simpler.

I also found the UI a little clunky -- rather than having a "select" to choose what kind of object it is, why not use something like Bootstrap's "radio" buttons functionality?

Also, why did you move items from arrayField.state.items to arrayField.state.formData.items?

@@ -30,4 +31,5 @@ export const samples = {
Files: files,
Single: single,
"Custom Array": customArray,
"Any of": anyOf
Copy link
Contributor

Choose a reason for hiding this comment

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

At first this bothered me, but I see our capitalization isn't consistent -- Custom Array vs. Date & time.

Copy link

Choose a reason for hiding this comment

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

In any case (😉), leaving "of" as lowercase is correct, even when you're using title case. http://grammar.yourdictionary.com/capitalization/rules-for-capitalization-in-titles.html

it("should render an add button", () => {
expect(node.querySelector(".array-item-add button"))
.not.eql(null);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this test really necessary? If there were no add button, the beforeEach would fail, wouldn't it?

it("should render a select along with the input field", () => {
expect(node.querySelectorAll(".array-item"))
.to.have.length.of(1);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test is misnamed, since it only checks for the array item, not for a select field.

it("should render two select and input fields", () => {
Simulate.click(node.querySelector(".array-item-add button"));
expect(node.querySelectorAll(".array-item")).to.have.length.of(2);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, no select fields.

const {definitions} = registry;
const anyOfItemsSchema = this.getAnyOfItemsSchema();
const newItems = items.slice();
const foundItem = anyOfItemsSchema.find((element) => element.type === value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work with things like $ref?

Copy link

Choose a reason for hiding this comment

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

I've tried it & it doesn't support $ref

<div className="form-group" style={{width: 120}}>
<SelectWidget
schema={{type: "integer", default: selectWidgetValue}}
id="select-widget-id"
Copy link
Contributor

Choose a reason for hiding this comment

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

The same ID shouldn't be used more than once in a DOM.

@@ -50,6 +50,7 @@ function DefaultArrayItem(props) {
<div key={props.index} className={props.className}>

<div className={props.hasToolbar ? "col-xs-9" : "col-xs-12"}>
{props.selectWidget}
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems kind of strange for selectWidget to be part of the props, and not part of the template.

@b1r3k
Copy link

b1r3k commented Mar 17, 2017

@glasserc Thanks for review. Argument for extending support for anyOf beyond array makes sense + it's very useful to me at this moment.

For example I would like to use this schema:

{
    "definitions": {
        "TPNode": {
            "title": "TPNode",
            "description": "TPNode",
            "type": "object",
            "properties": {
                "name": {
                    "type": "string"
                },
                "value": {
                    "anyOf": [
                        { "type": "string"},
                        {
                            "type": "array",
                            "items": {
                                "$ref": "#/definitions/TPNode"
                            }
                        }
                    ]
                }
            }
        }
    },
    "$ref": "#/definitions/TPNode"
};

Here is how it should render

@jManji are you intending to work this PR according to @glasserc suggestions any time soon?

@stathismor
Copy link
Author

Sorry for late reply guys. I've been quite busy lately, and didn't have time to properly look at this. @b1r3k yes, I would very much like to rework on this, when I have some time, based on @glasserc's suggestions. And there's a couple of things that came up from previous conversations here. I would say I should have something in the next couple of weeks.

@glasserc, thanks a lot for the detailed CR, appreciated! Just trying to answer some of your questions without touching the code yet:

How come the logic for this is part of the ArrayField class? Just to simplify, I guess?

I think that was because I wanted to make use of the ArrayField functionality that is there already, like adding multiple properties, all the UI stuff that comes with it, etc. anyOf looks and behaves a bit like an array, so I thought I could fit it in there (like for instance, how additionalItems feature does). Maybe a new component type makes more sense? That seemed much more work I guess.

I also found the UI a little clunky -- rather than having a "select" to choose what kind of object it is, why not use something like Bootstrap's "radio" buttons functionality?

You think radio is better for this case? How would you render lots of options? Will it not occupy too much space and make the form look a bit inconsistent? Not sure, but that's and easy change, if that's what we want.

Also, why did you move items from arrayField.state.items to arrayField.state.formData.items?

That's a good question... I must have had a reason, I will let you know on my next commit 😄. If I see no reason, I will just move it back to state.items

@exchgr
Copy link

exchgr commented Mar 21, 2017

Perhaps if this supports uiSchema, we could let the developer decide whether to use radios or selects.

@glasserc
Copy link
Contributor

I like @exchgr's suggestion to support uischema. As a default, I'd propose this heuristic: if the anyOf options are only two or three, I think radio buttons are more convenient (just click the one you want); if they are four or more, a select is probably more respectful of screen real estate.

Maybe a new component type makes more sense?

Could we maybe put it on SchemaField? That was my first instinct (coming from a classical OO background), since any schema could potentially be anyOf. On the other hand, that might make SchemaField too complicated. Maybe @n1k0 has an opinion?

@n1k0
Copy link
Collaborator

n1k0 commented Mar 22, 2017

I also like the idea of using uiSchema to configure the type selector a lot, though I fear the added complexity. Personally, I'd probably iterate here. Good support for anyOf is already hard to get right :)

Otherwise I concur with @glasserc, SchemaField feels like the natural place to implement such support.

Side note, @jManji could you please try to install prettier v0.22.0 and run:

$ prettier --jsx-bracket-same-line --trailing-comma es5 '{playground,src,test}/**/*.js' --write

on your current branch, then try to merge/rebase it with latest master? I also fear we're gonna eventually diffing and conflicting a lot if we postpone this operation too much. Thanks!

@stathismor
Copy link
Author

The challenge I see with uiSchema is that it is keyed by the property's key. You cannot just say, "I want anyOf items to be textarea" for instance. You have to define which anyOf item you refer to, and since we potentially want (currently this branch does not support it) multiple anyOf items of the same type, the uiSchema needs to probably reference the order/index of the anyOf item (?). I will definitely leave the uiSchema out of the scope of the next iteration. If we want, we can start by figuring out how a uiSchema for anyOf would look like.

@glasserc, @n1k0 Regarding moving code to SchemaField, it's been some time since I looked at the code, but currently this also feels challenging/weird to me. It will definitely make SchemaField more complicated as you said, and I think, it will need to have some kind of support for dynamically added widgets, like ArrayField has. But I am sure you know the codebase much better than me, so I will look into how this can be achieved.

Side note, @jManji could you please try to install prettier v0.22.0 and run...

Yes, will do!

... I also fear we're gonna eventually diffing and conflicting a lot if we postpone this operation too much.

Yes, you are right, I know... I will try to work on this asap, going through every comment mentioned here and in the code review.

@b1r3k
Copy link

b1r3k commented Jun 8, 2017

@jManji any news regarding this PR? I'm willing to take it further if you do not have time. In fact I've forked your solution and fixed few mistakes in order to use it in our internal tooling but it has meant to be workaround and the further we go with development the more crippled we are due to divergence from origin/master

@stathismor
Copy link
Author

@b1r3k I am very sorry but unfortunately I don't plan on working on it again, any time soon. I needed that feature for my work, but now we have shifted priorities. Please feel free to work on it!

I haven't checked in detail, but maybe #581 solves the same problem in a different way?

@danrot
Copy link

danrot commented Jul 27, 2017

I have checked #581, but it looks to me like this is only built to be used in dropdown and selects. From what I've seen it doesn't allow you to build more complex objects than simple strings.

@b1r3k Have you continued working on it? Anything worth sharing? We would also desperately need a feature like this, although my use case would probably be a oneOf, although anyOf should also be good enough :-)

@bchansell
Copy link

Is there any way to install this through npm as a node_module?

@epicfaace
Copy link
Member

Closing this as this functionality has been implemented in #1118.

@epicfaace epicfaace closed this Jan 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.