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

Expand references of DocuemntStore schemas #93

Closed

Conversation

richardlarocque
Copy link
Contributor

This is a fix for #92.

Calls expand_references! on loaded schemas before adding them to the
DocumentStore. This is intended to fix a bug where complex schemas
that include more than one level of cross-file use of $refs are not
being validated correctly.

This commit includes new test cases that reproduce this issue and show
the effectiveness of the proposed fix.

The inspiration for this fix comes from a command included in the
json_schema gem. In that gem, the stand-alone validator command
expands references within schemas before adding them to the store.
It seems reasonable to do the same here.

See https://github.com/brandur/json_schema/blob/20ccb82d7e18140d88ded508edd6c003865c98a0/lib/commands/validate_schema.rb#L92.

This is a fix for thoughtbot#92.

Calls `expand_references!` on loaded schemas before adding them to the
`DocumentStore`.  This is intended to fix a bug where complex schemas
that include more than one level of cross-file use of `$ref`s are not
being validated correctly.

This commit includes new test cases that reproduce this issue and show
the effectiveness of the proposed fix.

The inspiration for this fix comes from a command included in the
`json_schema` gem.  In that gem, the stand-alone validator command
expands references within schemas before adding them to the store.
It seems reasonable to do the same here.

See https://github.com/brandur/json_schema/blob/20ccb82d7e18140d88ded508edd6c003865c98a0/lib/commands/validate_schema.rb#L92.
@hovsater
Copy link
Contributor

Would really like to get this one merged. Anything I can do to help moving the needle?

@seanpdoyle
Copy link
Collaborator

I've appended (to the commit) some MiniTest versions of the RSpec tests you've introduced.

I've merged that commit into master as 3b66b4d.

I'm going to "close" this PR, but rest assured that your contribution has been merged into master and will be a part of the next release.

Thank you for this contribution @richardlarocque.

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