Skip to content

Changing the resolution base via 'id' does not work properly #447

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
Closed
erayd opened this issue Sep 21, 2017 · 15 comments
Closed

Changing the resolution base via 'id' does not work properly #447

erayd opened this issue Sep 21, 2017 · 15 comments

Comments

@erayd
Copy link
Contributor

erayd commented Sep 21, 2017 •
edited
Loading

Per issue #446. See below test case:

$data = json_decode(<<<'eot'
{
    "propertyOne": "blue"
}
eot
);

$base = json_decode(<<<'eot'
{
    "id": "schema.json",
    "properties": {
        "propertyOne": {
            "$ref": "propertyOne.json"
        }
    }
}
eot
);

$validator = new \JsonSchema\Validator();
var_dump($validator->validate($data, $base));

@bighappyface Could you please mark this as blocking for 6.0.0? I need to stare at the code some more, but at first look this may require significant refactoring of the $ref code.
@erayd erayd mentioned this issue Sep 21, 2017
Closed
@erayd
Copy link
Contributor Author

erayd commented Sep 21, 2017

@shmax Have you had anything to do with with the ref processing when the scope has been changed? Some insight into how this is intended to work would be handy and would save a lot of time tracing stuff. No worries if not.

The tests in draft4/refRemote.json all pass, including the ones with a nested scope change (e.g. folderInteger.json), but the logic that actually achieves this isn't clear, and it's not covering all cases. In the test code above, the resolution base gets completely dropped somewhere along the line, despite being explicitly set in the base schema.

@shmax
Copy link
Collaborator

shmax commented Sep 21, 2017

No, not only was that before my time, but I never really totally figured out how to get $ref functionality working in my own code; I had to resort to overriding the UriResolver and just cheeseballing the resolve method. I looked over this issue, but I think I'm a bit out of my depth; I don"t even know what a "behat" is. I'll see if I can get caught up a bit more this weekend.

@erayd
Copy link
Contributor Author

erayd commented Sep 21, 2017

Bugger. Will get to tracing stuff then. Thanks anyway :-).

@erayd
Copy link
Contributor Author

erayd commented Sep 21, 2017

For what it's worth, this bug has nothing to do with Behat. Behat just happens to depend on this validator, and @dkarlovi happened to find the bug while using Behat.

@shmax
Copy link
Collaborator

shmax commented Sep 21, 2017

Gotcha. I'll load up his sample app tomorrow and see if I can make any sense of it.

@erayd
Copy link
Contributor Author

erayd commented Sep 21, 2017

No need, unless you're curious - the test case at the top of this thread reproduces the bug using only the json-schema validator.

@dkarlovi
Copy link
Contributor

Behat is irrelevant here, I used it to replicate the bug the only way I knew how. :)

@mdeboer
Copy link

mdeboer commented Sep 27, 2017

Same problem here and I have no clue how to get it to work except for writing my own resolver I guess. Can't we just add a getter/setter for the base path for the resolver? This would make all relative file references relative to that path.

I can probably cook something up but I'm not sure if it would break stuff as this is a bug apparently.

@erayd
Copy link
Contributor Author

erayd commented Sep 27, 2017

@mdeboer This is definitely a bug. I will fix it.

Regarding your suggestion of a base path setter... what purpose would this serve? The spec already defines the id keyword as filling this role; it's just the implementation which is broken.

@mdeboer
Copy link

mdeboer commented Sep 27, 2017

Alright, awaiting the fix then 👍

erayd added a commit to erayd/json-schema that referenced this issue Sep 30, 2017
@erayd
Add proper recursive handling for $ref resolution base
b7a8f3f 
Fixes jsonrainbow#447

Note that this patch does not check whether a given container is
actually a schema when recursing into it. In most cases this will
not matter, however it does mean that in some edge cases it will
attempt to resolve a `$ref` in a context where ref is actually not
part of the spec. Limiting resolution to schema-context containers
is outside the scope of this patch, but can be added later.
@erayd
Copy link
Contributor Author

erayd commented Sep 30, 2017

@mdeboer See #448. Assuming code review is OK, I will backport this fix to 5.x.x after merge.

