Fully implement $ref compliance with draft 4 #210

araines · 2015-12-25T23:01:47Z

This makes public interface breaking changes - should consider bumping the library major version when taking this change

I tried to make these changes without making too many changes to the existing library, but unfortunately the end result is more changes than I'd really like (including changing some of the unit tests). However, we now have (local) $ref compatibility and it passes all the tests provided for JSON Schema Draft 4 $refs.

https://github.com/justinrainbow/json-schema/pull/210/files#diff-c1e4a623a503c7f9de75304aae24ae77L21

The major changes here are:

We can do away with the pesky maximum depth, as now we use object references to achieve recursion where necessary within the schema
The JSON Pointer resolution has been abstracted in to its own class, with hopefully a clearer and cleaner implementation
"definitions" is now resolved, although have not been formally fully tested, it is certainly more functional than it once was

I've tried to be respectful to the original library, but appreciate my changes may not be to everyone's taste - comments and improvements are obviously very welcome.

bighappyface · 2016-01-06T14:48:49Z

@araines thank you SO MUCH for tackling this, and thanks for your patience. Please rebase this PR on the latest master and squash your commits down. The travis build should be fixed now and it will be good to see this PR pass so that we can focus in on review.

compliance. This makes public interface breaking changes. Change RefResolver so that we can actually resolve recursive references properly Get most of the draft4 tests working (only 2 errors now) Create a better pointer resolver Now refs are draft4 compliant, getting unit tests to pass again for refresolver Fix up UriResolver unit tests Sort out UriRetriever unit test issues Make sure to set the id property too

araines · 2016-01-09T10:33:46Z

@bighappyface rebased and squashed

fgambino · 2016-01-14T04:13:03Z

Awesome, thanks for this.

mathroc · 2016-01-14T18:58:51Z

src/JsonSchema/Pointer.php

would be nice to add $json->id (if present) here

mathroc · 2016-01-14T19:11:51Z

Looks very nice. Do you think support for $ref in JSON Hyper-Schema could be added as well ?

I think those are the only possibilities added by JSON Hyper-Schema:

{
    "links" : [
        {
            "schema": {
                "$ref": "/a/schema"
            },
            "targetSchema": {
                "$ref": "/another/schema"
            }
        }
    ]
}

mathroc · 2016-01-20T12:39:32Z

Hi,

I tried to support the json hyper schema. it seems to be enough to add this :

    public function resolve($schema, $sourceUri = null)
    {
        // [snip]

        // Resolve links
        $this->resolveLinks($schema, $scope);

        // Resolve $ref
        $this->resolveRef($schema, $scope);

        // Pop back out of our scope
        array_pop($this->scopes);
    }

    /**
     * Look for the links property in the object. If found, look for schema
     * and targetSchema and resolves them.
     *
     * @param object $schema    JSON Schema to flesh out
     * @param string $sourceUri URI where this schema was located
     */
    public function resolveLinks($schema, $sourceUri)
    {
        if (! isset($schema->links)) {
            return;
        }

        foreach ($schema->links as $link) {
            $this->resolveProperty($link, 'schema', $sourceUri);
            $this->resolveProperty($link, 'targetSchema', $sourceUri);
        }
    }

@araines do you think you can add this in your PR ?

mathroc · 2016-01-20T12:47:46Z

and another topic, I have a problem resolving this schema:

{
  "type":"object",
  "definitions": {
    "schema": {
      "$ref": "http://json-schema.org/draft-04/hyper-schema"
    }
  },
  "properties": {
    "schema": {
      "$ref": "#/definitions/schema"
    }
  },
  "required": ["schema"]
}

here is an extract of the stacktrace :

Fatal error: Maximum function nesting level of '256' reached, aborting! in /scripts/vendor/justinrainbow/json-schema/src/JsonSchema/Uri/UriResolver.php on line 143

