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 works properly just in the first validation #311

Closed
mbarakaja opened this Issue Apr 20, 2017 · 7 comments

Comments

Projects
None yet
4 participants
@mbarakaja
Copy link

mbarakaja commented Apr 20, 2017

Given the next code:

from cerberus import Validator


v = Validator({
    'id': {'type': 'integer', 'readonly': True},
})


v.validate({'id': 1}) # False
v.validate({'id': 1}) # True
v.validate({'id': 1}) # True
v.validate({'id': 1}) # True

Just the first call validates correctly and return False. The subsequent calls to validate(...) return True. This just happen to me with property readonly. It looks like the validation works just on time.

Is this an intentional behavior of this library?

@funkyfuture

This comment has been minimized.

Copy link
Member

funkyfuture commented Apr 21, 2017

what is v.errors after the first call?

@mbarakaja

This comment has been minimized.

Copy link

mbarakaja commented Apr 21, 2017

@funkyfuture

The first call sets v.errors to {'id': ['field is read-only']}. Subsecuent calls set it to an empty dictionary {}.

@funkyfuture

This comment has been minimized.

Copy link
Member

funkyfuture commented Apr 21, 2017

just scratching at the surface, and i'm pretty sure that is caused by the readonly / default thing.

i changed is_normalized to a private property in order to not get it inherited by child-validators and made it being set and unset cosistently. that solve this issue, but breaks test_nested_readonly_field_with_default_value. cc @dkellner

@nicolaiarocci thus i'd say this is a show-stopper for a 1.2 release.

my tinkering so far:

diff --git a/cerberus/tests/test_validation.py b/cerberus/tests/test_validation.py
index ec97de5..155c89b 100644
--- a/cerberus/tests/test_validation.py
+++ b/cerberus/tests/test_validation.py
@@ -145,6 +145,13 @@ def test_nested_readonly_field_with_default_value():
                 schema, errors=expected_errors)
 
 
+def test_repeated_readonly(validator):
+    # https://github.com/pyeve/cerberus/issues/311
+    validator.schema = {'id': {'readonly': True}}
+    assert_fail({'id': 0}, validator=validator)
+    assert_fail({'id': 0}, validator=validator)
+
+
 def test_not_a_string():
     assert_bad_type('a_string', 'string', 1)
 
diff --git a/cerberus/validator.py b/cerberus/validator.py
index f28f74c..b8edca7 100644
--- a/cerberus/validator.py
+++ b/cerberus/validator.py
@@ -410,14 +410,14 @@ class Validator(object):
         Type: :class:`bool` """
         return self._config.get('is_child', False)
 
-    @property
-    def is_normalized(self):
-        """ ``True`` if the document is already normalized. """
-        return self._config.get('is_normalized', False)
-
-    @is_normalized.setter
-    def is_normalized(self, value):
-        self._config['is_normalized'] = value
+    # @property
+    # def is_normalized(self):
+    #     """ ``True`` if the document is already normalized. """
+    #     return self._config.get('is_normalized', False)
+    #
+    # @is_normalized.setter
+    # def is_normalized(self, value):
+    #     self._config['is_normalized'] = value
 
     @property
     def purge_unknown(self):
@@ -548,6 +548,7 @@ class Validator(object):
             return self.document
 
     def __normalize_mapping(self, mapping, schema):
+        self._is_normalized = False
         if isinstance(schema, _str_type):
             schema = self._resolve_schema(schema)
         schema = schema.copy()
@@ -564,6 +565,7 @@ class Validator(object):
         self.__normalize_default_fields(mapping, schema)
         self._normalize_coerce(mapping, schema)
         self.__normalize_containers(mapping, schema)
+        self._is_normalized = True
         return mapping
 
     def _normalize_coerce(self, mapping, schema):
@@ -816,7 +818,8 @@ class Validator(object):
         self.__init_processing(document, schema)
         if normalize:
             self.__normalize_mapping(self.document, self.schema)
-            self.is_normalized = True
+        else:
+            self._is_normalized = False
 
         for field in self.document:
             if self.ignore_none_values and self.document[field] is None:
@@ -1113,7 +1116,7 @@ class Validator(object):
     def _validate_readonly(self, readonly, field, value):
         """ {'type': 'boolean'} """
         if readonly:
-            if not self.is_normalized:
+            if not self._is_normalized:
                 self._error(field, errors.READONLY_FIELD)
             # If the document was normalized (and therefore already been
             # checked for readonly fields), we still have to return True
@@ -1121,7 +1124,7 @@ class Validator(object):
             has_error = errors.READONLY_FIELD in \
                 self.document_error_tree.fetch_errors_from(
                     self.document_path + (field,))
-            if self.is_normalized and has_error:
+            if self._is_normalized and has_error:
                 self._drop_remaining_rules()
 
     def _validate_regex(self, pattern, field, value):
@funkyfuture

This comment has been minimized.

Copy link
Member

funkyfuture commented May 2, 2017

@nicolaiarocci can you please add a bug label and assign this issue to the 1.2 milestone? i'm in fear it may be forgotten otherwise as there has been no feedback so far.

@nicolaiarocci nicolaiarocci added the bug label May 2, 2017

@nicolaiarocci nicolaiarocci added this to the 1.2 milestone May 2, 2017

@nicolaiarocci

This comment has been minimized.

Copy link
Member

nicolaiarocci commented May 2, 2017

yeah sorry about that

@dkellner

This comment has been minimized.

Copy link
Contributor

dkellner commented May 7, 2017

I'll look into this. The main reason for this is that we're treating readonly as a normalization rule (and not as other validation rules). My first proposed solution for default+readonly (#272) does not suffer from this problem.

@dkellner

This comment has been minimized.

Copy link
Contributor

dkellner commented May 7, 2017

It seems to be enough to reset is_normalized when a new document is coming in - all tests are passing now. The fact that this actually was not done before makes me wonder if there exist more bugs because of this. I will investigate further.

@funkyfuture What do you think about this?

diff --git a/cerberus/tests/test_validation.py b/cerberus/tests/test_validation.py
index ec97de5..2c31a74 100644
--- a/cerberus/tests/test_validation.py
+++ b/cerberus/tests/test_validation.py
@@ -85,6 +85,13 @@ def test_readonly_field_first_rule():
     assert 'read-only' in v.errors['a_readonly_number'][0]
 
 
+def test_repeated_readonly(validator):
+    # https://github.com/pyeve/cerberus/issues/311
+    validator.schema = {'id': {'readonly': True}}
+    assert_fail({'id': 0}, validator=validator)
+    assert_fail({'id': 0}, validator=validator)
+
+
 def test_readonly_field_with_default_value():
     schema = {
         'created': {
diff --git a/cerberus/validator.py b/cerberus/validator.py
index f28f74c..fd6b6eb 100644
--- a/cerberus/validator.py
+++ b/cerberus/validator.py
@@ -492,6 +492,8 @@ class Validator(object):
         self.document_error_tree = errors.DocumentErrorTree()
         self.schema_error_tree = errors.SchemaErrorTree()
         self.document = copy(document)
+        if not self.is_child:
+            self.is_normalized = False
 
         if schema is not None:
             self.schema = DefinitionSchema(self, schema)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment