Skip to content
This repository was archived by the owner on Apr 20, 2021. It is now read-only.

Fix $ref resolution problems#155

Closed
araines wants to merge 4 commits intoBehatch:masterfrom
araines:master
Closed

Fix $ref resolution problems#155
araines wants to merge 4 commits intoBehatch:masterfrom
araines:master

Conversation

@araines
Copy link
Copy Markdown
Contributor

@araines araines commented Dec 26, 2015

Following issue #95

In order for the $refs to be resolved correctly within the JSONSchema library, we need to pass a reference to the original JSON object, rather than a copy of its content, when resolving. This is because the library alters the object when it does its dereferencing.

@araines araines changed the title Issue #95: Fix $ref resolution problems Fix $ref resolution problems Dec 26, 2015
@Trogie
Copy link
Copy Markdown

Trogie commented Feb 4, 2016

Works for me!

@araines
Copy link
Copy Markdown
Contributor Author

araines commented Feb 9, 2016

@Trogie : I found that the JSONSchema library was failing silently when it comes to resolution of $ref. i.e. your tests will pass when they should not.

If you take a look at the documented method of dereferencing a schema in the JSON Schema libraries docs, you will see that the contexts here do not set up the library correctly. Without passing the reference to the JSON object, any dereferencing done is not reflected and is lost.

However, if you have a working example schema test failure which requires correct $ref resolution to have taken place, I'd like to see it - I couldn't get this to work without the changes specified here.

@Trogie
Copy link
Copy Markdown

Trogie commented Feb 17, 2016

I ment the patch works for me. :p

@araines
Copy link
Copy Markdown
Contributor Author

araines commented Feb 17, 2016

Ah I completely got the wrong end of the stick! Thanks for checking it out :)

@sanpii
Copy link
Copy Markdown
Member

sanpii commented Feb 19, 2016

Can you add a corresponding behat test scenario?

@araines
Copy link
Copy Markdown
Contributor Author

araines commented Mar 9, 2016

@sanpii I've added the invalid test case - the test cases already existed but were not actually working properly (due to the silent failures I mentioned earlier), which has meant I've had to add additional contexts to prove things out. Not ideal, but given you need the feature tests it was the best way I could think to do it.

Bring up to date with upstream master
@sanpii
Copy link
Copy Markdown
Member

sanpii commented Apr 4, 2016

Rebase & merged. Thank you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants