-
-
Notifications
You must be signed in to change notification settings - Fork 744
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
Support for Cerberus 1.1 #1001
Support for Cerberus 1.1 #1001
Conversation
Great, thank you for re-opening this! I've quite busy with Eve-SQLAlchemy for now so did not have time myself to look into this again. I just saw that |
@dkellner Removed the dependency_links lines from setup.py and tox.ini. |
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 made a review from a cerberus-only pov. it's encouraging to see client code to be reduced. :-)
i also found two possible leverages for more:
- cerberus could look up the rule constrains schemas from overridden methods when an overriding method doesn't provide one
- cerberus could auto-create the properties that map to
self._config
keys or implement__getattr__
and__setattr__
i noted these.
self._config['resource'] = value | ||
|
||
@property | ||
def document_id(self): |
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 think it's not a bad idea to add this property along with a schema_id
to the vanilla Validator. essentially to have canonically named properties that extensions like error handlers can hook to.
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.
Just to clarify: This is nothing concerning this very PR, right?
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.
that's correct.
eve/validation.py
Outdated
self._config['persisted_document'] = value | ||
|
||
|
||
class DefaultErrorHandler(cerberus.errors.BasicErrorHandler): |
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.
assuming that config.VALIDATION_ERROR_AS_LIST
doesn't change during runtime, you could reduce overhead with roughly this:
class SingleElementListMixin(object):
def pretty_tree(self):
...
if not config.VALIDATION_ERROR_AS_LIST:
DefaultErrorHandler = type('DefaultErrorHandler', (cerberus.errors.BasicErrorHandler, SingleElementListMixin), {})
else:
DefaultErrorHandler = cerberus.errors.BasicErrorHandler
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 agree that we could do this check only one time at application startup. But I wonder if creating a dynamic type based on a config item can get confusing once someone will subclass this again.
How about renaming DefaultErrorHandler
to something like SingleErrorsAsStringErrorHandler
(or a better name if you can come with one) and then amend Validator.__init__
as follows?
if not config.VALIDATION_ERROR_AS_LIST:
kwargs['error_handler'] = SingleErrorsAsStringErrorHandler
But again I cannot see a "real" issue with your (definitely more sophisticated) suggestion.
eve/validation.py
Outdated
return field not in self.document and \ | ||
self.persisted_document and \ | ||
field in self.persisted_document | ||
return list(filter(persisted_but_not_in_document, fields)) |
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.
aren't list comprehensions the common idioms for such expressions nowadays?
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.
Agree, and it should make it more readable, too.
""" {'type': ['dict', 'hashable', 'hashables']} """ | ||
persisted = self._filter_persisted_fields_not_in_document(dependencies) | ||
if persisted: | ||
dcopy = copy.copy(self.document) |
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.
would a deepcopy be less error prone here?
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, especially regarding the fact that the child validator can do normalization, too. I just looked into this and copy.copy
was used in the original code as well. I think that's the reason I kept in there in the beginning. A "real" fix would include a test case where a plain copy fails so we never need to wonder about this again.
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'm leaving this for later. I'm not familiar enough with the code to be able to make an informed change here.
@nicolaiarocci I will review this during the next days. In the meantime we can discuss about updating the documentation (as already mentioned here: #917 (comment)). Additionally, we should add a section about upgrading, as this will introduce some breaking changes. |
Yes docs need to be amended and upgrading should be covered. |
I looked into this again, but this is my code without changes (so far) - so I'm not sure if I'm really the right person to review it ;). Nonetheless, I took some notes regarding needed documentation changes. The list is probably not complete, but a hopefully a good start:
@bcrochet Can you implement @funkyfuture's suggestions (amend to my commit if you want, I think that's fine in this case) and add another commit for the documentation changes? |
@dkellner You're absolutely right. It is mostly your original patch. I think I maybe changed 2 lines, just to get it working with the latest cerebrus. But yes, I will take @funkyfuture 's suggestions and amend this commit. |
actually, i'm not sure whether this errorhandler is really needed. in cerberus the argument was that current client code should already be ready to deal with such structures as they were used for 2+ errors. |
@bcrochet is this work ready for merge, or you still have work left to do? |
@nicolaiarocci I'm currently rebasing and will have some changes, so it's not quite ready. |
35e774f
to
33c3415
Compare
I'd consider this ready. However, the python2.6 job is not passing, due to python2.6 being not found. Should this be dropped from tox.ini? |
Looks like the latest Travis update broke our .travis.yml. Can you please rebase (again) on top of master? Thanks! |
b5e33b2
to
43cf038
Compare
Rebase done. I also went ahead and included doc changes. |
Excellent. One last effort and we're done. Please, update the 'breaking changes' section in the changelog. I guess a good reference would be Cerberus' own "upgrading to 1.0" page at https://cerberus.readthedocs.io/en/stable/upgrading.html. |
(Based on original patch from dkellner) This is a rather big change. I still decided to do a single commit, as intermediate commits would be in a non-working state anyway. Breaking changes: - `keyschema` was renamed to `valueschema` and `propertyschema` to `keyschema` (following changes in Cerberus). - A PATCH on a document which misses a field having a default value will now result in setting this value, even if the field was not provided in the PATCH's payload. - Error messages for `keyschema` are now returned as dictionary. Before: {'propertyschema_dict': 'propertyschema_dict'} Now: {'keyschema_dict': {'AAA': "value does not match regex '[a-z]+'"}} - Error messages for `type` validations are different now (following changes in Cerberus). - It is no longer valid to have a field with `default` = None and `nullable` = False. (see patch.py:test_patch_nested_document_nullable_missing) In a nutshell, changes to the codebase are as follows: - Add data layer independent subclass of `cerberus.Validator` * Support new signature of `__init__` and `validate` * Use `_config`-aware properties instead of bare member attributes to pass the `resource`, `document_id` and `persisted_document` to make them available to child validators * Add schema-docstrings to all `_validate_*` methods - Adjust Mongo-specific `Validator` subclass * Adjust `_validate_type_*` methods (following changes in Cerberus) * Add schema-docstrings to all `_validate_*` methods - Add custom ErrorHandler to support `VALIDATION_ERROR_AS_LIST` - A few renames: * `ValidationError` -> `DocumentError` * `propertyschema` -> `keyschema` and `keyschema` -> `valueschema` - Adjust tests due to different validation error messages (mostly for `type`) - Remove `transparent_schema_rules` without replacement - Remove `default`-handling, as Cerberus takes care of this now
@nicolaiarocci Updated |
Merged after rebase,see 3bfff77. Stellar, thank you. |
(Based on original patch from dkellner)
This is a rather big change. I still decided to do a single commit, as
intermediate commits would be in a non-working state anyway.
Breaking changes:
keyschema
was renamed tovalueschema
andpropertyschema
tokeyschema
(following changes in Cerberus).now result in setting this value, even if the field was not provided
in the PATCH's payload.
keyschema
are now returned as dictionary.Before: {'propertyschema_dict': 'propertyschema_dict'}
Now: {'keyschema_dict': {'AAA': "value does not match regex '[a-z]+'"}}
type
validations are different now (followingchanges in Cerberus).
default
= None andnullable
= False.(see patch.py:test_patch_nested_document_nullable_missing)
In a nutshell, changes to the codebase are as follows:
Add data layer independent subclass of
cerberus.Validator
__init__
andvalidate
_config
-aware properties instead of bare member attributes topass the
resource
,document_id
andpersisted_document
to makethem available to child validators
_validate_*
methodsAdjust Mongo-specific
Validator
subclass_validate_type_*
methods (following changes in Cerberus)_validate_*
methodsAdd custom ErrorHandler to support
VALIDATION_ERROR_AS_LIST
A few renames:
ValidationError
->DocumentError
propertyschema
->keyschema
andkeyschema
->valueschema
Adjust tests due to different validation error messages
(mostly for
type
)Remove
transparent_schema_rules
without replacementRemove
default
-handling, as Cerberus takes care of this now