Call Stack:
    0.0007     462216   1. {main}() /scripts/vendor/justinrainbow/json-schema/bin/validate-json:0
    0.0041     677568   2. JsonSchema\RefResolver->resolve() /scripts/vendor/justinrainbow/json-schema/bin/validate-json:218
    0.6655     798864   3. JsonSchema\RefResolver->resolveObjectOfSchemas() /scripts/vendor/justinrainbow/json-schema/src/JsonSchema/RefResolver.php:147
    0.6655     799176   4. JsonSchema\RefResolver->resolve() /scripts/vendor/justinrainbow/json-schema/src/JsonSchema/RefResolver.php:219
    0.6655     799400   5. JsonSchema\RefResolver->resolveRef() /scripts/vendor/justinrainbow/json-schema/src/JsonSchema/RefResolver.php:154
    0.6656     799448   6. JsonSchema\RefResolver->fetchRef() /scripts/vendor/justinrainbow/json-schema/src/JsonSchema/RefResolver.php:277
    0.6657     799968   7. JsonSchema\RefResolver->resolve() /scripts/vendor/justinrainbow/json-schema/src/JsonSchema/RefResolver.php:75
    0.6657     800816   8. JsonSchema\RefResolver->resolveObjectOfSchemas() /scripts/vendor/justinrainbow/json-schema/src/JsonSchema/RefResolver.php:147
    0.6658     804280  13. JsonSchema\RefResolver->resolve() /scripts/vendor/justinrainbow/json-schema/src/JsonSchema/RefResolver.php:219
    0.6659     805128  14. JsonSchema\RefResolver->resolveProperty() /scripts/vendor/justinrainbow/json-schema/src/JsonSchema/RefResolver.php:135
    0.6659     805128  15. JsonSchema\RefResolver->resolve() /scripts/vendor/justinrainbow/json-schema/src/JsonSchema/RefResolver.php:237
    0.6659     805872  16. JsonSchema\RefResolver->resolveObjectOfSchemas() /scripts/vendor/justinrainbow/json-schema/src/JsonSchema/RefResolver.php:147
[snip]
    0.6720    1082456 253. JsonSchema\RefResolver->resolve() /scripts/vendor/justinrainbow/json-schema/src/JsonSchema/RefResolver.php:219
    0.6720    1082456 254. JsonSchema\RefResolver->getResolutionScope() /scripts/vendor/justinrainbow/json-schema/src/JsonSchema/RefResolver.php:130
    0.6720    1082552 255. JsonSchema\Uri\UriResolver->resolve() /scripts/vendor/justinrainbow/json-schema/src/JsonSchema/RefResolver.php:177
    0.6720    1082552 256. JsonSchema\Uri\UriResolver->parse() /scripts/vendor/justinrainbow/json-schema/src/JsonSchema/Uri/UriResolver.php:37

but it works with this one :

{
  "type":"object",
  "properties": {
    "schema": {
      "$ref": "http://json-schema.org/draft-04/hyper-schema"
    }
  },
  "required": ["schema"]
}

not sure where the problem comes from

Markard · 2016-01-25T14:14:11Z

src/JsonSchema/RefResolver.php

I guess you should resolveRef before other resolutions. Before 134 line.

The reason for doing it afterward is to ensure we don't get stuck in an infinite loop whilst attempting to dereference everything.

webmozart · 2016-02-01T14:14:43Z

What's the state of this?

bighappyface · 2016-02-01T14:22:50Z

@webmozart good question. Would you mind reviewing the PR and feedback from @Markard and please post your thoughts?

Once the PR gains a couple of +1s I can proceed with the merge and bump.

webmozart · 2016-02-01T14:24:43Z

src/JsonSchema/Pointer.php

Is there any test case for this class?

Very good point, in putting the tests together I've identified a number of bugs!

Cool :) I think it's a bit weird that this code lives in a class named Pointer. Objects of this class do not actually represent a pointer (e.g. /data/2), but the JSON data. Also, I don't understand why this class stores any state ($json).

In my opinion, this should be a stateless service class with a method resolvePointer($json, $pointer). Following this thought, I think that this code could be just as well merged back into UriRetriever.

Alternatively, we could call this PointerResolver with a single method resolvePointer($json, $pointer). I'd actually prefer this since the URI retriever's responsibility is loading schemas, not dealing with pointers.

