Skip to content

Test property named $ref, containing an actual $ref #392

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

Conversation

ryangalamb
Copy link
Contributor

No description provided.

@karenetheridge
Copy link
Member

this looks similar to #383.

@ryangalamb
Copy link
Contributor Author

@karenetheridge Thanks! Those test cases are quite helpful for what I'm working on.

The bug this PR covers in my implementation is a bit different. (I don't see any $refs as non-keyword property names in the other PR.) I have a function that takes a validation path and figures out the appropriate base URI for that schema location. My first implementation would see /properties/$ref and then try to resolve the schema as a $ref. Since I was only using that function to determine the base URI to resolve a $ref against, nothing in the test suite was exercising that yet.

Since this test case broke my implementation, I figured it was worth sharing the breakage love.

Copy link
Member

@karenetheridge karenetheridge left a comment

Choose a reason for hiding this comment

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

LGTM.

@karenetheridge
Copy link
Member

karenetheridge commented Jun 10, 2020

This is a good test as some implementations do indeed attempt to parse every $ref they see as a jumping point, without looking to see if it is actually a schema, as opposed to simply a property.

two points about this test in particular:

  • it would be more robust by testing "$ref": "...a property value...", as the current test case has the $ref value being an object, which is not syntactically valid for a $ref keyword. The easiest (only?) way to do this with the current schema syntax is to have the $ref be inside an "enum" or "const" keyword.

  • Also, the test(s) should be extended back to every draft where it's relevant (which is all of them, in this case).

@karenetheridge karenetheridge requested review from karenetheridge and a team and removed request for karenetheridge June 10, 2020 15:16
@ryangalamb
Copy link
Contributor Author

The easiest (only?) way to do this with the current schema syntax is to have the $ref be inside an "enum" or "const" keyword.

That'd be good as an additional test, thanks. I'm happy to add it.

For my implementation, that wouldn't have triggered the bug this PR is targeted for. (My specific issue is when "$ref" is on the validation path, and then the validator tries to apply an actual "$ref" keyword. It was messing up the way I determine the base URI to resolve the $ref against.)

Also, the test(s) should be extended back to every draft where it's relevant (which is all of them, in this case).

👍 good point. Thanks!

@ryangalamb
Copy link
Contributor Author

Done backporting the cases!

@Julian
Copy link
Member

Julian commented Jun 11, 2020

Thanks, backport looks good to me, so merging this too -- sounds like possibly there was a follow-up test we were considering? If so probably perfectly good to do that in a follow-up PR if needed.

@Julian Julian merged commit 25598a3 into json-schema-org:master Jun 11, 2020
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.

3 participants