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

Add default normalization rule #172

Merged
merged 6 commits into from
Nov 25, 2015

Conversation

dnohales
Copy link
Contributor

This is for #131, it adds the default normalization rule to add missing fields in the document. Few examples:

>>> schema = {'amount': {'type': 'integer'}, 'kind': {'type': 'string', 'default': 'purchase'}}
>>> v.validated({'amount': 1}, schema)
{'amount': 1, 'kind': 'purchase'}
>>> v.validated({'amount': 1, 'kind': 'other'}, schema)
{'amount': 1, 'kind': 'other'}
>>> v.validated({'amount': 1, 'kind': None}, schema)
{'amount': 1, 'kind': None}
>>> schema = {'thing': {'type': 'dict', 'schema': {'kind': {'type': 'string', 'default': 'purchase'}}
>>> v.validated({'thing': {}}, schema)
{'thing': {'kind': 'purchase'}}

Concerns and thoughts

  • If nullable is False, value is None and default is present, should we override the field with default or throw a validation error?
  • If default is a callable, should we run the callable and set the callable result? Do we need another option like default_is_callable that when is True we call the default value if is a callable and when is False we don't call anything? We also can always call the default value if is callable and if the user really want to set a function definition as a default value for the document, she could wrap it with a lambda function or something like that.

@funkyfuture
Copy link
Member

cool. cool. cool.

please also extend the documentation. your examples above are fine to be added as .. doctest::. maybe except the last.

If nullable is False, value is None and default is present, should we override the field with default or throw a validation error?

i tend to overriding a None. since nullable can still prevent it, it seems safe to me.

If default is a callable, should we run the callable and set the callable result?

what would be a usecase for that?
i can think of it like a method of a custom validator (thus with context access). (like #102)

@@ -454,6 +454,7 @@ def __normalize_mapping(self, mapping, schema):
if self.purge_unknown:
self._normalize_purge_unknown(mapping, schema)
self._normalize_coerce(mapping, schema)
self._normalize_default(mapping, schema)
Copy link
Member

Choose a reason for hiding this comment

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

though i have only one silly argument for setting defaults before coercing, here it is: it seems more intuitive to normalize in that order: purge unwanted items, fill up missing items, work on items.

but maybe it's be useful to coerce a value to None in order to replace it with a default. coercing defaults on the other hand shouldn't make sense.

so propably it's better to keep it like it is. but i'd be interested in other thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

but maybe it's be useful to coerce a value to None in order to replace it with a default. coercing defaults on the other hand shouldn't make sense.

Agreed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but maybe it's be useful to coerce a value to None in order to replace it with a default.

But as told by @nicolaiarocci and I also think the same, using default to replace None seems odd, specially if None is a valid value (nullable fields for example). But I know that maybe becomes handy, that's why I proposed to only override Nones with default only for non nullable fields, but the @nicolaiarocci argument in the comment below seems valid too.

coercing defaults on the other hand shouldn't make sense.

It shouldn't but I don't know if it hurts, maybe it's handy and safer.

@nicolaiarocci
Copy link
Member

If nullable is False, value is None and default is present, should we override the field with default or throw a validation error?

I'd say that in this case the user explicitly wants a None for the field, so a validation error would probably be more appropriate (let the user know that's not allowed). If we replace it with a default value it might seem it has been accepted while it has not (I'm thinking Eve use case for example, where a client sends a payload to the remote REST API and gets it accepted - I'd probably want to know that None is not allowed).

If default is a callable, should we run the callable and set the callable result?

I'm unsure about this. As @funkyfuture says, there are other ways to achieve the same result already. I probably prefer a static default value.

@dnohales
Copy link
Contributor Author

If default is a callable, should we run the callable and set the callable result?

what would be a usecase for that?

I was particularly thinking in something like:

{'a_datetime': {'default': utcnow}}

Then if a_datetime is not present in the document, you set the utcnow result, not the callable.

But yeah, this over-complicate the feature and makes it more unintuitive. I really don't understand which other way you have to accomplish this, can you give an example?

Damián Nohales added 2 commits November 20, 2015 10:05
This method allow to validate a document for a specific schema/validator
and then compare the normalized document to another document in one
single call.
@dnohales
Copy link
Contributor Author

Updates: added documentation and assertNormalizedEqual method.

@nicolaiarocci
Copy link
Member

Very nice. However this does not seem to work with lists of dictionaries.

schema = {
    'mylist': {
        'type': 'list', 
        'schema': {
            'type': 'dict', 
            'schema': {
                'customer': {'type': 'boolean', 'default': False}, 
                'name': {'type': 'string'}
            }
       }
    }
}

Blocks with: SchemaError: unknown rule 'customer' for field 'customer'

@funkyfuture
Copy link
Member

Then if a_datetime is not present in the document, you set the utcnow result, not the callable.

But yeah, this over-complicate the feature and makes it more unintuitive. I really don't understand which other way you have to accomplish this, can you give an example?

i don't think this is over-complicated. and i think that's an interesting use-case, so this may come up at some point anyway.
there is no other way to accomplish that now. i didn't mean that there exists one, but that it should be done like custom validation methods. that i could imagine yesterday as use-case, as it would allow context-sensitive defaults.

If we replace it with a default value it might seem it has been accepted while it has not ... I'd probably want to know that None is not allowed

what about the following:
field value is None, nullable=True -> default is ignored
field value is None, nullable=False -> field value is set to default value, an 'error' is emitted

in conjunction with an ability to ignore specific errors that'd allow flexible usage.

However this does not seem to work with lists of dictionaries.

i think that may be #168.

@nicolaiarocci
Copy link
Member

Let's do it this way:

field value is None, nullable=True -> default is ignored
field value is None, nullable=False -> an 'error' is emitted

@nicolaiarocci
Copy link
Member

For the record, I have a default implementation in Eve and I am looking forward to replace that one with this native Cerberus implementation.

@funkyfuture
Copy link
Member

@nicolaiarocci and Eve's default wouldn't touch a None, so my proposal would break it?

@dnohales concerning the non-static defaults: that could be achieved if coercing happened after setting defaults; so default would ensure that a field is present in a document and could be altered dynamically with a custom coerce method. (in this case coercing defaults becomes useful as a workaround.)

i'm curious about this question: are dynamic defaults still in the realm of normalization or is it generating data? and thus, does it belong into a library with a focus on validation?

@nicolaiarocci
Copy link
Member

Errors should be show stoppers in my opinion. Especially so when reported by a validation tool. Furthermore, nullable = False is a very explicit and clear rule.

In Eve when Nullable=False we return an error and don't process the payload. If Nullable=True we accept the null. In both cases we ignore the default rule, which only kicks in when field is missing.

@dnohales
Copy link
Contributor Author

So, I updated the code. Let me know if you agree with the changes (specially the latest commit), since I see some discrepancies about the compatibility with Eve.

@dnohales
Copy link
Contributor Author

But yeah, I agree with @nicolaiarocci regarding the nullable matter.

@nicolaiarocci nicolaiarocci merged commit 71ccc39 into pyeve:master Nov 25, 2015
nicolaiarocci added a commit that referenced this pull request Nov 25, 2015
@nicolaiarocci
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants