-
Notifications
You must be signed in to change notification settings - Fork 14
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
Colander declarative schemas handling #42
Conversation
Include parameters `items` field
Simplify parameter converters and solve array body references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing this! I've got some minor comments, but we can merge the PR as it is. 😄
Also, can you please add your name to the contributors file?
@@ -344,36 +347,34 @@ def from_schema(self, schema_node, validators=[]): | |||
if not isinstance(schema_node, colander.Schema): | |||
schema_node = schema_node() | |||
|
|||
if colander_validator in validators: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way we are taking schema
assuming it's described for colander_validator
even if we don't use the validator? This is an interesting behavior but I'm not so sure if it's a good idea, as it can break with some unusual custom validators, but we can ignore such use case for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is intentional. schema
is descriptive attribute storing the schema for the service (see Cornices/cornice#376), and colander_validator
is just a shorthand validator that validate the request against the schema defined on the service. The original intention was to to keep schema and validation two different concerns, and I think that swagger should use only the schema because it serves the same precise concern (describing the service).
The introduction of colander_body_validator
broke that behaviour, schema and validator became tightly coupled, so colander_body_validator
should be treated as special case.
@@ -2,7 +2,12 @@ | |||
|
|||
from cornice_swagger.swagger import ResponseHandler, CorniceSwaggerException | |||
from cornice_swagger.converters import convert_schema | |||
from .support import BodySchema, HeaderSchema, ResponseSchema, response_schemas | |||
from .support import (BodySchema, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: you may keep all the imports in one or two lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, sure. I'm used to the pylons project code style :) I will fix in the next commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to follow the cornice way to do it, but I just realized in some parts they have all kinds of import styles 😐. So it's OK to keep it like this. I just think it's important to make sure the rest of the package consistent.
Also, we didn't consolidate the API yet, so you are free to break it if you think it makes sense. :) How would you do it without validators? |
Following the comment above, my idea is that class RequestSchema(colander.MappingSchema):
body = BodySchema()
querystring = QuerySchema()
header = HeaderSchema() Doing so, it doesn't need to know about validators. But this approach needs an intermediate step aware of validators between [edit] A pluggable transform stage could also address your concern about unusual custom validators. In case exotic schemas with custom validators are used, you should provide a transform stage that transforms the schema to the full schema expected by |
That looks good to me, and it's definitely an improvement. So let's do it! 👍 |
colander body schema transformer
Here you go. Now |
Great! LGTM! |
When colander declarative schemas with
@instantiate
decorator are used, egschema names and refs collide and only one schema is used for all the services.
This pull request fixes this behaviour.
I've also refactored
ParameterHandler.from_schema
, removing repeated code. I'm still not happy that the method has to be aware of the validators, but all the solution I've in mind involve breaking changes to the api, so I left them :)