Treat RefResolver cache args as decorators.

[avian2]

Currently, RefResolver class expects urljoin_cache and remote_cache arguments to be functions that in turn call urljoin and RefResolver.resolve_from_url methods.

This makes it inconvenient to provide a custom cache implementation: in order to pass it to the constructor, you need to make a function that calls the resolve_from_url method of a RefResolver instance before the said instance exists. This is possible, but usually ugly.

With this change, the constructor treats these arguments as decorator objects. For instance, this makes functools.lru_cache(16) an acceptable value for remote_cache. Existing docstrings already seem to suggest that this should be possible.

I'm not sure how much the urljoin_cache and remote_cache arguments are used in the wild. This change obviously breaks compatibility with existing code that uses them.

[avian2]

I've made this change backwards compatible.

New urljoin_cache_dec and remote_cache_dec arguments can now be used to provide caching decorators. Old urljoin_cache and remote_cache still accept pre-decorated functions.

[Julian]

Hey -- sorry, I was sure I responded here with something about backwards compatibility but apparently I managed to not actually do that :/

Yeah! Backwards compatible is definitely necessary here -- I can see the pain, and will have a look at the changes, but yeah we'd need to preserve whatever's here regardless.

Will try to post a more useful set of comments, I'm actually travelling so will have to find a bit of time to have a look, but really appreciated certainly.

[Julian]

So as I'm getting ready for Draft 6, I'm noticing some more things wrong with the RefResolver interface which is making me think this needs a bit of thought before just doing.

Specifically, resolve_fragment is kind of the wrong interface, because you can change scope while you're resolving, and it will affect refs within the thing you resolve to.

There's also still just a ton of stupid methods on RefResolver -- having the *_scope methods be public really bothers me.

I think this needs a bit of rethinking overall rather than patching things yet again, it's kind of a mess already :/

Thoughts / suggestions welcome of course!

[handrews]

@Julian I actually wrote an id-processor / reference resolver in Python at one point, and then decided that when I eventually get to building a python hyper-schema client it would probably make more sense to use your validator as a base anyway. I can dig it up if you want- may not be suitable for direct use but it might be nice to compare implementation hazards.

[Julian]

@handrews that'd definitely be appreciated!

My inclination is actually to move ref resolution into a new external package, with a correct interface, and to deprecate everything here, after reimplementing it on top of it.

[handrews]

I'll look into it on Monday. It's not too far off from being able to be its own library, although whether it's the API you'd want I don't know (I don't actually remember the exact state it's in- I last touched it around December, I think).

[avian2]

@Julian I find current resolve_fragment useful in jsonmerge to resolve pointers in the scope of different subparts of the document (arrayMergeById, idRef option here, code).

While on topic of reworking the RefResolver interface. My other wish would be to have an easy way to determine if a reference is internal or external before resolving it. jsonmerge often doesn't need to follow external references. Also, arbitrary HTTP requests might be a security issue for some people (similar to how XML external entities can be).

[Julian]

@Julian I find current resolve_fragment useful in jsonmerge to resolve pointers in the scope of different subparts of the document (arrayMergeById, idRef option here, code).

Makes sense, will try to keep this use case in mind as well, we can probably come up with something that suits both that's still cleaner than what we've got here.

While on topic of reworking the RefResolver interface. My other wish would be to have an easy way to determine if a reference is internal or external before resolving it. jsonmerge often doesn't need to follow external references. Also, arbitrary HTTP requests might be a security issue for some people (similar to how XML external entities can be).

Would like to hear more about this. In the current way of dealing with things, this is the difference between resolve_from_url or not -- I've seen people who don't want to resolve remote refs override that method and make it raise an exception (even though subclassing isn't supported in jsonschema), so yeah it seems like there should be a reasonable API for swapping that method out.

[avian2]

Would like to hear more about this. In the current way of dealing with things, this is the difference between resolve_from_url or not -- I've seen people who don't want to resolve remote refs override that method and make it raise an exception

I've made myself a is_remote_ref() method here that seems to work.

Raising an exception from resolve_remote (or resolve_from_url) was awkward for me because with code like this:

with resolver.resolving(ref) as resolved:
    ...

It's AFAIK impossible to cleanly catch and handle an exception that originates from resolving() context manager and not at the same time catch exceptions from the ... block with-in. For example, approaches discussed here don't work because in the contrast to open() the exception here only gets raised on __enter__() call.

[Julian]

contextlib.ExitStack is the way to do that pretty much. But yeah context managers are a bit painful in these ways. The duplication there is part of the interface bloat I was referring to.

[avian2]

I updated this pull request for the current master branch.

As far as I can see, the remaining Sphinx "broken link" errors that are still thrown by Travis don't have anything to do with my changes.

[Julian]

@avian2 hrm.. will have a look to see why Travis is complaining. Thanks for updating this, I still don't quite know how I feel about it :/, but it's been sitting here long enough that I'm starting to lean towards merging it despite my desire to revamp the whole interface.

[codecov]

Codecov Report

Merging #333 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     python-jsonschema/jsonschema#333      +/-   ##
==========================================
+ Coverage   96.31%   96.33%   +0.02%     
==========================================
  Files          19       19              
  Lines        2386     2403      +17     
  Branches      312      314       +2     
==========================================
+ Hits         2298     2315      +17     
  Misses         75       75              
  Partials       13       13              

[avian2]

Was this PR closed by mistake? I understand if you don't want to merge this. I'm just asking because the message in the squashed commit that's linked above makes it look like it includes my changes, but it doesn't actually. The GitHub issue # numbers don't match the repo names.

[Julian]

Oy. Hi @avian2 It was indeed closed by mistake though not from the push (which yeah mingles issue #s across repos but it's a known issue), probably just me somehow misclicking while I wasn't paying attention.

Going to reopen but yeah... somehow in all this time I still don't feel comfortable merging this, so to be honest I may re-close (hopefully intentionally) and just say we need to hit python-jsonschema/referencing#3.