Fix for string invalid scheme error when string contains colon

[benSlaughter]

I found a very odd edge case that will cause the JSON::Schema::UriError to 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_data final 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.

[benSlaughter]

I checked out the master branch and ran all the tests locally and the same tests are failing. These failures appear to not be related to my change.

Running the test file I modified yields:

Finished in 0.06318s
8 tests, 34 assertions, 0 failures, 0 errors, 0 skips

[iainbeeston]

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.

[iainbeeston]

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

[benSlaughter]

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. Please keep me updated, if there is anything I can do to help, please let me know.

[RST-J]

Wouldn't it help a lot if adressable had a validation method which doesn't raise an error but simply returned false if the given value couldn't be interpreted as an URI? Maybe the maintainers were up to such a feature request. I mean, it wouldn't be only for us, using exceptions for control flow is considered bad in general.

[benSlaughter]

That is a bloody good idea! I have raised an issue here: https://github.com/sporkmonger/addressable/issues/257 Maybe in the mean time a method could be added to the URI utils that abstracts the exception management away and acts as a validation method? I'd be happy to add such a method to this PR.

[iainbeeston]

@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

[benSlaughter]

Yes good point. I have updated changelog. Do I need to clean or squash my commits, I'm not sure of your standard.

[iainbeeston]

377 should fix the failing tests here

[benSlaughter]

@iainbeeston I have another issue, that may be solved in the same way. One response that I verify is an array of strings, it is possible that a string is just plain text. The only problem is, is that it is trying to use the custom_open functionality and is raising a file not found. Could it also catch the JSON::Schema::JsonLoadError here too?

EDIT: While playing with the unit tests I noticed that '"test"' works, however 'test' will fail.

[iainbeeston]

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

[benSlaughter]

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 JSON::Schema::JsonLoadError exception is already rescued. My local gem code was missing this and causing my tests to fail.

I have added a test to this PR to cover plain text data. Apologies once again. Pending tests this PR should be ready to go now.

[iainbeeston]

@benSlaughter could you rebase this branch onto master? The tests should work again now

[iainbeeston]

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

[benSlaughter]

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.

[benSlaughter]

@iainbeeston ready for review

[iainbeeston]

Great, thanks!

I'll get this released shortly