Fix schema typing

[tisdall]

fixes: #307

Unfortunately I didn't have the time to fully understand the code and try to do duck typing. Right now it uses the unknown property that only really exists on the Mapping type. The major thing is a SchemaNode(Mapping()) will now work.

[almet]

The major thing is a SchemaNode(Mapping()) will now work.

This really is a problem, since it would break Cornice in an incompatible way with previous versions. We might need to find a better mechanism to decide if we should apply the validation.

For instance, it might make sense to be able to override this with a property of the passed schema, or pass a callable that should be called.

[tisdall]

I don't understand. Here's the definition of a MappingSchema in colander:

class Schema(SchemaNode):
    schema_type = Mapping

MappingSchema = Schema

It's just a SchemaNode with schema_type() is Mapping(). schema_type() is called on __init__ if no typ is provided.

MappingSchema is just a shortcut class if you don't want to do SchemaNode(Mapping()). They're equivalent.

Without this change, it's impossible to use a SchemaNode which is the basic building block of all colander schema. Also, I don't see how this change would break any functionality as it just removes erroneously throwing an error on SchemaNode instances that would otherwise work fine.

(maybe the confusion lies with my changes to the tests... the tests pass with or without my changes. The changes are just to make things clearer and more consistent.)

Additionally, colander has the "feature" of allowing you to do the following:

>>> import colander
>>> x = colander.MappingSchema(colander.Sequence())
>>> x.typ
<colander.Sequence object at 0x7fb908fc6208>
>>> x.typ.unknown
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'Sequence' object has no attribute 'unknown'
>>> 

So, testing if a class is an instance of a MappingSchema doesn't mean that it actually has a Mapping type. (Pylons/colander#168) So, you're really testing the wrong thing with checking if a schema is an instance of a MappingSchema.

[almet]

Thanks for working on this fix! I actually misread "now" with "not", sorry about that.

This fix will break cornice because the test isn't done in the same way. I know at least about one application that relies on the schema_type method.

So, here are a few things we should add to the this PR before merging:

• Tests! We need to showcase what this fixes, in order to avoid future problems when modifying the code;
• In case we will fail, we should probably check both typ and the schema_type before failing, since both are used in different projects.

[tisdall]

The issue is, though, is schema_type() (despite the name) is not intended to tell you the type of the schema. (See Pylons/colander#229 (comment)) The only purpose/use of schema_type() is for subclassing SchemaNode and defining a default type to use if you don't supply one directly. However, there's nothing restricting a subclass from just using a different type and ignoring schema_type()

example:

>>> import colander
>>> x = colander.MappingSchema(colander.Sequence())
>>> x.typ
<colander.Sequence object at 0x7ff71c745390>
>>> x.schema_type
<class 'colander.Mapping'>

In the above example, x should be treated as a schema with type Sequence despite what schema_type says (or what class it's subclassing from).

Also, schema_type is a static method that returns a new instance of a type, so with Mapping it won't return the proper value of unknown.

>>> x = colander.MappingSchema(colander.Mapping(unknown="raise"))
>>> x.typ
<colander.Mapping object at 0x7ff71c745358>
>>> x.typ.unknown
'raise'
>>> x.schema_type
<class 'colander.Mapping'>
>>> x.schema_type().unknown
'ignore'

So... anything using schema_type for anything other than providing a default typ with sub-classing is just doing it wrong. I think the only use for checking schema_type is if Cornice accepts schema classes and not instances (I'll have to defer to you if that happens... I'm not really sure).

I'll see what I can do about tests...

[tisdall]

Okay, the call to CorniceSchema.colander_schema() guarantees that it's an instance and not a class so we should ignore schema_type there. I'll add a comment to that effect to make sure it's clear.

[tisdall]

Okay, I think this covers it...

[tisdall]

Would you like me to make a note in CHANGES? Is there anything else missing on this PR?

[almet]

/me summons our very own colander expert @leplatrem for a review :)

[leplatrem]

I don't see anything wrong, and agree with @tisdall comments! it's indeed cleaner!

[almet]

Merged! Thanks :)