Skip to content

Conversation

@tisdall
Copy link
Contributor

@tisdall tisdall commented Jul 31, 2015

Okay, due to all the issues and comments surrounding colander validation I started working on a replacement. Basically a lot of things were being done in Cornice when they could have easily been left to Colander (like iterating over child elements). The schema.py file is now 62 lines where the old one was 180. The 4 main components (querystring, headers, body, and path) are grouped into one structure and passed through a single schema for validation tests. Each of those 4 components, if validated, are moved to request.validated. Any errors are put in request.errors.

This is obviously a huge change from the previous method, so I wanted to get some input before I complete all aspects of this PR (please ignore the failing tests for the moment).

I modified a few existing tests to kind of demonstrate how the system works on validating the body but obviously more are needed to test validating the other 3 types.

This deals with the following issues: #310, #211, #190, #327, #193, #249

Perhaps we could support #173 by calling unflatten in the validator.

@tisdall
Copy link
Contributor Author

tisdall commented Jul 31, 2015

BTW, don't bother looking at the diff's for the two files as they're pretty drastic changes and it's probably easier to just read the whole file...

With this change, `default` values are effectively ignored in
schemas. The schema would only have to take into account the
`deserialize()` process.
@tisdall
Copy link
Contributor Author

tisdall commented Aug 18, 2015

As per the discussion on #329, I've updated this PR to not call colander's serialize() any more and just manually create the cstruct. This way the validation schema only has to take the deserialize() process into account.

@tisdall
Copy link
Contributor Author

tisdall commented Aug 18, 2015

I'd like to hear back from someone with the ability to merge this PR before continuing work on it... I don't want to spend more time on it if it won't be used.

@leplatrem
Copy link
Contributor

Hi @tisdall,

Thanks a lot for tackling this! It was extremely necessary!

You can definitely go on with making the tests pass :) I'll take some time to review the pull-requests about schemas (like #337), and give as much feedback as I can :)

Thanks (again) :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When looking at this: https://travis-ci.org/mozilla-services/cornice/jobs/76151945#L525 recursivity seems infinite!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep. it seems I didn't properly test simple_cstruct_serialize(). It's because strings are iterable. I'll patch this.

@delijati
Copy link
Contributor

This is perfect. I'm currently working on #285 swagger integration and it would make the whole process of generating a consistent swagger schema much easier. Ony one schema for ever location. So only for body we could allow nested schemes.

The current validation process is too complicated a simple schema.deserialize(data) would be enough, if we got rid of the location and use this pull request (seperate schema for body, path, querystring, header).

@delijati
Copy link
Contributor

This would make it possible to use https://github.com/stefanofontanelli/ColanderAlchemy ;)

@tisdall
Copy link
Contributor Author

tisdall commented Nov 12, 2015

Hey guys... I'm not really working on the project where I used cornice any more and other responsibilities have come up that make it even more unlikely that I'll be able to finish this. Hopefully someone else can pickup where I left off.

@leplatrem
Copy link
Contributor

@leplatrem leplatrem closed this Oct 19, 2016
@delijati
Copy link
Contributor

Nicely solved :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants