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

`readonly` conflicts with `default` #268

Closed
dkellner opened this Issue Sep 15, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@dkellner
Copy link
Contributor

dkellner commented Sep 15, 2016

Used Cerberus version / latest commit: 1.0.1 / ebd281b

  • I consulted these documentations:
  • I consulted these sections of the docs to see if there is any mention of this behaviour or any warning about using them together:
    • readonly
    • default
  • I found nothing relevant to my problem in the docs.
  • I found the documentation not helpful to my problem.
  • I have the capacity to improve the docs when my problem is solved.
  • I have the capacity to submit a patch when a bug is identified.

Bug report

Documents always fail validation when default and readonly are used on the same field.

This is because default value handling / normalization will happen before the readonly validator is called and the latter will therefore always fail, because the document will always contain the field. I think we should allow validators to be processed before normalization and maybe another list like priority_validations for those rules would be useful.

I've started working on this already, but of course any comments are appreciated :).

Failing test

    def test_readonly_field_with_default_value(self):
        schema = {
            'created': {
                'type': 'string',
                'readonly': True,
                'default': 'today'
            }
        }
        self.assertSuccess({}, schema)
        self.assertFail({'created': 'tomorrow'}, schema)
        self.assertFail({'created': 'today'}, schema)
@funkyfuture

This comment has been minimized.

Copy link
Member

funkyfuture commented Sep 17, 2016

well, yes. they are conflicting. by definition. it's okay.

nothing hinders you to run various validations and normalizations in any arbritrary order. i would even suggest that this is a good practice when schemas are getting complex.

dkellner added a commit to dkellner/cerberus that referenced this issue Sep 20, 2016

dkellner added a commit to dkellner/cerberus that referenced this issue Sep 20, 2016

@dkellner

This comment has been minimized.

Copy link
Contributor

dkellner commented Sep 20, 2016

I've encountered this issue because I'm integrating Cerberus 1.0 in Eve: https://github.com/nicolaiarocci/eve/issues/776

Eve currently has its own default-rule supporting the combination with readonly with the behaviour described by the test above. Of course we can always discuss about dropping a corner-case feature like this, but in this case I would update docs and maybe even let the schema validation fail.


I have some working code allowing default and readonly already. In a nutshell, the needed change is the following. Right now we are doing two "steps" (in validate with normalize=True):

  1. Normalization of the complete document, incl. subdocuments
  2. Validation of the complete document, incl. subdocuments

If we want to have fields with both readonly and default, we have to do:

  1. Validation, but just process only readonly-rules and ignore the rest (but still recursing down all subdocuments, because normalization will do the same!)
  2. Normalization
  3. Validation of all rules except readonly

If you are interested, you can see the code here: https://github.com/dkellner/cerberus/tree/default-readonly. There is still one failing test, because the first validator (processing "only" readonly) needs to run the recursing rules like items and schema etc. too, but they not just recurse but can fail by themselves, too. But I did want to hear your thoughts first before investing more time on this.

So the important question is: Do we want to allow default and readonly on the same field? In this case I would clean up my code, do some further refactoring and create a PR. If not, I will at least update the documentation and create a PR, too.


One further question to your "good practice"-suggestion: Do you mean running a validator with just read-only checks and allow_unknown=True first, and then running another validator with normalization and the other stuff? I guess this would work, but I kind of like having just one schema describing how my data looks like.

@nicolaiarocci

This comment has been minimized.

Copy link
Member

nicolaiarocci commented Sep 20, 2016

Do we want to allow default and readonly on the same field?

My answer would be Yes.

dkellner added a commit to dkellner/eve that referenced this issue Sep 26, 2016

Support for Cerberus 1.0 (work in progress)
"Work in progress" because we still have to wait for the Cerberus issue
"`readonly` conflicts with `default`" to be resolved.
(see pyeve/cerberus#268)

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

dkellner added a commit to dkellner/cerberus that referenced this issue Sep 27, 2016

dkellner added a commit to dkellner/cerberus that referenced this issue Sep 27, 2016

dkellner added a commit to dkellner/cerberus that referenced this issue Sep 28, 2016

Fix pyeve#268: `readonly` conflicts with `default`
The fix uses the newly introduced `rule_filter` to process validation of
'read-only' before normalization and all other rules afterwards.

dkellner added a commit to dkellner/cerberus that referenced this issue Sep 28, 2016

dkellner added a commit to dkellner/cerberus that referenced this issue Nov 1, 2016

@funkyfuture

This comment has been minimized.

Copy link
Member

funkyfuture commented Nov 8, 2016

i'm not sure if I understand the use-case from the tests above. but wouldn't it suffice to update the document with the default values in the client code before it is processed by cerberus?

dkellner added a commit to dkellner/cerberus that referenced this issue Nov 12, 2016

@nicolaiarocci nicolaiarocci added this to the Unreleased milestone Nov 14, 2016

dkellner added a commit to dkellner/cerberus that referenced this issue Nov 16, 2016

bcrochet added a commit to bcrochet/eve that referenced this issue Feb 13, 2017

Support for Cerberus 1.0 (work in progress)
"Work in progress" because we still have to wait for the Cerberus issue
"`readonly` conflicts with `default`" to be resolved.
(see pyeve/cerberus#268)

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment