Skip to content

Add tests for refs with relative uris #457

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 1, 2021
Merged

Add tests for refs with relative uris #457

merged 2 commits into from
Jun 1, 2021

Conversation

amosonn
Copy link
Contributor

@amosonn amosonn commented Mar 4, 2021

No description provided.

@amosonn
Copy link
Contributor Author

amosonn commented Mar 4, 2021

I could not find a way to port this to older versions, as they seem not to allow definitions in nested schemas. Is there a way to write a test with multiple, separate schemas?

@amosonn amosonn mentioned this pull request Mar 4, 2021
Merged
@amosonn amosonn force-pushed the patch-1 branch 3 times, most recently from 5d07dbe to 0eeed11 Compare March 4, 2021 05:50
@gregsdennis
Copy link
Member

@amosonn take a look at the remotes folder. External schemas are stored here and can be referenced from the tests.

@amosonn amosonn force-pushed the patch-1 branch 2 times, most recently from f435fc9 to f883394 Compare March 4, 2021 16:43
@amosonn
Copy link
Contributor Author

amosonn commented Mar 4, 2021

Added these tests, and also some for dynamicRef.

jdesrosiers
jdesrosiers requested changes Mar 4, 2021
View reviewed changes
Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

You can leave out the tests for $recursiveRef and $dynamicRef. $recursiveRef doesn't support any value other than an empty fragment ("#"), so that doesn't apply to what is being tested. $dynamicRef does support non-fragment-only URIs, but how that works is a little complicated. I'll be writing tests for $dynamicRef next week, and I'll make sure relative URI resolution is covered.

I've only skimmed this so far, but I'd be really surprised if relative URI resolution isn't covered somewhere already. I'll take a closer look later.

@amosonn
Copy link
Contributor Author

amosonn commented Mar 5, 2021

There are tests for relative uri resolution, I think, but not for relative uri resolution followed by $defs resolution. I got around to write these cases because they highlighted a bug in the client I'm using which wasn't covered by other tests from the suite, see json-everything/json-everything#79 .

@amosonn amosonn force-pushed the patch-1 branch 2 times, most recently from 7796a24 to 1b7a4d6 Compare March 5, 2021 01:22
jdesrosiers
jdesrosiers requested changes Mar 5, 2021
View reviewed changes
Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

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

I looked closer at the tests and they look fine except for some use of relative URIs for $ids at the top level of schemas.

I'm not sure "ref.json" is the right place for these tests. All other tests regarding embedding schemas with $id are in "refRemote.json". These should probably be there as well. I don't feel strongly about it tho if others disagree.

@amosonn
Copy link
Contributor Author

amosonn commented Mar 13, 2021 •
edited
Loading

Summary:

  • I changed all the relative top-level $id-s to be absolute. I still think this is worth revisiting, but I agree that not in this PR.
  • I removed the $dynamicRef tests from this PR completely, as that discussion was getting long. Once this lands, I'll open a new one where we can discuss solely those tests.

Anything else?

