Cornices / cornice

Build Web Services with Pyramid.
https://cornice.readthedocs.io
Other
384 stars 149 forks source link

None values in nested schemas #349

Open tsauerwein opened 8 years ago

tsauerwein commented 8 years ago

When using nested schemas, there is a problem during the validation of data that contains None values. For example for the following schema, the validation fails with the error message colander.Invalid: {'b.a': '"None" is not a mapping type: Does not implement dict-like functionality.'}.

class SchemaA(MappingSchema):
    val = SchemaNode(String())
class SchemaB(MappingSchema):
    a = SchemaA()
class SchemaC(MappingSchema):
    b = SchemaB()

schema = CorniceSchema.from_colander(SchemaC)
dummy_request = get_mock_request( '{"b": {"a": null}}')
validate_colander_schema(schema, dummy_request)

Similar errors arise when using sequences or tuples in nested mappings ('"None" is not iterable'). I added 3 failing tests to the test-suite to demonstrate the issue: https://github.com/tsauerwein/cornice/commit/5bd09404d10fad5b7065c0a98a7bc250ff47e360

The problem is that deserialize is called with data that still contains None values. Because Colander expects null, it will error during the validation.

I would propose to recursively replace all Nones with null before calling deserialize. I can prepare a PR, but I first wanted to check if there is no better solution.

almet commented 8 years ago

Hey, can you have a look at this pull request, and try to see if it solves your issues? https://github.com/mozilla-services/cornice/pull/330

tsauerwein commented 8 years ago

I added one of the failing tests to that branch (see https://github.com/tsauerwein/cornice/commit/e6f6d3d3a1e0d749b14ac5c1d1408370b8008f20), but I am getting exactly the same error:

[{'description': '"None" is not a mapping type: Does not implement dict-like functionality.', 'name': 'b.a', 'location': 'body'}]

Are there plans to integrate #330?

almet commented 8 years ago

About #330, we would like to see it have some more work before getting it merged. I'm not a expert about colander, so let me summon @leplatrem which has some more experience on this :)

delijati commented 8 years ago

I've mentioned this also in my pull request #340