Fix for string invalid scheme error when string contains colon #373
Conversation
|
I checked out the master branch and ran all the tests locally and the same tests are failing. Running the test file I modified yields: |
|
Thanks for your contribution. Sorry about the test failures, those are related to the json schema common test suite and I have a fix for that in progress. |
|
The bug you've fixed is an unfortunate one (I really wish we weren't using errors to determine the type of the data). I'll have to have a think about your proposed fix, I think it may be the correct solution but I'll have to think about possible consequences before we merge |
|
Thanks for getting back to me. I understand this is not the simplest of areas. I have also gone for a simple change to fix this one edge case. It's a shame that the string has to be parsed to see if it is a valid URI or not. |
|
Wouldn't it help a lot if adressable had a validation method which doesn't raise an error but simply returned |
|
That is a bloody good idea! I have raised an issue here: sporkmonger/addressable#257 |
|
@benSlaughter That's ok, it's probably worth waiting until we can do something like that in addressable, and we would also need to fix the other exception case in this method, which is when we try parse the string as a json object. I think this PR is fine. Could you please add an entry to the changelog? I'll try to push up my changes today that fix the failing tests |
|
Yes good point. I have updated changelog. |
|
#377 should fix the failing tests here |
|
@iainbeeston I have another issue, that may be solved in the same way. EDIT: While playing with the unit tests I noticed that '"test"' works, however 'test' will fail. |
|
It's starting to feel like there should be a parent class for all of these errors... Anyway, can you add a test for that? I'm open to your suggestion but I'd really want a test case to make sure we don't break it again in the future |
|
Apologies, I have made a mistake. I use RubyMine and it allows me to change the code of locally installed gems, and while debugging the issue it appears that I have corrupted the code. When I created the test locally in the json-schema code it passed as expected as the I have added a test to this PR to cover plain text data. |
|
@benSlaughter could you rebase this branch onto master? The tests should work again now |
CHANGELOG.md
Outdated
| @@ -5,6 +5,13 @@ This project adheres to [Semantic Versioning](http://semver.org/). | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ### Added | |||
There was a problem hiding this comment.
Sorry @benSlaughter - can we remove the test changes from the changelog? (We try to keep the changelog for user-facing changes only)
|
I'll rebase and merge this manually in the next few days if I don't hear anything else. Would be good to get this merged |
|
Apologies, I have been off ill for a few days, I'll get the rebase done Monday morning, and remove the tests from the changelog. |
|
@iainbeeston ready for review |
|
Great, thanks! I'll get this released shortly |
I found a very odd edge case that will cause the
JSON::Schema::UriErrorto be raised if a string JSON contains a colon.ex.
look at this: oh no!To fix this I have added the exception to the
initialize_datafinal rescue, as it seemed to make sense that it is caught and silently discarded in this case.If the URI option is passed to the validator it will still raise the correct error.
If an invalid URI is passed to the validator, without the URI option, then it would also be silently discarded, I'm not sure if this is the best approach to this, my guess was that it would be user error, but opinions/references would be greatly appreciated as this is not really my area.