Skip to content

Rel JSON Pointer: add ABNF/examples for index manipulation #1049

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

Fixes #1048

relative-json-pointer =/ non-negative-integer "#"
index-manipulation = ("+" / "-") non-negative-integer
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this definition allows pointers such as 0-0/foo (which would be logically equivalent to 0/foo). I don't feel too strongly one way or another about whether that should be allowed. I'm happy to change it.

@Relequestual Relequestual requested a review from awwright December 3, 2020 13:02
@awwright
Copy link
Member

awwright commented Dec 3, 2020

This looks OK at a glance, I'll test this more thoroughly soon

@karenetheridge
Copy link
Member

Can you write some test cases (that we can add to the test suite for draft2020-12, when it exists) that would cover the changes here?

@Relequestual
Copy link
Member

My understanding is this isn't a spec change but a correction of the ABNF to properly align with the spec.

@ryangalamb
Copy link
Contributor Author

@karenetheridge Is there an official test suite for Relative JSON Pointer? I'm happy to add some test cases to it if so.

Or are you talking about the optional "format" keyword cases in JSON Schema test suite? (Here for 2019-09 cases.) That's a good idea.

Is there a branch/place for 2020-12 cases yet? Or should I wait for that to be made?

@Relequestual
Copy link
Member

I would guess in relation to format.
There's no 2020-12 test suite yet, but if you wanted to put a test here, we can track it at least.

@karenetheridge
Copy link
Member

Right, in 2019-09 we'd put the changes in optional/format/json-pointer.json, but in 2020-12 we'd probably have a json-pointer.json test file right in the main directory that used the format-as-validation vocabulary in its metaschema. So if we're not considering these ABNF changes also effective in 2019-09, there's nowhere to add the tests yet, but I think we'll get something going pretty soon.

@Relequestual
Copy link
Member

I'm not sure we CAN quite do that. We need to have vocabulary specific tests. format as an assertion vocabulary isn't included in any dialect, yet.

@gregsdennis
Copy link
Member

gregsdennis commented Dec 8, 2020

If it's just up to creating a metaschema that includes the assert-format vocab, then we can make one for the purpose of the test. Yeah, it'll also depend on vocabs being supported properly, and ideally you only want to test a single feature... I'm rambing.

If we create a meta-schema for the purpose of supporting the test, then we should be able to test all of the format assertions. The only question is whether to make them optional. I guess it's optional of whether they want to support such a vocab, but I figure if they want to support format assertions, they'd want to test them. That would mean they'd have to support the assert-format vocab, so they shouldn't have a problem supporting a test meta-schema.

Sorry it's late and my brain is fuzzy. The real option is whether they support the assert-format vocab. That means it should go in optional tests.

@Relequestual
Copy link
Member

Right. We would want to have tests for specific vocabularies.
They would be optional in terms of "Do you comply with JSON Schema", but not optional if you're claiming support of that vocabulary.

This is really an aside of the PR I'd like to get merged =D

Copy link
Member

@ssilverman ssilverman left a comment

Choose a reason for hiding this comment

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

👍

@ryangalamb
Copy link
Contributor Author

ryangalamb commented Dec 8, 2020

Sounds good. I'm happy to share the test cases I'm using for my relative JSON pointer implementation when/where there's an appropriate place for it. (The examples I added in this PR come from my test cases.)

I'll share the cases I didn't put in the spec here. If I'm hit by the proverbial bus before I can contribute these properly, please feel free to add them on my behalf 🙂

Please let me know if you disagree with or have questions about any of these.

Success Cases

These are relative to the value "baz" at /foo/0 (the examples I added were relative to /foo/1 so only exercised the - path)

"0"         "bar"  # (This one's silly, but it's a sanity check.)
"0+1"       "baz"
"0+1#"      1

Syntax Errors

None of these are valid Relative JSON Pointers.

"0#/foo"
"0+/foo"
"/foo"

@Relequestual
Copy link
Member

This issue is about to get closed. Want to file a new one on the test suite repo? =]

@Relequestual Relequestual merged commit 5cd32df into json-schema-org:master Dec 8, 2020
@karenetheridge
Copy link
Member

karenetheridge commented Dec 13, 2020

I'm not sure we CAN quite do that. We need to have vocabulary specific tests. format as an assertion vocabulary isn't included in any dialect, yet.

If this is just a fix to the ABNF, not the spec itself, then the additional tests would be valid for 2019-09 as well, so we can add them today to tests/draft2019-09/optional/format/relative-json-pointer.json. (But I don't see anything regarding index manipulation in https://json-schema.org/draft/2019-09/relative-json-pointer.html - so I guess that part is new for 2020-12)

@Relequestual
Copy link
Member

This was a change made after 2019-09: #926
And it appears the ABNF wasn't correct having made that change.

So any test relating to #926 would be for 2020-12.

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.

Rel JSON Pointer: ABNF does not cover the new index manipulation syntax
6 participants