JSON::Schema::Reader

[pd]

Adds a JSON::Schema::Reader object/interface that can be used to control how URLs/paths are fetched. The default behavior is the same as on master: unregistered schema references will be fetched used open-uri or File.read. The default implementation accepts initializer arguments that can be used to reject all URIs, all files, or as callbacks that will receive an Addressable::URI or Pathname object and should return true or false to indicate whether it should be requested.

I've added documentation to the README explaining how to configure the reader, either by replacing the default instance used, or supplying a reader to use for a single call to validate.

This addresses issues from #47, #65, #152, #173 / #174. Probably others. I didn't add any support for my NullLoader NullReader idea mentioned in #152; since it's a simple interface to implement, users can just define their own which returns an empty schema (JSON::Schema.new({}, ...)).

[iainbeeston]

So the loader will be the single point through which we load any url, anywhere? (If so I'm all for it)

[pd]

I think the remaining failures here are artifacts of the mixed use of URI and Addressable::URI, and would probably be solved by #174. I'll try merging that in (minus the no-file-URI rule stuff) and seeing what happens when I get a chance.

[pd]

Rebased onto @RST-J's changes in #174 and the build is green. =)

Still a variety of things to do: document this in the README, see if we can 100% disconnect the test suite from the interwebs and maybe even drop webmock, etc.

[pd]

Just rebased this onto the new master after a lot of other changes around URI handling have landed; @RST-J's new tests for handling $ref with spaces in the names ended up exposing a bug in this branch!

I'll try to come back and tidy this up soon, plus add some documentation.

[pd]

@RST-J @iainbeeston this is ready for review if you get time.

[iainbeeston]

This is kind of a side-note, but it'd be really good if we could catch specific errors here...

[iainbeeston]

Sorry I didn't get a chance before, but this looks good. I like the idea of having a modular loader like this.

My only comment is that I wonder if there should be a simpler API for users to set accept_uri and accept_file (I've no idea how, maybe by adding a convenience method on JSON::Validator?). It's a feature I can imagine using a lot, but every other json-schema option (at present) can be set when calling JSON::Validator.validate

[pd]

I'd be nervous about adding any further options to validate and its friends. I don't imagine there being a lot of variants to using this feature in a single code base, and my main goal was to be able to replace the entire thing with a single call; in my use case, I actually just want to deny any-remote-request-ever, because that's a sign of a bug. My initial (now-rebased-away) version of the Loader class didn't support any options at all, it just maintained the current behavior of master, but made it trivial for me to change that by providing my own loader instance.

[iainbeeston]

Does anyone else have any feedback on this PR?

[RST-J]

Very cool feature, this boosts performance more than we could with anything else at the moment I think. Plus it provides more security :-)
Seems fine to me. 👍

[iainbeeston]

So, do we want to rename Loader to something else before merging?

[pd]

Yeah, let me rename this first. I'm gonna go with Reader since it's basically spot on with how I described it's role: "read (as in I/O)". Not sure why it didn't occur to me before. =)

I'll do that in the next couple of hours and ping you guys here when I'm done.

[pd]

Renamed JSON::Schema::Loader -> JSON::Schema::Reader. I think I got everything, but this could definitely use another set of eyes; ruby can be a bit unfriendly for "rename class" type refactorings. =)

[iainbeeston]

The tests would catch it, right? 😉

[RST-J]

👍

[iainbeeston]

👍