BUG: validate_colander_schema don't handle missing=drop

[surabujin]

Function validate_colander_schema call attr.deserialize even if
attr.missing == drop and data don't contain attr.name. This behaviour become
a problem if attr have preparer, which one expect something except
colander.null.

[almet]

Thanks for pointing that out. It misses tests to be merged, though.

[tisdall]

@surabujin - good catch! Can you also write some tests to cover this case?

[surabujin]

I have fixed existing test. Tomorrow I will add test for this particular case.

[surabujin]

Can anyone see my patch? If there are something wrong, I want to know it and fix it.

[tisdall]

@surabujin, I haven't had a chance to look over the substance of this patch, but there's a few things on the tests that don't look quite right... You've altered some schema that had default values and removed them or replaced them with missing. I think we need to continue testing for default values.

[surabujin]

"default" attribute in SchemaNode used only during .serialize() method. So using it here as "default" placeholder for .deserialize() is incorrect. Thats why I replace them with "missing", which one is used in .deserialize() method.

It works before, only because "weird" scheme deserialise way:

if attr.default != null:
    deserialized = attr.deserialize(attr.serialize())

it can/should be replaced with:

if attr.missing is not required:
    deserialized = attr.missing

[tisdall]

It looks like it's doing attr.serialize() to get the default value (when the default isn't null) and then using attr.deserialize() to put the type back (for example, if it's an integer the deserialize will put it back to an integer). missing would only be used in that case if default == null which doesn't happen in this case. The second half of the if statement calling deserialize() will effectively fetch the missing value since nothing is passed in.

It seems like the whole if statement should just be deserialized = attr.deserialize(attr.serialize()) as issues will be caught by the try:.

[tisdall]

Okay, this patch needs work... I just tried running the tests without the change in cornice/schemas.py and it runs without failure. You need to first write a test that demonstrates the issue with the existing code (a test that fails with the existing code but functions with the fix). Also, changes made to DefaultSchema and DefaultValueSchema effect the meaning of other tests and should probably be left alone.

[surabujin]

We don't use colander to serialize data, only to deserialize. According to colander documentation http://docs.pylonsproject.org/projects/colander/en/latest/api.html#schema-related :

• default: The default serialization value for this node. Default: colander.null.
• missing: The default deserialization value for this node. If it is not provided, the missing value of this
  node will be the special marker value colander.required, indicating that it is considered ‘required’.
  When missing is colander.required, the required computed attribute will be True. When missing is
  colander.drop, the node is dropped from the schema if it isn’t set during serialization/deserialization.

So we should never use "default" attribute to SchemaNode objects. We should not make any decision based on value of "default" attribute.

Current behaviour is leading to misunderstanding to people familiar with colander.

PS I will add requested by you test.

[tisdall]

@surabujin - colander is used in this case primarily for validation. They're accomplishing that with deserialize(serialize()). So, cornice does make calls to both deserialize and serialize. If default is set to something other than null than that element can be missing and the validation will still pass.

To re-iterate, my points are:

• don't modify existing tests unless it's directly related to the changes you're making.
• write tests that demonstrate the issue you're trying to fix (they should fail without your fix but pass after the fix is added).

[surabujin]

Last time - current cornice behaviour is incorrect from colander point of view. We should not use foreign library in incorrect way, only because we like "default" attribute more than "missing".

PS is validation != deserialisation?

[tisdall]

^_^ I'm not going to argue if cornice is using colander right or wrong. I'm just pointing out how it is doing it. And how it's doing validation is by calling deserialize(serialize()) and looking for colander exceptions. So, because of that, the values of both default and missing do matter.

[surabujin]

Weird logic... Sounds like - better add one more spike, than fix root clause.

[tisdall]

@surabujin, perhaps you're confusion is with what "deserialize" does... Colander has no concept of JSON and it's serialized format is an internal-use format called "cstruct" which is a dict that contains only strings. "deserialize" is the process to translate the cstruct into a dict with more "normal" types (such as integers) or custom types and will throw exceptions if it encounters errors.

So, the process of validation (as far as I understand it) is like this:

1. JSON -> simplejson -> basic dict
2. basic dict -> colander serialize -> cstruct (dict with only strings)
3. cstruct -> colander deserialize -> basic dict that may contain custom types

Any of those steps can throw an exception which indicates the validation failed. The colander steps will also make appropriate calls to validators and preparers.

[surabujin]

@tisdal look into the code, it acting in different way than you describe here. It you .serialize only if there is a not "null" default value.

PS Looks like we need to limit maximum version of mock packages in dependencies, because py26 tests become broken.

[almet]

I concur with @tisdall here: if we want to fix how colander is used in cornice, we'll need to make a new non-compatible release of Cornice. This might be acceptable, but if possible we should avoid doing it.

I propose to act in two steps:

1. We fix the behaviour you're trying to fix, without changing how colander is used in cornice;
2. We fix how colander is used in cornice, and release a major version of cornice with this inside (and probably other big breaking changes about the @resource decorator)

How does that sound?

[leplatrem]

I agree with @surabujin, the use of Colander library in Cornice was not sound, and was somehow confusing for people familiar with it.

I think the PR #330 covers the issue you found here since the code fully now relies on Colander. It is probably the best candidate for the many fixes about schemas.

But since @tisdall was discussing about default= and preparers in #329, I think we should make the issue explicitly clear and make sure it is fixed there in #330.

[leplatrem]

We took a different approach, see https://github.com/Cornices/cornice/blob/master/docs/source/upgrading.rst#colander-validation