erayd added a commit to erayd/json-schema that referenced this issue Sep 30, 2017
@erayd
Add proper recursive handling for $ref resolution base
eea58bd 
Fixes jsonrainbow#447

Note that this patch does not check whether a given container is
actually a schema when recursing into it. In most cases this will
not matter, however it does mean that in some edge cases it will
attempt to resolve a `$ref` in a context where ref is actually not
part of the spec. Limiting resolution to schema-context containers
is outside the scope of this patch, but can be added later.
erayd added a commit to erayd/json-schema that referenced this issue Sep 30, 2017
@erayd
Add proper recursive handling for $ref resolution base
b639049 
Fixes jsonrainbow#447

Note that this patch does not check whether a given container is
actually a schema when recursing into it. In most cases this will
not matter, however it does mean that in some edge cases it will
attempt to resolve a `$ref` in a context where ref is actually not
part of the spec. Limiting resolution to schema-context containers
is outside the scope of this patch, but can be added later.
@dkarlovi
Copy link
Contributor

dkarlovi commented Oct 2, 2017

@erayd is this actually backward compatible, will it even land in 5.x?

@erayd
Copy link
Contributor Author

erayd commented Oct 2, 2017

@dkarlovi Yes. I very deliberately implemented it in a manner that could be backported to 5.x.x without breaking anything.

@dkarlovi
Copy link
Contributor

dkarlovi commented Oct 2, 2017

@erayd great, looking forward to seeing it released, thanks!

erayd added a commit to erayd/json-schema that referenced this issue Oct 2, 2017
@erayd
Add proper recursive handling for $ref resolution base
737facf 
Fixes jsonrainbow#447

Note that this patch does not check whether a given container is
actually a schema when recursing into it. In most cases this will
not matter, however it does mean that in some edge cases it will
attempt to resolve a `$ref` in a context where ref is actually not
part of the spec. Limiting resolution to schema-context containers
is outside the scope of this patch, but can be added later.
bighappyface pushed a commit that referenced this issue Oct 2, 2017
@erayd @bighappyface
Add proper recursive handling for $ref resolution base (#448)
0a6210e 
Fixes #447

Note that this patch does not check whether a given container is
actually a schema when recursing into it. In most cases this will
not matter, however it does mean that in some edge cases it will
attempt to resolve a `$ref` in a context where ref is actually not
part of the spec. Limiting resolution to schema-context containers
is outside the scope of this patch, but can be added later.
erayd added a commit to erayd/json-schema that referenced this issue Oct 3, 2017
@erayd
Add proper recursive handling for $ref resolution base (jsonrainbow#448)
a2cfa75 
Fixes jsonrainbow#447

Note that this patch does not check whether a given container is
actually a schema when recursing into it. In most cases this will
not matter, however it does mean that in some edge cases it will
attempt to resolve a `$ref` in a context where ref is actually not
part of the spec. Limiting resolution to schema-context containers
is outside the scope of this patch, but can be added later.
@erayd erayd mentioned this issue Oct 3, 2017
Merged
@erayd
Copy link
Contributor Author

erayd commented Oct 3, 2017

@dkarlovi Now backported for 5.2.2 - see PR #450.

bighappyface pushed a commit that referenced this issue Oct 3, 2017
@erayd @bighappyface
Backports for 5.2.2 (Part 3) (#450)
b80053b 
* Update php-csfixer rules to address problem in 2.7 & new multiline rule (#449)

* Update php-csfixer rules to address problem in 2.7 & new multiline rule

 * yoda_style in 2.7 is dangerous and may result in logic errors. In
 some cases, it also results in invalid syntax.

 * multiline comments prefixed with // now seem to be misaligned, and
 this cannot be disabled, so have changed the relevant comment.

* PHP-5.3 is not available on trusty, so explicitly specify precise for 5.3

* Add proper recursive handling for $ref resolution base (#448)

Fixes #447

Note that this patch does not check whether a given container is
actually a schema when recursing into it. In most cases this will
not matter, however it does mean that in some edge cases it will
attempt to resolve a `$ref` in a context where ref is actually not
part of the spec. Limiting resolution to schema-context containers
is outside the scope of this patch, but can be added later.
@erayd erayd closed this as completed Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@erayd @dkarlovi @shmax @mdeboer