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

Support for oneOf / anyOf #229

Closed
wants to merge 2 commits into from
Closed

Support for oneOf / anyOf #229

wants to merge 2 commits into from

Conversation

KtorZ
Copy link

@KtorZ KtorZ commented Mar 10, 2017

Hey (thanks for 4899f3e) !

I am opening this PR as a discussion about a feature I do need; I am willing to implement it with some guidances of simply, letting it you do if you feel like it.

Add support for oneOf and anyOf keywords

At the moment, the allOf keywords gets more or less merged inside one big object in order to be represented. I am in case where I have to provide different schema possible for a given endpoint, schemas that doesn't have any discriminator properties (they are actually nested schemas of a parent which has a discriminator).

So, as a start, I started to add a new _widgetType which led at the end to a lot of duplication. Actually, the way oneOf or anyOf keywords would work is quite similar to the way objects do, except that you first have to selet which object you want to view. So, I went for another strategy: wrapping each object in a list, and dealing with same in an uniform manner then; A drop-down is displayed to select which object to display when there're more than one possible choice, otherwise it falls back to the old (current) behavior and simply display the object schema without drop-down.

I've added an example to the demo under the "Change behaviors" operation which looks like this:

screenshot from 2017-03-10 13-28-22

Issues & Improvement

If you like the idea so far, there're a few things I would like to add / discuss. First of all, the current change doesn't work for nested oneOf of anyOf but only if they are defined at the root-level of an object. When nested, an empty drop-down is rendered, and no schema at all is displayed. Some inputs here are welcomed.

Another one is about the titles displayed in the drop-down. I am currently simply taking the "title" defined at the schema level which isn't necessarily the most readable thing. I was thinking about adding a x-oneOf-title custom property or something like that to override it if necessary.

Hoping we can collaborate on that one :)
PS: My knowledge of Angular 2 and TypeScript are closed to 0.

@RomanHotsiy
Copy link
Member

Wow @KtorZ! You rock! 🔥 I didn't expect this PR :)

But I am still not sure how I would like to implement this. When it goes to nested schemas, my code becomes very messy :( OpenAPI/Swagger is really non-trivial to work with as it has milion of variants to define same thing. When I started I didn't know all them. Now I have a better understanding.

So I am considering some refactor to make code more readable and stable.

If you are OK with it, I will make refactor first (keeping in mind anyOf and oneOf).
I hope I will finish it till the end of next week. Then I will reach you and we'll discuss how to implement this feature.

What do you think.

@brendo
Copy link
Contributor

brendo commented Mar 10, 2017

Wow, this is awesome. This was next on my list too! Unfortunately our API makes use of oneOf and anyOf as well so we've been documenting around it.

I was excited to see it in the OpenAPI V3 spec and have been looking for renderers ever since :)

@brendo
Copy link
Contributor

brendo commented Mar 10, 2017

Oh, so what I forgot to add is that if you need any help testing this feature I'm happy to lend a hand

@KtorZ
Copy link
Author

KtorZ commented Mar 10, 2017

@RomanGotsiy That sounds fair :)
I do not need it urgently - It's only for one part of our API which is quite broad and, there's nothing preventing me of specifying as oneOf / anyOf, things just don't get displayed.

If I can however already communicate a bit about an implementation idea, I would say that considering any object as an array of objects simplifies things a lot. They can all be handled the same manner, single objects become singletons and, from a display perspective the drop-down can simply be omitted for singletons.

Having this approach in a refactor could lead to an extremely easy implementation for oneOf-like keywords :)

@brendo
Copy link
Contributor

brendo commented Mar 13, 2017

Had a little play with this today and it doesn't fit my exact use case. The current implementation just checks for oneOf/anyOf at the root level, which simplifies it a bit, at the expense of flexibility. For example, we define oneOf within the schema definition to describe the required property sets, a little like this:

{
  "$schema": "http://json-schema.org/draft-04/schema#",
  "properties": {
    "bearerToken": {
      "type": "string"
    },
    "accountId": {
      "type": "string"
    },
    "email": {
      "type": "string",
      "format": "email"
    },
    "password": {
      "type": "string"
    },
    "scope": {
      "type": "string"
    }
  },
  "oneOf": [
    {
      "required": [
        "bearerToken",
        "accountId",
        "scope"
      ]
    },
    {
      "required": [
        "email",
        "password",
        "scope"
      ]
    }
  ]
}

I realise supporting this usage is where the complexity comes in, the UI could be dramatically different as this changes.

@RomanHotsiy
Copy link
Member

RomanHotsiy commented Mar 13, 2017

@brendo yes, it is not working correct for all the cases. But trying to make this work for all the cases will be nightmare at the moment. As I said, the code is too complex so I have to do a refactor. I will make the code great again 😂

Btw, I am considering implementing support for OAS 3.0 as part of this refactor (#207)

@eitan101 eitan101 mentioned this pull request Mar 14, 2017
@RomanHotsiy
Copy link
Member

Hey guys, sorry for the delay with this.

I'm still busy with other things. Most likely I will start working on that at the end of the next week.
Just FYI :)

@KtorZ
Copy link
Author

KtorZ commented Apr 24, 2017

Good news!
I was about to ask some actually. If you think you might need any help, shout it out :)

@raderio
Copy link

raderio commented Aug 16, 2017

Any news about this PR?

@KtorZ KtorZ closed this Aug 18, 2017
@RomanHotsiy
Copy link
Member

Hey @KtorZ why did you close this?
I hope you just closed it in favour of #327 and not because I didn't merge it for so long time

@KtorZ
Copy link
Author

KtorZ commented Aug 22, 2017

@RomanGotsiy Not at all ^^
I just wanted to close it before. The PR was more like an open discussion and doesn't make much sense code-wise especially after the changes ReDoc is going (or will go soon enough I guess).

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.

4 participants