As you identified - the intention of this class was to keep all pointer resolution code encapsulated in one place whose only responsibility is to resolve pointers. My choice of naming is definitely not ideal, in retrospect!

The other part of the thinking was to have the JSON document as part of the state of the class as that allows to resolve multiple pointers from a single document, but there clearly isn't a need for that, and it does make for a slightly confusing usage pattern. I'll rename and make the changes as you've described.

araines · 2016-02-01T14:31:18Z

Sorry guys, been v busy recently and haven't had a chance to come back to comment on and fix up review comments. I'll try to find some time over the next few days to go through everything and tidy up.

bighappyface · 2016-02-01T14:48:08Z

@araines no worries. @Markard and @webmozart have been super helpful and I think we can get this one across the finish line soon.

bighappyface · 2016-02-02T13:24:27Z

@webmozart thanks for your feedback. As this is a community-driven project, all PRs are welcome, so the various recommendations you have would be best conveyed as individual contributions as there is no official "maintainer" to put it on any given roadmap (I just happen to have push access to this repo).

Regarding those various recommendations, I think it is good to focus on one iteration at a time. Not every PR need be a masterpiece. Provided that we bump to 2.0.0 with the merge of this PR (which is the tentative plan) we can then iterate further based on your recommendations and those from others. This will help folks like @araines have their contributions accepted in timely fashions and a keep this package progressing.

A lot of people have been asking for better $ref handling for awhile and this work gets us distinctly closer, so I want to work with all of us to get to a happier place, and reviews like this go a long way toward that goal. That being said, what are the minimum additional changes to this PR that you would like to see to obtain your "+1" so that we can move forward?

webmozart · 2016-02-02T13:27:58Z

@bighappyface Yes, I was mainly dropping my thoughts here not to lose them. :) Let's keep this PR focused!

araines · 2016-02-02T13:31:39Z

I've added changes to aid with code clarity which were directly related to the changes I've made, but as already discussed, I've left the rest for another PR :)

webmozart · 2016-02-02T13:36:47Z

src/JsonSchema/PointerResolver.php

I think it would be better to pass this pointer to resolve() instead of storing it. We don't really need to store this for any other reason but showing an exception message.

bighappyface · 2016-02-17T15:32:42Z

@araines @webmozart @mathroc

Hey folks, are we good on this or are there still pending changes?

I'm going to get caught up on this soon but want to ask if we know anything is outstanding before I dive in once again.

bighappyface · 2016-02-17T15:33:17Z

@rtucek would you mind reviewing this PR as a return favor to your recently accepted work in #232 ?

mathroc · 2016-02-17T15:56:54Z

I'm ok with the state of this PR. but I'd suggest not to make a new release immediately after merging. It could give some time to make some BC improvement as suggested by @webmozart
@webmozart woudl you mind creating a few issue based on your earlier comments ? (maybe they could be put into a 2.0 roadmap). I'll be happy to work on some of them

rtucek · 2016-02-18T14:03:54Z

@bighappyface yeah, sure no problem.
But this PR is huge and it takes some time to pick the pieces. I'll look into this during weekend.

webmozart · 2016-02-18T17:16:15Z

I would rather see this PR finished instead of merged + fixes later on. Otherwise you run the risk of publishing undesired code with the next release and having to maintain BC for that code.

bighappyface · 2016-02-18T18:59:08Z

@rtucek thank you for the help!

@webmozart I would like to see this PR finished as well.

araines · 2016-02-18T19:07:49Z

It looks like there are still some comments here to be addressed. I'm happy to do so, but I'm afraid my time gets torn between a lot of different things! I'll try to get on to some of the remaining comments as soon as I can.

Assuming everyone is happy with taking on the few remaining comments from @webmozart which involve making further BC breaking changes (although ultimately for the better I believe), then I should be able to get them sorted out in the next few days.

TerjeBr · 2016-03-20T23:10:00Z

Have a look at the test file I added in this commit e3f8e11

I will be working to rectify these errors.

araines · 2016-03-31T09:00:58Z

I'm at a bit of a loss at the moment as to why these tests are failing - I've got them passing on my local machine. Will have to try to understand what is going on better, then get these issues resolved.

TerjeBr · 2016-04-02T23:42:39Z

