Skip to content

Conversation

@JanTvrdik
Copy link
Contributor

No description provided.

@stefk
Copy link
Owner

stefk commented Apr 10, 2016

@JanTvrdik, I think we must see this and the ideas in #6 and #11 as a whole and put a little more thought in it. AFAIK the issue isn't about correctness but usability. What's needed is a more convenient API to deal with paths/URIS in general (relative/absolute/not-specified). Implementation may then vary wildly.

I won't have much time in the next days unfortunalely, so here are just quick notes:

  • A solution for Absolute schema URI is required even for local references #6 is a must-have IMO, but it's actually a little trickier than I thought, given we'll also want some kind of cache for resolved pointers.
  • Opening registerSchema could have its uses, but allowing to register relative URIs might be a source of problems (how do we resolve relative URIs within the included schema, if any?) and in some cases, contrary to the spec (we'll just make blind substitutions instead of following the official resolution algorithm)
  • There are probably other things to explore, like adding shortcut methods on the validator for the most common cases (validateFromUri($instance, $schemaUri), validateFromFile($instance, $schemaPath), etc.).

@JanTvrdik JanTvrdik force-pushed the pr/resolver_register_schema_ignores_empty_id branch 2 times, most recently from 675c11c to e2f4f1b Compare April 22, 2016 12:46
@JanTvrdik JanTvrdik force-pushed the pr/resolver_register_schema_ignores_empty_id branch from e2f4f1b to d037430 Compare April 22, 2016 12:59
@JanTvrdik JanTvrdik changed the title Ignore empty primary resource identifier in Resolve::registerSchema Improve Resolver Apr 22, 2016
@JanTvrdik
Copy link
Contributor Author

This should now hopefully solve #6 and #11. #15 is out of scope of this already large PR.

@stefk
Copy link
Owner

stefk commented Apr 24, 2016

Thanks for your work. On the positive side:

On the other hand:

$identifier .= '?'.$this->parts['query'];
}

if ($this->parts['fragment'] !== '' && $this->parts['fragment'][0] !== '/') {
Copy link
Owner

Choose a reason for hiding this comment

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

This isn't right: as per RFC 3986, a fragment denotes a secondary resource; it can't be part of the primary resource identifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I was unaware of the RFC 3986 terminology. The purpose of this is that we need non-JSON-pointer fragments to be used as part of schema identifiers.

@JanTvrdik
Copy link
Contributor Author

JanTvrdik commented Apr 24, 2016

#6 isn't solved yet

It should be. Functionally mentioned in #6 (comment) remained intentionally unimplemented because relative URLs cannot be handled reliably.

The test you've deleted was failing for a good reason.

I disagree. The specification regarding $ref and id is terrible, but the way I understand it, the evaluation process of the scoped-references.json schema should work as follows

  • The whole schema has assigned URI http://localhost:1234/valid/scoped.json
  • First $ref is resolved against current schema URI to
    http://localhost:1234/valid/items-array.json and fetched from disk
  • Second $ref is resolved against current schema URI to
    http://localhost:1234/valid/exclusiveMinimum-not-present.json and fetched from disk
  • New schema is declared with URI resolved to http://localhost:1234/valid/exclusiveMaximum-not-present.json
  • Third $ref is resolved against current schema URI (which was changed!) to
    http://localhost:1234/valid/exclusiveMaximum-not-present.json and fetched from memory

Note: the schema is actually sort-of invalid because content of http://localhost:1234/valid/scoped.json#/properties/baz should be identical to http://localhost:1234/valid/exclusiveMaximum-not-present.json but it not.

@stefk
Copy link
Owner

stefk commented Apr 24, 2016

I don't see anything in the spec to support that reasonning. Scopes have nothing to do with "declaration" of schemas, they're just a way to alter the URI against which references will be resolved. They're also completely indifferent to the way references will be fetched (disk/memory/...). Reading the relevant section of the spec, I think the only possible interpretation is:

  1. The original resolution scope is the URI of the file (file://path/to/scoped-references.json).
  2. The first id creates the scope http://localhost:1234/valid/scoped.json.
  3. References in foo and barare resolved against that scope, which gives:
  4. The id in baz alters the parent scope, making it http://localhost:1234/valid/exclusiveMaximum-not-present.json
  5. The $ref in oneOf is resolved against that scope, leading to http://localhost:1234/valid/exclusiveMaximum-not-present.json#

The expected content of #/baz/oneOf/0is thus the whole schema located at that last URI, i.e.:

{
  "maximum": 2
}

It is the case in the current implementation, and that's what the test is checking.

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.

2 participants