Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert None to colander.null before calling deserialize #359

Closed
wants to merge 1 commit into from

Conversation

tsauerwein
Copy link

This PR is an attempt to fix #349 by replacing all occurrences of None with colander.null before the validation.

A more detailed description of the problem is in #349.

Closes #349

@leplatrem
Copy link
Contributor

leplatrem commented May 30, 2016

I don't really understand why colander is not in charge of this. Is it because of the hacky usage of colander in Cornice?

Otherwise it does look edit:NOT bad to me. @almet @Natim any feedback?

@leplatrem
Copy link
Contributor

@tsauerwein do you know why we have to do this? Why colander would not be in charge of this?

@tsauerwein tsauerwein force-pushed the nested-none branch 2 times, most recently from 73fd02b to 674c1be Compare October 20, 2016 13:19
@tsauerwein
Copy link
Author

Why colander would not be in charge of this?

That is a good question, and I am not totally sure about this myself.

In their tests they are explicitly testing that None can not be processed:
https://github.com/Pylons/colander/blob/master/colander/tests/test_colander.py#L935
or https://github.com/Pylons/colander/blob/master/colander/tests/test_colander.py#L690

The documentation of deserialize says: "Deserialize the cstruct into an appstruct ...". And a cstruct is a "data structure generated by the colander.SchemaNode.serialize()". And I assume that a cstruct is not supposed to contain None?

@leplatrem
Copy link
Contributor

The behaviour with None does not seem weird:

>>> import colander
>>> class Payload(colander.MappingSchema):
...     name = colander.SchemaNode(colander.String())
... 
>>> class Body(colander.MappingSchema):
...     payload = Payload()
... 
>>> schema = Body()
>>> schema.deserialize({"payload": {"name": None}})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/mathieu/Code/Mozilla/kinto/.venv/local/lib/python2.7/site-packages/colander/__init__.py", line 2058, in deserialize
    appstruct = self.typ.deserialize(self, cstruct)
  File "/home/mathieu/Code/Mozilla/kinto/.venv/local/lib/python2.7/site-packages/colander/__init__.py", line 719, in deserialize
    return self._impl(node, cstruct, callback)
  File "/home/mathieu/Code/Mozilla/kinto/.venv/local/lib/python2.7/site-packages/colander/__init__.py", line 699, in _impl
    raise error
colander.Invalid: {'payload.name': u'Required'}
>>> 

@tsauerwein
Copy link
Author

The behaviour with None does not seem weird:

In this case, yes. But it gets weird once you have sequences or nested schemas. I added some examples in the tests: https://github.com/Cornices/cornice/pull/359/files#diff-bdda56b22b0eb46ed999750ec24c5785R121

@leplatrem
Copy link
Contributor

Hum, I see...

Well to be honest I'm currently not sure if Cornice should be in charge of handling the weird cases of Colander :/

I think it would be good to start a discussion with Colander devs first, based a very simple example that reproduces the problem. And then do such preprocessing here only if we're obliged to :)

(The PR is good, it's just that we're trying to keep the source code small and decorrelated as much as possible from colander specificities)

@tsauerwein
Copy link
Author

tsauerwein commented Oct 26, 2016

That makes sense, yes. I will try to get an answer from the Colander folks.

//edit: Issue on Colander is here: Pylons/colander#276

@leplatrem
Copy link
Contributor

I can see that the issue upstream on Colander is still open.
But I'm closing this PR, since it's 4 years old.

@leplatrem leplatrem closed this Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None values in nested schemas
2 participants