The tests that are failing is from vendor/json-schema/JSON-Schema-Test-Suite/tests/draft4/ref.json
This is the new test file that was included.

It is the test at lines 129-142 that fails.

The test tries to validate against the schema at http://json-schema.org/draft-04/schema#
and this fails because of a bug in JsonSchema/Constraints/ObjectConstraint.php at line163

jojo1981 · 2016-04-14T18:20:23Z

@araines I think this PR can be closed?

bighappyface · 2016-04-14T20:22:33Z

Closed with #174

bighappyface added Needs Review Needs rebase on target branch labels Jan 6, 2016

araines force-pushed the master branch 2 times, most recently from baff14a to 63da0ec Compare January 9, 2016 10:21

araines force-pushed the master branch from 63da0ec to 0a0e8c7 Compare January 9, 2016 10:30

mathroc mentioned this pull request Jan 14, 2016

improve RefResolver #216

Closed

bighappyface removed the Needs rebase on target branch label Jan 14, 2016

mathroc reviewed Jan 14, 2016
View reviewed changes

bighappyface mentioned this pull request Jan 20, 2016

Best way of using definitions in another file? #214

Closed

Markard reviewed Jan 25, 2016
View reviewed changes

bighappyface mentioned this pull request Feb 1, 2016

Fixed references to schemas that contain relative references #225

Closed

webmozart reviewed Feb 1, 2016
View reviewed changes

Add testing for Pointer class (and fix some bugs)

23b9e9c

araines force-pushed the master branch from eaf55a9 to 23b9e9c Compare February 1, 2016 22:24

Renaming of Pointer class to PointerResolver

079d2ba

Clarify scope entering/leaving by better named methods

4708e0c

webmozart reviewed Feb 2, 2016
View reviewed changes

This was referenced Feb 8, 2016

Issue with references in schema files #228

Closed

Update from 1.4.4 to 1.5.0 multiple issues #188

Closed

TerjeBr mentioned this pull request Mar 20, 2016

Implement $ref compliance #243

Closed

bighappyface mentioned this pull request Mar 23, 2016

New package owner #244

Closed

Further tidy up following code reviews

6df6a8f

araines force-pushed the master branch from 64ce9d1 to 6df6a8f Compare March 31, 2016 08:50

araines mentioned this pull request Apr 3, 2016

#240 Fix RefResolver and make it compatible with draft-04 #245

Merged

araines force-pushed the master branch from e3dab43 to 6df6a8f Compare April 5, 2016 18:12

Merge remote-tracking branch 'upstream/master'

b3ac9a6

bighappyface closed this Apr 14, 2016

Fully implement $ref compliance with draft 4 #210

Fully implement $ref compliance with draft 4 #210

Uh oh!

Conversation

araines commented Dec 25, 2015

Uh oh!

bighappyface commented Jan 6, 2016

Uh oh!

araines commented Jan 9, 2016

Uh oh!

fgambino commented Jan 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathroc commented Jan 14, 2016

Uh oh!

mathroc commented Jan 20, 2016

Uh oh!

mathroc commented Jan 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webmozart commented Feb 1, 2016

Uh oh!

bighappyface commented Feb 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

araines commented Feb 1, 2016

Uh oh!

bighappyface commented Feb 1, 2016

Uh oh!

bighappyface commented Feb 2, 2016

Uh oh!

webmozart commented Feb 2, 2016

Uh oh!

araines commented Feb 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bighappyface commented Feb 17, 2016

Uh oh!

bighappyface commented Feb 17, 2016

Uh oh!

mathroc commented Feb 17, 2016

Uh oh!

rtucek commented Feb 18, 2016

Uh oh!

webmozart commented Feb 18, 2016

Uh oh!

bighappyface commented Feb 18, 2016

Uh oh!

araines commented Feb 18, 2016

Uh oh!

TerjeBr commented Mar 20, 2016

Uh oh!

araines commented Mar 31, 2016

Uh oh!

TerjeBr commented Apr 2, 2016

Uh oh!

jojo1981 commented Apr 14, 2016

Uh oh!

bighappyface commented Apr 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects