`readonly` conflicts with `default` ("simple" fix) #282

dkellner · 2016-11-12T19:45:43Z

This is a careful fix for issue #268 without introducing any new features (like in #272) and without any (known) breaking changes.

For the full discussion see #272.

funkyfuture

nice and tidy.

funkyfuture · 2016-11-12T22:52:09Z

cerberus/validator.py

@@ -665,6 +678,12 @@ def _normalize_rename_handler(self, mapping, schema, field):
            mapping[new_name] = mapping[field]
            del mapping[field]

+    def __validate_readonly_fields(self, mapping, schema):
+        for field in [x for x in schema if x in mapping and


i'd prefer a generator- over a list-comprehension here.

You're right, I will change that.

funkyfuture · 2016-11-12T22:54:18Z

cerberus/validator.py

+            # If the document was normalized (and therefore already been
+            # checked for readonly fields), we still have to return True
+            # if an error was filed if readonly is a priority rule.
+            if 'readonly' in self.priority_validations:


isn't that always true? or is this different in the eve-validator?

No, I did not think about Eve at all when implementing this. There is this "configuration" for priority validations, so I thought it's best to reuse it instead of hardcoding the fact. But thinking about that again it's special anyway because returning True is just something priority validations do. I will remove that check.

funkyfuture · 2016-11-12T22:57:29Z

cerberus/validator.py

+            # checked for readonly fields), we still have to return True
+            # if an error was filed if readonly is a priority rule.
+            if 'readonly' in self.priority_validations:
+                has_error = self.schema_error_tree.fetch_errors_from(


shall any (or a prior readonly) error be respected here?

Just a prior readonly error on this particular field. I think it's more clean and specific when the readonly-logic just cares about it's own errors. Do you see any problems with that approach?

no problem with the approach, but that would be errors.READONLY_FIELD in self.schema_error_tree....

Are you sure? I thought the SchemaErrorTree allows to access errors for certain rules. In fact, I just tried that and indeed only get the error for readonly:

d = errors.DocumentErrorTree() s = errors.SchemaErrorTree() e = errors.ValidationError(('a'), ('a', errors.READONLY_FIELD.rule), errors.READONLY_FIELD.code, errors.READONLY_FIELD.rule, None, None, None) f = errors.ValidationError(('a'), ('a', errors.MIN_VALUE.rule), errors.MIN_VALUE.code, errors.MIN_VALUE.rule, None, None, None) [tree.add(error) for tree in (d, s) for error in (e, f)] # DocumentErrorTree returns all errors for the given field. print(d.fetch_errors_from(('a',))) >> [ValidationError @ 0x7f7686359ed0 ( document_path=a,schema_path=('a', 'min'),code=0x42,constraint=None,value=None,info=None ), ValidationError @ 0x7f76874f2150 ( document_path=a,schema_path=('a', 'readonly'),code=0x63,constraint=None,value=None,info=None )] # SchemaErrorTree returns just the readonly error. print(s.fetch_errors_from(('a', 'readonly')))) >> [ValidationError @ 0x7f76874f2150 ( document_path=a,schema_path=('a', 'readonly'),code=0x63,constraint=None,value=None,info=None )]

sorry, my mistake. i confused it with the document tree. but i learned that there can be more than one iteraton in a comprehension from your example. :-)

shower thought: wouldn't a query on the document error tree be safer with regards to implicit validation like from allow_unknown?

I'm not sure if an unknown field can ever be readonly in a meaningful way. I find querying the schema tree much clearer to understand, but that's maybe just my way of thinking. If you agree I would like to keep it this way :).

i have no preference here. you are right that the example case is hardly meaningful, but people come up with uses the issue tracker i'd never thought of, regularly.
my way of thinking here is: schemas can be tricky constructs and sometimes rules are evaluated that have not been adressed directly. documents on the other hand are clearer, in the document error tree you will find all errros associated with a field at exactly one node and nowhere else. it seems less error prone to me for the rather weird cases. and as mentioned above, someone will come up with something weird (which is charming).
why would you say the scheme error tree is clearer to understand? genetically they are the same.

I did not mean my understanding of both trees, but the code readability at that specific location. But we don't need a deep discussion about this small issue, I am happy to go with your suggestion as well - you are way more experienced with the Cerberus codebase than I am, so I trust you with that.

funkyfuture · 2016-11-13T13:33:07Z

some elaboration on this in the docs would be good.

nicolaiarocci · 2016-11-14T06:41:24Z

Very nice. Agree that a docs upgrade would be good.

dkellner · 2016-11-16T16:45:32Z

Yep, I will update the docs later today and update this PR so we can hopefully close this issue soon.

dkellner · 2016-11-16T20:46:47Z

Looks like the pypy3 test fails for some technical reason. @nicolaiarocci: Can you fix that?

funkyfuture · 2016-11-16T22:19:47Z

docs/normalization-rules.rst

@@ -90,6 +90,14 @@ string, it points to a :doc:`custom method <customize>`.
   >>> v.errors
   {'a': ["default value for 'a' cannot be set: Circular dependencies of default setters."]}

+You can even use both `default` and `readonly` on the same field. This will


don't let this bit delay this pr to be merged, but a link do the other doc on each side would be handy as amendment.
other rule names are commonly enclosed by double backticks for monospaced appearance.
is the feature/fix worthenough to extend the ~~docs~~ edit: examples?

I will look into that. First I finally have to setup Sphinx to compile the docs locally ;).

funkyfuture · 2016-11-16T22:20:20Z

docs/validation-rules.rst

@@ -427,6 +427,9 @@ present in the target dictionary. This is useful, for example, when receiving
 a payload which is to be validated before it is sent to the datastore. The field
 might be provided by the datastore, but should not writable.

+.. versionchanged:: 1.0.2
+   Can be used in conjunction with `default`.


and default_setter

funkyfuture · 2016-11-16T22:28:31Z

this issue may be of interested concerning PyPy3 on travis.

the tests succeed locally.

nicolaiarocci · 2016-11-19T08:37:10Z

I temporarily removed pypy3 from CI matrix, I'll just test PRs locally until it's fixed upstream.

nicolaiarocci · 2016-11-19T08:43:45Z

Merged after rebase: 7408da5

Thanks!

funkyfuture reviewed Nov 12, 2016

View reviewed changes

nicolaiarocci mentioned this pull request Nov 14, 2016

readonly conflicts with default (and rule_filter) #272

Closed

Add failing tests for pyeve#268: readonly conflicts with default

806e04d

dkellner force-pushed the normalize-readonly branch from 97a0d85 to c90acaf Compare November 16, 2016 20:40

funkyfuture approved these changes Nov 16, 2016

View reviewed changes

Check readonly before default if necessary

d675240

dkellner force-pushed the normalize-readonly branch from c90acaf to d675240 Compare November 17, 2016 19:44

nicolaiarocci added a commit that referenced this pull request Nov 19, 2016

Changelog for #282

5a50262

nicolaiarocci closed this Nov 19, 2016

dkellner deleted the normalize-readonly branch May 7, 2017 03:45

dkellner mentioned this pull request Feb 14, 2018

readonly param designed behaviour is broken with oneof, anyof #297

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`readonly` conflicts with `default` ("simple" fix) #282

`readonly` conflicts with `default` ("simple" fix) #282

dkellner commented Nov 12, 2016

funkyfuture left a comment

funkyfuture Nov 12, 2016

dkellner Nov 12, 2016

funkyfuture Nov 12, 2016

dkellner Nov 12, 2016

funkyfuture Nov 12, 2016

dkellner Nov 12, 2016

funkyfuture Nov 12, 2016

dkellner Nov 13, 2016

funkyfuture Nov 13, 2016

funkyfuture Nov 13, 2016

dkellner Nov 16, 2016

funkyfuture Nov 16, 2016 •

edited

Loading

dkellner Nov 16, 2016

funkyfuture commented Nov 13, 2016

nicolaiarocci commented Nov 14, 2016

dkellner commented Nov 16, 2016

dkellner commented Nov 16, 2016

funkyfuture Nov 16, 2016 •

edited

Loading

dkellner Nov 17, 2016

funkyfuture Nov 16, 2016

funkyfuture commented Nov 16, 2016 •

edited

Loading

nicolaiarocci commented Nov 19, 2016

nicolaiarocci commented Nov 19, 2016 •

edited

Loading

readonly conflicts with default ("simple" fix) #282

readonly conflicts with default ("simple" fix) #282

Conversation

dkellner commented Nov 12, 2016

funkyfuture left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funkyfuture Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funkyfuture commented Nov 13, 2016

nicolaiarocci commented Nov 14, 2016

dkellner commented Nov 16, 2016

dkellner commented Nov 16, 2016

funkyfuture Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funkyfuture commented Nov 16, 2016 • edited Loading

nicolaiarocci commented Nov 19, 2016

nicolaiarocci commented Nov 19, 2016 • edited Loading

`readonly` conflicts with `default` ("simple" fix) #282

`readonly` conflicts with `default` ("simple" fix) #282

funkyfuture Nov 16, 2016 •

edited

Loading

funkyfuture Nov 16, 2016 •

edited

Loading

funkyfuture commented Nov 16, 2016 •

edited

Loading

nicolaiarocci commented Nov 19, 2016 •

edited

Loading