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

Rewrite colander schema #337

Closed
wants to merge 18 commits into from
Closed

Rewrite colander schema #337

wants to merge 18 commits into from

Conversation

surabujin
Copy link

I am propose modular interface to schema validation. It's interface and behaviour mostly compatible with existing schema validator.

But it use "clean" colander deserialisation call, so all existing hacks that emulate in incompatible way colander features are removed.

PS New interface allow add other schema validators.

This package will replace existing cornice.schemas module. The aim of replace
- more smooth dependency check and more correct colander integration.

Current implementation of colander schema "split" schema provided by user code
into separate attributes and check each attribute separately. Because of that
it ignore "preparer" and "validator" attached to schema object itself. Also it
try to do a lot of work that colander do itself, if properly called (handle
missing and default attributes etc).

task: rewrite schema integration
This tools allow to select correct adapter and validate data using
this adapter.
Till this time "callable" schema handled by GenericAdapter. Create separate
adapter for "callable" schema validator. It allow us make more correct adapter
detection.

Also convert GenericAdapter into abstract class with one required method
"__call__".

Also move dotted name resolution from generic adapter constructor into adapter
"use" function. This is required because adapter can be created on module
import time. So it can be too early to resolve dotted name. And lead to
dependency loop.
This adapter will emulate current cornice schema validation technique. As far
as it not disrupt new features.
By default all "war" collander schemas into backward compatible adaptor.
Add ability to return metadata from module's init method. At now only name of
adapter is used.
Remove hadnling of "extra" fields, because they are handled by colander itself.
Make manual "unpack" of errors tree returned from colander. It allow extract
from there info about "extra" fields. Also it allow to map fileds source to
errors.

Depends from Pylons/colander#241
Also remove tests related to old schemas module only.
Before we try to make colander work, and handle "default" values in
incompatible with colander way. We thread as default value of "default"
atribut. But colander treat as default value of "missing" attribut for
deserialize operation.

Because we don't do colander job any more, we can remove tests used to test
incompatible behaviour.
Because all colander job is done by colander, some response messages have been
changes. So tests updated accordingly.
Now when schema detect at least one error, it don't fill "validated" field at
all. Some existing test expect that "validated" should be filled even in case
of erros. Remove such expectations from tests.
@surabujin
Copy link
Author

Depends from Pylons/colander#241

@leplatrem
Copy link
Contributor

Thanks a lot @surabujin!

I really like the fact to be able to validate the data with any library! It covers issue #274 (and stuff like mozilla-services/cliquet#244). Plus it looks like you managed to write some backward compat stuff. Neat!

The diff is pretty huge to review, did you write specific tests for the new code?

What do you think if you split the Colander usage refactor from the pluggability (based on #330)? Is that even possible?

Thanks again for this huge piece of work!

@surabujin
Copy link
Author

I have not add any specific tests yet. I just reach first goal - make it pluggable, leave colander work to colander and make it as much as possible to previous API (it can be even more compatible, but I don't see a sense for it now).

Even if I can split this 2 change - make schemas pluggable and rewrite colander support, I can't make to work old colander validation code with proposed pluggable model. So one patch can't be used without another one.

@tisdall
Copy link
Contributor

tisdall commented Sep 14, 2015

I like the idea of making the schema checking pluggable as there's been other requests for using other validation libraries already. I'm having a little trouble understanding how this patch works, though... Do you have any example code that you used to test when developing?

@surabujin
Copy link
Author

@tisdall you want examples how other libraries can be integrated here? I don't have them yet.

I will update documentation, also I have a plan to add support of FormEncode.

@tisdall
Copy link
Contributor

tisdall commented Sep 14, 2015

No, I was thinking more along the lines of examples of how this works with
existing colander schemas.

On 14 September 2015 at 15:46, Dmitriy Bogun notifications@github.com
wrote:

@tisdall https://github.com/tisdall you want examples how other
libraries can be integrated here? I don't have them yet.

I will update documentation, also I have a plan to add support of
FormEncode.


Reply to this email directly or view it on GitHub
#337 (comment)
.

Plus fix compatible schema adapter enforcing logic.
@surabujin
Copy link
Author

Promised usage example: https://gist.github.com/surabujin/8109f55dd02b57c17fda

@leplatrem
Copy link
Contributor

Promised usage example

I like it, even though I wonder if we shouldn't get rid of those custom location parameters, as in #330

Depends from Pylons/colander#241

Unfortunately, the merge and release cycles of colander are very long. See for example the PR #206, which depends on Pylons/colander#157. The PR was merged more than a year ago in colander, but never got released.

That's also why having pluggable schema libraries is a must-have! Projects like https://github.com/nicolaiarocci/cerberus or https://github.com/marshmallow-code/marshmallow are a lot more active and reactive!

Nevertheless, please be aware that we cannot merge so much new code without exhaustive and «specs» tests [1]. Especially because afterwards, we become the implicit maintainers of all this :)

Good luck and thank again for this excellent initiative :)

[1] : http://blog.mathieu-leplatre.info/your-tests-as-your-specs.html
https://medium.com/javascript-scene/what-every-unit-test-needs-f6cd34d9836d

@lmctv
Copy link

lmctv commented Jan 16, 2016

Pylons/colander#241 has been merged and released as of colander 1.2!

@leplatrem
Copy link
Contributor

@leplatrem leplatrem closed this Oct 19, 2016
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.

4 participants