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

Reduce duplications and complexity in v3.0 JSON Schema #1800

Merged
merged 10 commits into from
Mar 26, 2019
Merged

Reduce duplications and complexity in v3.0 JSON Schema #1800

merged 10 commits into from
Mar 26, 2019

Conversation

vearutop
Copy link
Contributor

@vearutop vearutop commented Jan 8, 2019

Current WIP Schema suffers from copy-paste, with these changes it can be improved.

Combine HTTP Methods in patternProperties

patternProperties:
  '^(get|put|post|delete|options|head|patch|trace)$': $ref: '#/definitions/Operation'

Combine some definitions as filtered supersets

In order to avoid massive duplication all possible properties can be defined in a superset.
Custom rules of exclusiveness can be further defined as a list of traits in allOf.

For example such structure does not allow having example and examples in same object:

not:
  required:
    - example
    - examples

Embed references

In order to simplify oneOf logic, many references can be embedded in respective definitions

All references to #/definitions/Schema are happening in such form:

oneOf:
  - $ref: '#/definitions/Reference'
  - $ref: '#/definitions/Schema'

Therefore #/definitions/Reference can be merged into #/definitions/Schema to simplify usages:

$ref: '#/definitions/Schema'

Validation difference after this change is that additional unknown properties are no longer accepted for References.
Such data would become invalid:

{"$ref": "#", "unknown":  1}

Validate example schemas

Modified schema fails validation against example schemas:

  • petstore-expanded.yaml: GET /pets has query parameter tags with invalid style simple,
  • api-with-examples.yaml: Object expected for examples, array received.

@handrews
Copy link
Member

handrews commented Jan 9, 2019

@vearutop I wish you'd commented on the issue before doing this. I'm thrilled to have someone else work on it but this is mostly but not quite a duplicate of what I was about to post later this week- it only didn't get posted during December because I got really sick for several weeks. So now I guess we try to put PRs next to each other and figure out the overlap and what needs to be kept from each :-/

@handrews
Copy link
Member

handrews commented Jan 9, 2019

@vearutop if you have any more of these in the works, please let me know- as I noted, it's great that you're working on this, and you may get things done faster than me. I'd just like to avoid duplication.

@vearutop
Copy link
Contributor Author

vearutop commented Jan 9, 2019

I did this refactoring in just couple of hours, more like a demo of alternative approach. I don't mind to withdraw my PR or rebase it on anything else.

I don't have any other ideas to apply to this schema so far. :)

@handrews
Copy link
Member

@vearutop oh, cool, glad this was a quick thing for you- I'll take a look in more detail. I'm more than happy to recommend merging yours if it works. I am not possessive here :-)

@handrews
Copy link
Member

@vearutop @webron embarrassingly since commenting above I've been plagued by a series of minor illnesses and never quite getting caught up enough to attend to the OAS schema. :-( I haven't given up, but I'm currently coming down with my 4th cold in 2.5 months and I'm not sure when I'll be back to full strength and caught up with all of my projects.

Copy link
Member

@handrews handrews left a comment

Choose a reason for hiding this comment

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

This is largely the same as what I have had mostly-done in my working copy for quite a while, but been unable to finish and test.

I don't see any way for me to get back to working on this schema, as I told the TSC recently. JSON Schema's next draft has been stalled for 5 months, and I need to focus on that (instead of staring at both projects, feeling overwhelmed, and then not working on either).

So contrary to my prior comment, I'm officially giving up :-( At least until the next JSON Schema draft is published, and then I probably want to stop thinking about schemas for a bit :-P

However, this is great work and I hope other folks can follow up on it.

Thanks @vearutop for moving this forward, and I apologize for my initial reaction.

schemas/v3.0/schema.yaml Outdated Show resolved Hide resolved
@MikeRalphson
Copy link
Member

@vearutop could you look at breaking this PR down into smaller commits? The change I've so far spotted problems with is with folding the $ref object definition into surrounding objects and removing the oneOfs. If you look specifically at the response object, you seem to have made $ref a pattern property of the main object, but the constraint that description is required is still present. This leads my test suite to fail.

@vearutop
Copy link
Contributor Author

vearutop commented Mar 8, 2019

@MikeRalphson sure, I can do that. Could you share few schemas that are failing validation if they are in public domain?

@MikeRalphson
Copy link
Member

Sure, all of these point into https://github.com/APIs-guru/openapi-directory

../openapi-directory/APIs/6-dot-authentiqio.appspot.com/6/swagger.yaml:
../openapi-directory/APIs/adafruit.com/2.0.0/swagger.yaml:
../openapi-directory/APIs/agco-ats.com/v1/swagger.yaml:
../openapi-directory/APIs/aiception.com/1.0.0/swagger.yaml:
../openapi-directory/APIs/amazonaws.com/AWSMigrationHub/2017-05-31/swagger.yaml:
../openapi-directory/APIs/amazonaws.com/acm-pca/2017-08-22/swagger.yaml:

@vearutop
Copy link
Contributor Author

vearutop commented Mar 8, 2019

@MikeRalphson indeed I underestimated the effect of embedding references. Having $ref in patternProperties of respective schema affects required and oneOf/allOf. Now I think the benefits of having embedded refs are not enough so I reverted that commit.

Please check again.

@MikeRalphson
Copy link
Member

No other regressions detected across the APIs-guru collection. Good job!

@webron
Copy link
Member

webron commented Mar 20, 2019

@vearutop we're having a meeting dedicated to the JSON Schema tomorrow. Feel free to join us if you want. Details are at https://openapi.groups.io/g/tsc/viewevent?repeatid=3837&eventid=428242&calstart=2019-03-21.

@darrelmiller
Copy link
Member

Summary of TSC discussion

  • We would like to revert the references to schema objects to use the oneOf syntax as referenced above in the "Embed Reference" comment.

@vearutop
Copy link
Contributor Author

@darrelmiller done, please check.

@webron
Copy link
Member

webron commented Mar 25, 2019

Thanks for all the work, @vearutop! You've definitely managed to elegantly reduce the size of the JSON Schema without compromising validation. Top-notch work!

@OAI/tsc - if I can get one or two more approvals, I'll merge this and make the final additions to my PR.

@webron webron merged commit c094c22 into OAI:oas3-schema Mar 26, 2019
@vearutop vearutop deleted the oas3-schema branch March 26, 2019 19:44
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.

5 participants