Support namedtuple

[ejconlon]

Though json doesn't treat namedtuple instances as objects, simplejson does. It would be useful to be able to validate object graphs with namedtuple instances like dict instances.

The main complication here is that Validator's is_type and DEFAULT_TYPES and very subclass-oriented, and detecting a namedtuple is a matter of duck-typing (hasattr(instance.__class__, '_fields') or the less-restrictive hasattr(instance, '_asdict')). From there I'm not quite sure where to make the change to traverse object items.

Thanks for your consideration.

[Julian]

:/ that makes me a bit sad to see, but I've already been open to the idea that is_type's interface should be wider (it started narrow to drive out needing it to be wide, but it's likely getting to the point where now it needs to be -- draft 6 already poses some issues there).

Will have to think about possible ways to widen it but I think this sounds reasonable. Suggestions welcome obviously, as well as certainly patches.

[bsmithers]

Just moving the conversation from the PR (#370 (comment)) here to keep things more contained.

I've had a little look at your suggestions [1]. I can certainly see the benefit in immutability and a TypeChecker object feels like a good approach. I think the interface cannot be quite as simple as it first seems though.

Although type_checker.redefine(**types) looks nice, it can't handle non-string types [2]. Another minor issue is that it doesn't allow for the removal of a type check (e.g. any was a valid type in draft3). So I've implemented it as type_checker.update(redefine=(), remove=()) where redefine is a dictionary of checks to add or update and remove is any iterable.

The namedtuple case is then the slightly more verbose:

type_checker = Draft6Validator.TYPE_CHECKER.update(redefine={
            u"object": object_or_named_tuple
        })
v = Draft6Validator(schema, type_checker=type_checker)

Some additional complications:

1. Since the old type checking was via isinstance, type_checker tests if has been given a type (or tuple of types) and invokes the legacy type checking in this case. This is at least self-contained and could be removed in future
2. type_checker cannot raise a proper UnknownType exception as it doesn't have the schema. Currently I've handled this by raising a secondary exception, which is re-raised as an UnknownType exception by the Validator
3. Finally, it feels odd to me that types are specified both in create and in a validator's init. I might just be missing some history here but it feels that a Validator providing custom type checking is no longer a DraftXValidator? Perhaps it would keep things cleaner to only allow type_checker to be specified in create()?

[1] draft6...bsmithers:types
[2] https://github.com/Julian/jsonschema/blob/master/jsonschema/tests/test_validators.py#L706

[Julian]

Awesome!

Progress looks great, really grateful you're persisting here! Gonna leave a few notes just on the first half of your comment, I need to read your code to be able to answer the "additional complication" section I suspect. Will try to do that ASAP.

Although type_checker.redefine(**types) looks nice, it can't handle non-string types [2].

Man... I don't even remember what the use case is for that. Will have to hunt down the original PR. In theory obviously you can still support that via type_checker.redefine(**{None: bla}). If the use caes isn't strong I'd say that's probably OK, no? If it is though, yeah, I'd be thinking we should consider then an API that looks more like type_checker.redefine(type, fn), which you'd then chain (though it might be convenient to provide a second one that took a whole bunch to do at once, but we could decide separately whether to do that initially or not based on whether any place inside jsonschema itself needed to do that operation in a way that made chaining a few together ugly).

Another minor issue is that it doesn't allow for the removal of a type check

I'd prefer this as a separate API -- i.e. type_checker.remove(type)

Your namedtuple case looks perfect to me structure-wise -- with the above about removal I think we could get back to Draft6Validator.TYPE_CHECKER.redefine(object=object_or_named_tuple) right?

Since the old type checking was via isinstance, type_checker tests if has been given a type (or tuple of types) and invokes the legacy type checking in this case. This is at least self-contained and could be removed in future

I'd prefer an explicit API for this -- a private one on TypeChecker, so that we could decide later whether it should be public or not.

[Julian]

OK! Few more comments now after skimming the code:

Since the old type checking was via isinstance, type_checker tests if has been given a type (or tuple of types) and invokes the legacy type checking in this case. This is at least self-contained and could be removed in future

So I'd move this outside of TypeChecker -- within both create and SomeValidator you know that types are tuples, so the conversion can happen there (possibly via a private helper function), such that the real TypeChecker only has to deal with things satisfying its new better interface.

type_checker cannot raise a proper UnknownType exception as it doesn't have the schema. Currently I've handled this by raising a secondary exception, which is re-raised as an UnknownType exception by the Validator

I think it's OK here to have type checkers return True or False, and let the exception happen inside the Validator. I think also that it's probably a good idea to deprecate Validator.is_type entirely (which will make this "problem" go away and better divide between the two) -- does that make sense to you?

Finally, it feels odd to me that types are specified both in create and in a validator's init. I might just be missing some history here but it feels that a Validator providing custom type checking is no longer a DraftXValidator? Perhaps it would keep things cleaner to only allow type_checker to be specified in create()

So I guess there's a historical answer (that create and extend didn't always exist -- now that they do yeah it's pretty easy to just call extend) but also a "convenience" answer -- my guess (in retrospect) is that things like "add decimal as a number type" is common enough for people to want to do, and that well, you could squint and still call that a DraftXValidator.

But! I think it might still be a good idea to deprecate the types parameter as you've done, and to not add a replacement but to say "use extend" until it's obvious that we should re-add it back to a validator itself? Definitely though will need to have the documentation show examples for how to do this the new way.

Again, really appreciate the work! Think we're definitely headed in the right direction, lemme know what you think about all the above!

[bsmithers]

Man... I don't even remember what the use case is for that. Will have to hunt down the original PR. In theory obviously you can still support that via type_checker.redefine(**{None: bla})

Unfortunately that won't work. Keyword arguments must be strings.

I'd be thinking we should consider then an API that looks more like type_checker.redefine(type, fn), which you'd then chain (though it might be convenient to provide a second one that took a whole bunch to do at once

Sure, works for me. I think we do want the multiple redefinition option though. To make the cleanest use of attr's frozen and the pyrsistent map, it feels nicer if all changes to the type checkers happen through evolution, which includes the initial Draft3 checks. We could juggle the initialisation with setattr, but it feels unpleasant.

With that in mind, we have .redefine(type_, fn), .redefine_many(definitions=()), .remove(type_), and - for completeness - .remove_many(types)

So I'd move this outside of TypeChecker -- within both create and SomeValidator you know that types are tuples, so the conversion can happen there (possibly via a private helper function), such that the real TypeChecker only has to deal with things satisfying its new better interface

A really good point and I completely agree.

I think it's OK here to have type checkers return True or False, and let the exception happen inside the Validator. I think also that it's probably a good idea to deprecate Validator.is_type entirely (which will make this "problem" go away and better divide between the two) -- does that make sense to you?

I'm not sure about either of these things. The return value of TypeChecker.is_type indicates if the instance was the correct type or not. I think it needs to raise an Exception here. Maybe I'm misunderstanding - where do you imagine the exception being raised by the Validator? Whatever calls TypeChecker.is_type needs to handle the case of an unknown type and to me it makes sense to continue with this being Validator.is_type, which each individual validator in _validators can continue to call. We then have Validator as the only thing that knows type checking is deferred to the TypeChecker object, which feels correct?

But! I think it might still be a good idea to deprecate the types parameter as you've done, and to not add a replacement but to say "use extend" until it's obvious that we should re-add it back to a validator itself? Definitely though will need to have the documentation show examples for how to do this the new way.

Agreed. I'm pretty happy this is heading the right way now (thanks for all the input!) so I'll start trying to round this out with test coverage & documentation changes also.

By the way, I've based this work on the draft6 branch - to me it makes sense to release the changed type interface alongside draft 6 support - but let me know if you'd prefer it rebased against master.

[Julian]

Unfortunately that won't work. Keyword arguments must be strings.

Er yeah let's make believe I didn't say that :D, I wrote it off the cuff and was thinking of the similar case where you do have strings but they're keywords (e.g. class) and can't use the regular syntax.

I think we do want the multiple redefinition option though.

OK, fine with me too.

Whatever calls TypeChecker.is_type needs to handle the case of an unknown type and to me it makes sense to continue with this being Validator.is_type, which each individual validator in _validators can continue to call. We then have Validator as the only thing that knows type checking is deferred to the TypeChecker object, which feels correct?

Sorry, I think I misunderstood what you meant here, although there's some ugliness here that I'm not sure I remembered :/ -- I think what you have is the best current solution, although it'll be super confusing to have 2 "unknown-type"-y exceptions... At very least we should document in both places clearly which one they'll raise, but yeah I don't have better ideas than that at the moment.

By the way, I've based this work on the draft6 branch - to me it makes sense to release the changed type interface alongside draft 6 support - but let me know if you'd prefer it rebased against master.

I think it'd help it get merged quicker since it would be independently doable, but either way I think is fine.

[bsmithers]

I think what you have is the best current solution, although it'll be super confusing to have 2 "unknown-type"-y exceptions... At very least we should document in both places clearly which one they'll raise, but yeah I don't have better ideas than that at the moment.

Agreed. At least one of these is fairly "internal"; i.e. the new exception is either caught by the validator itself or only raised during "advanced" usage (definition of new TypeChecker).

I think it'd help it get merged quicker since it would be independently doable, but either way I think is fine.

Sure. I'm basically done with implementation & testing, so before I do the documentation changes I'll see how awkward the rebase is.

[Julian]

Now that #374 is merged (and mostly cleaned up), something facilitating this should land in the next release!