@jdesrosiers jdesrosiers mentioned this pull request Mar 15, 2021
Closed
jdesrosiers
jdesrosiers reviewed Mar 15, 2021
View reviewed changes
remotes/ref-and-defs.json
@@ -0,0 +1,11 @@
{
"$id": "http://localhost:1234/ref-and-defs.json",
Copy link
Member

Choose a reason for hiding this comment

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

When we talked about $ids being required to be absolute URIs, I said,

Same goes for the $ids in the "remotes" schemas.

It occurred to me that that's not true. The "remotes" are designed as if they are fetched from http://localhost:1234/{path-to-schema}. Therefore, they have a retrieval URI and don't need an absolute URI. That's why none of them have $id and that's not a problem. So, $ids (or even relative $ids) are fine in the "remotes" schemas, but they aren't necessary. I'm ok with leaving them in (even tho they are redundant) or taking them out.

@jdesrosiers
Copy link
Member

The "Test Suite Sanity Checking" build failure appears to be wrong. I think this is safe to merge.

@jdesrosiers jdesrosiers merged commit 8a89f58 into json-schema-org:master Jun 1, 2021
@amosonn amosonn deleted the patch-1 branch June 1, 2021 22:13
davishmcclurg added a commit to davishmcclurg/json_schemer that referenced this pull request Oct 21, 2021
@davishmcclurg
Evaluate ref pointers relative to parent
a8f8d62 
Previously, this was always evalutating pointers using the root schema.
We actually want to evaluate them using the nearest parent schema with
an `$id`, which we look up here using `parent_uri`.

Addresses failures from: json-schema-org/JSON-Schema-Test-Suite#457
davishmcclurg added a commit to davishmcclurg/json_schemer that referenced this pull request Apr 22, 2022
@davishmcclurg
Evaluate ref pointers relative to parent
9f5148a 
Previously, this was always evalutating pointers using the root schema.
We actually want to evaluate them using the nearest parent schema with
an `$id`, which we look up here using `parent_uri`.

Addresses failures from: json-schema-org/JSON-Schema-Test-Suite#457
davishmcclurg added a commit to davishmcclurg/json_schemer that referenced this pull request May 15, 2023
@davishmcclurg
Evaluate ref pointers relative to parent
e019baf 
Previously, this was always evalutating pointers using the root schema.
We actually want to evaluate them using the nearest parent schema with
an `$id`, which we look up here using `parent_uri`.

Addresses failures from: json-schema-org/JSON-Schema-Test-Suite#457
davishmcclurg added a commit to davishmcclurg/json_schemer that referenced this pull request May 16, 2023
@davishmcclurg
Evaluate ref pointers relative to parent
139f8ae 
Previously, this was always evalutating pointers using the root schema.
We actually want to evaluate them using the nearest parent schema with
an `$id`, which we look up here using `parent_uri`.

Addresses failures from: json-schema-org/JSON-Schema-Test-Suite#457
davishmcclurg added a commit to davishmcclurg/json_schemer that referenced this pull request May 16, 2023
@davishmcclurg
Evaluate ref pointers relative to parent
0b332dc 
Previously, this was always evalutating pointers using the root schema.
We actually want to evaluate them using the nearest parent schema with
an `$id`, which we look up here using `parent_uri`.

Addresses failures from: json-schema-org/JSON-Schema-Test-Suite#457
davishmcclurg added a commit to davishmcclurg/json_schemer that referenced this pull request May 22, 2023
@davishmcclurg
Rework ref deferencing
abe660c 
This is an attempt to simplify dereferencing refs and address issues
exposed by new json-schema-test-suite tests. The main thing here is
giving schema objects a `base_uri` that's used when resolving ids (and
as the initial base URI for instances). Child schemas use the `ref_uri`
that was used to resolve them as their `base_uri` in order to get nested
refs and ids to resolve properly.

More details about specific issues:

- `$id` is ignored when `$ref` is present (`instance.base_uri` is
  updated after `validate_ref`), because `$ref` isn't supposed to take
  sibling `$id` values into account. `@base_uri` handles nested refs.
  Exposed by: json-schema-org/JSON-Schema-Test-Suite#493

- JSON pointers are evaluated relative to the ref URI. Previously,
  pointers were always evaluated using the root schema. Now they're
  evaluated relative to the schema with a matching `$id` (usually
  nearest parent with an `$id`; or specific id (see below); default is
  root).
  Exposed by: json-schema-org/JSON-Schema-Test-Suite#457

- JSON pointers are evaluated for id refs. This allows a ref to look up
  a schema by `$id` and then apply a JSON pointer to use a subschema.
  This uses the same logic as above. The important part is removing the
  fragment from `ref_uri` if it's a JSON pointer so that the lookup in
  `ids` works properly. The fragment is kept if it's not a JSON pointer
  to support location-independent ids.
  Exposed by: json-schema-org/JSON-Schema-Test-Suite#578

- JSON pointer refs are always joined with the base URI. I [started
  handling them][0] separately because of an [issue][1] with invalid
  URIs. But now I think that was incorrect and that fragment pointers
  need to be encoded properly for URIs. The [specification says][2]:

  > In all cases, dereferencing a "$ref" reference involves first
  > resolving its value as a URI reference against the current base URI.

- Empty fragments are removed in `join_uri` to have consistent URIs to
  lookup in `ids`. Meta schemas, for example, have empty fragments in
  their top-level ids (eg, `http://json-schema.org/draft-07/schema#`)
  and removing the JSON pointer fragments causes them not to be found.

[0]: b91115e
[1]: #54
[2]: https://datatracker.ietf.org/doc/html/draft-handrews-json-schema-01#section-8.3.2
davishmcclurg added a commit to davishmcclurg/json_schemer that referenced this pull request May 26, 2023
@davishmcclurg
Rework ref deferencing
923c825 
This is an attempt to simplify dereferencing refs and address issues
exposed by new json-schema-test-suite tests. The main thing here is
giving schema objects a `base_uri` that's used when resolving ids (and
as the initial base URI for instances). Child schemas use the `ref_uri`
that was used to resolve them as their `base_uri` in order to get nested
refs and ids to resolve properly.

More details about specific issues:

- `$id` is ignored when `$ref` is present (`instance.base_uri` is
  updated after `validate_ref`), because `$ref` isn't supposed to take
  sibling `$id` values into account. `@base_uri` handles nested refs.
  Exposed by: json-schema-org/JSON-Schema-Test-Suite#493

- JSON pointers are evaluated relative to the ref URI. Previously,
  pointers were always evaluated using the root schema. Now they're
  evaluated relative to the schema with a matching `$id` (usually
  nearest parent with an `$id`; or specific id (see below); default is
  root).
  Exposed by: json-schema-org/JSON-Schema-Test-Suite#457

- JSON pointers are evaluated for id refs. This allows a ref to look up
  a schema by `$id` and then apply a JSON pointer to use a subschema.
  This uses the same logic as above. The important part is removing the
  fragment from `ref_uri` if it's a JSON pointer so that the lookup in
  `ids` works properly. The fragment is kept if it's not a JSON pointer
  to support location-independent ids.
  Exposed by: json-schema-org/JSON-Schema-Test-Suite#578

- JSON pointer refs are always joined with the base URI. I [started
  handling them][0] separately because of an [issue][1] with invalid
  URIs. But now I think that was incorrect and that fragment pointers
  need to be encoded properly for URIs. The [specification says][2]:

  > In all cases, dereferencing a "$ref" reference involves first
  > resolving its value as a URI reference against the current base URI.

- Empty fragments are removed in `join_uri` to have consistent URIs to
  lookup in `ids`. Meta schemas, for example, have empty fragments in
  their top-level ids (eg, `http://json-schema.org/draft-07/schema#`)
  and removing the JSON pointer fragments causes them not to be found.

[0]: b91115e
[1]: #54
[2]: https://datatracker.ietf.org/doc/html/draft-handrews-json-schema-01#section-8.3.2
davishmcclurg added a commit to davishmcclurg/json_schemer that referenced this pull request May 26, 2023
@davishmcclurg
Rework ref deferencing
ea446f7 
This is an attempt to simplify dereferencing refs and address issues
exposed by new json-schema-test-suite tests. The main thing here is
giving schema objects a `base_uri` that's used when resolving ids (and
as the initial base URI for instances). Child schemas use the `ref_uri`
that was used to resolve them as their `base_uri` in order to get nested
refs and ids to resolve properly.

More details about specific issues:

- `$id` is ignored when `$ref` is present (`instance.base_uri` is
  updated after `validate_ref`), because `$ref` isn't supposed to take
  sibling `$id` values into account. `@base_uri` handles nested refs.
  Exposed by: json-schema-org/JSON-Schema-Test-Suite#493

- JSON pointers are evaluated relative to the ref URI. Previously,
  pointers were always evaluated using the root schema. Now they're
  evaluated relative to the schema with a matching `$id` (usually
  nearest parent with an `$id`; or specific id (see below); default is
  root).
  Exposed by: json-schema-org/JSON-Schema-Test-Suite#457

- JSON pointers are evaluated for id refs. This allows a ref to look up
  a schema by `$id` and then apply a JSON pointer to use a subschema.
  This uses the same logic as above. The important part is removing the
  fragment from `ref_uri` if it's a JSON pointer so that the lookup in
  `ids` works properly. The fragment is kept if it's not a JSON pointer
  to support location-independent ids.
  Exposed by: json-schema-org/JSON-Schema-Test-Suite#578

- JSON pointer refs are always joined with the base URI. I [started
  handling them][0] separately because of an [issue][1] with invalid
  URIs. But now I think that was incorrect and that fragment pointers
  need to be encoded properly for URIs. The [specification says][2]:

  > In all cases, dereferencing a "$ref" reference involves first
  > resolving its value as a URI reference against the current base URI.

- Empty fragments are removed in `join_uri` to have consistent URIs to
  lookup in `ids`. Meta schemas, for example, have empty fragments in
  their top-level ids (eg, `http://json-schema.org/draft-07/schema#`)
  and removing the JSON pointer fragments causes them not to be found.

[0]: b91115e
[1]: #54
[2]: https://datatracker.ietf.org/doc/html/draft-handrews-json-schema-01#section-8.3.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karenetheridge karenetheridge karenetheridge left review comments

@jdesrosiers jdesrosiers jdesrosiers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amosonn @gregsdennis @jdesrosiers @karenetheridge