add `require_all` validator argument #421

pohmelie · 2018-07-07T13:24:58Z

pohmelie · 2018-07-07T13:28:36Z

The problem place resolved badly, since I'm not familiar with codebase. Need help here.

funkyfuture · 2018-07-07T17:29:39Z

i don't have the time to look at it now, but i think a complementary rule like the allow_unknown rule is necessary because people will certainly have use-cases for this (where an overall option is too restrictive like allow_unknown was).

funkyfuture

beside the mentioned accompanying rule for subdocuments this needs tests. and not few of them. please mind at least theses aspects:

fields with overriding 'required': False
uses in conjunction with excludes
proper error data

funkyfuture · 2018-07-08T17:34:38Z

cerberus/validator.py

@@ -84,6 +84,9 @@ class BareValidator(object):
    :param allow_unknown: See :attr:`~cerberus.Validator.allow_unknown`.
                          Defaults to ``False``.
    :type allow_unknown: :class:`bool` or any :term:`mapping`
+    :param require_all: See :attr:`~cerberus.Validator.require_all`.
+                          Defaults to ``False``.


this has a surplus of indentation.

funkyfuture · 2018-07-08T17:37:08Z

cerberus/validator.py

@@ -281,7 +287,7 @@ def _error(self, *args):
                if rule == 'nullable':
                    constraint = field_definitions.get(rule, False)
                else:
-                    constraint = field_definitions[rule]
+                    constraint = field_definitions.get(rule)


is this a special case for required? i'd rather keep that prone to a KeyError exception than have possibly incorrect None values in the error data.

Is it correct?

if rule == 'nullable': constraint = field_definitions.get(rule, False) elif rule == 'required': constraint = field_definitions.get(rule, self.require_all) else: constraint = field_definitions[rule]

looks good.

funkyfuture · 2018-07-08T17:40:14Z

cerberus/validator.py

+
+    @require_all.setter
+    def require_all(self, value):
+        if not (self.is_child or isinstance(value, (bool, DefinitionSchema))):


you musn't copy that from allow_unknown where it has the purpose to validate the provided constraint.

require_all should not have setter? I'm not familiar with internals, sorry.

it should have a setter. but the check in line 447-448 is specific to allow_unknown. just assign the value like purge_readonly for example.

funkyfuture

i really tend to insist on a full check of the submitted error data.

ValidationError has the property schema_path that references the failed rule in the used schema. when the cause of the failure is defined by the validator option, i propose to set that property to __require_all__ similar to here.

funkyfuture · 2018-07-11T19:33:21Z

cerberus/tests/test_validation.py

+
+def test_require_all_simple():
+    v = Validator({'foo': {'type': 'string'}}, require_all=True)
+    assert not v({})


instead of assert statements you could use the helper functions assert_fail and assert_success that are more helpful for debugging. with the function that expects a failure one can also pass the expected error data, there are some uses of this as reference.

funkyfuture · 2018-07-11T19:41:19Z

cerberus/tests/test_validation.py

+def test_require_all_errors():
+    v = Validator({'foo': {'type': 'string'}}, require_all=True)
+    assert not v({})
+    assert v.errors == {'foo': ['required field']}


a more detailed test (one should be enough) of the error data (all the properties of a ValidationError) with the functionality mentioned before would be preferable.

your test also checks the result of the default error handler, its behaviour could change and that wouldn't relate to your feature / these tests.

pohmelie · 2018-07-12T11:34:21Z

@funkyfuture

__require_all__ similar to here

Sorry, have no idea what is going on here and how to make some similar changes to __validate_required_fields method. Need help.

funkyfuture · 2018-07-12T17:42:20Z

cerberus/tests/test_validation.py

+        {},
+        schema,
+        validator,
+        error=('foo', ('foo', 'required'), errors.REQUIRED_FIELD, True),


as a start replace the second tuple value here with '__require_all__'. as far as i can oversee the situation, the schema_path variable (which is eventually compared with that value) in the Validator._error method must then be figured out with regards to the presence of the required rule for the submitted field in the schema when an errors.REQUIRED_FIELD is processed.

That is obvious... and this is test, I mean, at first we need to fix code, so it will result __require_all__ in error schema path and only after tests should be fixed. The problem is to fix validator.py and replace required with __require_all__ in __validate_requried_fields in some cases.

and this is test, I mean, at first we need to fix code

following the paradigm of test-driven development, one first formulates assertions in a test.

__validate_required_fields calls _error which only takes a reference to the field and an error definition as argument, hence the detemination of the correct (pseudo-) schema path has to happen in the latter method.

following the paradigm of test-driven development, one first formulates assertions in a test.

Ok, but this does not help me at all. TDD is for developers, this PR won't be accepted without tests and implementation, so there is no reason to order changes.

hence the detemination of the correct (pseudo-) schema path has to happen in the latter method.

I don't get it. We have __validate_unknown_fields, which calls _error with some preparation and we have __validate_required_fields, which calls _error without something. Why this cases are not equal? What is "later method" in this case?

i don't get it.

TDD is for developers, this PR won't be accepted without tests and implementation, so there is no reason to order changes.

afaict you are developing a feature here, hence you're in the role of a developer. i don't care how you work if the result is fine from a variety of angles, one of these are tests that define expected behaviour and are passing. at the moment the test defines an undesired behaviour.

Why this cases are not equal?

__validate_unknown_fields uses a 'child validator' that handles the schema path (that may eventually be used in _error).

__validate_required_fields, which calls _error without something

no. that never happens.

What is "later method" in this case?

the latter that was mentioned in the previous part in the sentence, _error.

if the result is fine from a variety of angles, one of these are tests that define expected behaviour and are passing. at the moment the test defines an undesired behaviour.

That is the point. I understand what behaviour we need, but I don't understand how to do it. And advice to "start with test" sound like a joke. 😄

the latter that was mentioned in the previous part in the sentence, _error.

You suggest to add __require_all__ to schema_path in _error method, am I right?

And advice to "start with test" sound like a joke.

neither did i intend to be funny with that statement nor can i find traces of humour in it.

so far, my successes with that approach were really fun, but not laughable.

You suggest to add __require_all__ to schema_path in _error method, am I right?

more specificly: __require_all__ is the schema_path when the error was defined by the require_all option of the root validator.

funkyfuture · 2018-07-23T19:24:32Z

a hint on a not so obvious spot: the third paragraph in schema.rst also needs to be updated.

pohmelie · 2018-07-24T11:49:42Z

@funkyfuture
This paragraph? How this should be updated?

funkyfuture · 2018-07-25T15:28:32Z

no, i mean the third paragraph in the first section.

on that occasion: what's your overall status on this feature?

pohmelie · 2018-07-25T23:03:30Z

on that occasion: what's your overall status on this feature?

Sorry, I'm not native english speaker, so I don't get you... As I understand you, then answer is: «I can't check readiness of this feature, since I'm contributor and you are reviewer. It's up to you do decide, that my implementation is good enough or not».

funkyfuture · 2018-07-29T15:55:37Z

i'm not a native english speaker as well (okay, i have an advantage w/ my mother tongue) and even as a citizen of the gdr i didn't learn russian. it seems we have no other choice than using the international language of science and technology. maybe you can get support with that.

regarding the documentation i can provide corrected formulations when reviewing.

so far the changes look good, but the whole feature is still lacking these to major aspects:

an require_all rule that can override the configuration value of an instance when a subdocument is validated with schema
an enhancement of the documentation in all appropriate places

again, the allow_unknown rule can give good orientation on both, but it's not identical.

funkyfuture · 2018-08-04T13:48:17Z

cerberus/schema.py

@@ -67,6 +67,7 @@ def __init__(self, validator, schema={}):
        self.schema_validator = SchemaValidator(
            None,
            allow_unknown=self.validation_schema,
+            require_all=self.validation_schema,


what is this assigning of a schema supposed to achieve?

funkyfuture · 2018-08-04T13:48:57Z

cerberus/schema.py

@@ -275,6 +276,7 @@ class SchemaValidationSchema(UnvalidatedSchema):
    def __init__(self, validator):
        self.schema = {
            'allow_unknown': False,
+            'require_all': False,


there's no need to set the default again.

funkyfuture · 2018-08-04T13:50:37Z

cerberus/tests/test_validation.py

+    sub_schema = {'foo': {'type': 'string'}}
+    schema = {'foo': {'type': 'dict', 'require_all': True, 'schema': sub_schema}}
+    validator = Validator(require_all=False)
+    assert_success({'foo': {'foo': 'bar'}}, schema, validator)


using the same field name at different levels make debugging much harder.

funkyfuture · 2018-08-04T13:53:40Z

cerberus/tests/test_validation.py

+    assert_success({'foo': 'bar'}, schema, validator)
+
+
+def test_require_all_override_by_subdoc_require_all():


could this test be better readable by parametrizing it? at least adding empty lines to group the setup and test of different circumstances should make it better comprehendable.

funkyfuture · 2018-08-04T13:56:22Z

cerberus/validator.py

@@ -1043,6 +1072,7 @@ def validate_rule(rule):
                       {'type': ['dict', 'string'],
                        'check_with': 'bulk_schema'}]} """
    )
+    _validate_require_all = dummy_for_rule_validation("{'type': 'boolean'}")


please keep the methods sorted alphabetically in order to keep it easy to find them.

it looks like they are not in alphabetical order right now:

logical anyof nullable keysrules

funkyfuture · 2018-08-04T14:01:48Z

docs/schemas.rst

@@ -15,7 +15,8 @@ which is expected to be a string not longer than 10 characters. Something like
 very long string'}`` or ``{'name': 99}`` would not.

 By default all keys in a document are optional unless the :ref:`required`-rule
-is set for a key.
+is set for a key or validator attribute :attr:`~cerberus.Validator.require_all`


"is set True for individual fields or the validator's :attr:~cerberus.Validator.require_all is set to True in order to expect all schema-defined fields to be present in the document.

funkyfuture · 2018-08-04T14:03:02Z

docs/usage.rst

+    >>> v.validate({}, schema)
+    True
+
+However, you can require all document keys pairs by either setting


if there's an 'either', what's the other case?

funkyfuture · 2018-08-04T14:04:33Z

docs/usage.rst

@@ -162,6 +162,67 @@ mapping that is checked against the :ref:`schema <schema_dict-rule>` rule:
   ``allow_unknown`` can also be set to a validation schema.


+.. _requiring-all:
+
+Requiring all


this looks like copied from the allow_unknown section. given that require_all is less complex, a sufficient explanation should be doable with less prosa.

Have no idea how to reduce text. Exclude some examples?

funkyfuture · 2018-08-04T14:06:13Z

docs/validation-rules.rst

+:attr:`~cerberus.Validator.require_all` property of the validator for the
+subdocument.
+For a full elaboration refer to :ref:`this paragraph <requiring-all>`.
+
 .. _required:

 required


please add a short cross-reference to the section in usage.rst here.

funkyfuture · 2018-08-04T14:06:50Z

docs/usage.rst

+
+    >>> v.validate({'a_dict': {'address': 'foobar'}}, schema)
+    True
+


please add a short cross-reference to the require and require_all rules here.

funkyfuture · 2018-08-08T14:42:20Z

cerberus/tests/test_validation.py

@@ -5,6 +5,7 @@
 from datetime import datetime, date
 from random import choice
 from string import ascii_lowercase
+import itertools


please shift this above re.

funkyfuture · 2018-08-08T14:44:00Z

docs/usage.rst

+
+By default any keys defined in the schema are not required:
+
+.. doctest::


i don't think this example is needed.

funkyfuture · 2018-08-08T14:46:50Z

docs/usage.rst

+    >>> v.validate({}, schema)
+    True
+
+However, you can require all document keys pairs by setting


i think that suffices as a sentence after the explanation of the variant when it is set at initialization. an inline example (validator_require_all = True) should also be enough, no need for a code block.

funkyfuture · 2018-08-08T14:47:37Z

docs/usage.rst

+    >>> v = Validator({'name': {'type': 'string', 'maxlength': 10}}, require_all=True)
+    >>> v.validate({})
+    False
+    >>> v.require_all = False


actually, this shoull be enough with regards to the validator attribute.

funkyfuture · 2018-08-08T14:48:28Z

docs/usage.rst

+    >>> v.validate({})
+    True
+
+``require_all`` can also be set as rule to configure a validator for a nested


i'm not sure, but i think we commonly use the term subdocument instead of 'nested mapping' through the docs.

funkyfuture · 2018-08-08T14:49:22Z

docs/usage.rst

+    ...   'name': {'type': 'string'},
+    ...   'a_dict': {
+    ...     'type': 'dict',
+    ...     'require_all': True, # this overrides the behaviour for


the comments' content has already been stated.

funkyfuture · 2018-08-08T14:52:02Z

docs/usage.rst

+    ...   }
+    ... }
+
+    >>> v.validate({'a_dict': {}}, schema)


the example would make more sense if there was another field defined in the schema at the top level, that doesn't exist in the document. the output of v.errors could conclude the story.

funkyfuture · 2018-08-08T14:52:18Z

docs/usage.rst

+    >>> v.validate({'a_dict': {}}, schema)
+    False
+
+    >>> v.validate({'a_dict': {'address': 'foobar'}}, schema)


not really needed imo.

funkyfuture · 2018-08-08T14:54:07Z

docs/validation-rules.rst

 .. _required:

 required
 --------
+See also :ref:`this paragraph <requiring-all>`.


rather put this in a first note admonition below, a little more elaborated: "To define all fields of a document ..."

funkyfuture · 2018-08-08T15:02:12Z

what about this?

schema = {
    'foo': {
        'type': 'dict',
        'require_all': True,
        'allow_unknown': {…},
    }
}

would that make something explode? should require_all and allow_unknown mutually exclude each other?

pohmelie · 2018-08-09T15:13:47Z

would that make something explode?

At first look they are not crossing anywhere.

funkyfuture · 2018-08-13T11:05:48Z

At first look they are not crossing anywhere.

should the child validator that is dispatched for allow_unknown have the require_all argument always set ro False?

funkyfuture · 2018-08-13T11:06:40Z

docs/usage.rst

-``require_all`` can also be set as rule to configure a validator for a nested
-mapping that is checked against the :ref:`schema <schema_dict-rule>` rule:
+``require_all`` to ``True`` at validator initialization (``v = Validator(…, require_all=True)``)
+or change latter via attribute access (``v.require_all = True``).


… change it later …

funkyfuture

okay, i think beside these minor changes we're getting close to resolve this.

could you please squash all your commits into one, so a final review would be easier.

pohmelie · 2018-08-13T16:52:53Z

should the child validator that is dispatched for allow_unknown have the require_all argument always set ro False?

Hm, if it validates only unknown fields, then «yes», I think.

pohmelie · 2018-08-13T17:16:31Z

It looks like squash broke all merge logic, since all history goes away. And I don't known how to squash only my commits, cause there was merges from master to my branch between. You can squash all my pull to one commit on merge actually, and squashed changes are on "Files" tab above.

Anyway, do not know how to restore this PR to previous state. 😞

funkyfuture · 2018-08-13T19:05:41Z

sorry, i don't know what the situation in your local repository actually looks like. i hope you have all your changes still on your machine. just apply them on a new branch checked out from master an open a new pr.

funkyfuture added this to the 1.3 milestone Jul 7, 2018

funkyfuture requested changes Jul 8, 2018

View reviewed changes

funkyfuture requested changes Jul 11, 2018

View reviewed changes

funkyfuture reviewed Jul 12, 2018

View reviewed changes

funkyfuture requested changes Aug 4, 2018

View reviewed changes

funkyfuture requested changes Aug 8, 2018

View reviewed changes

funkyfuture reviewed Aug 13, 2018

View reviewed changes

funkyfuture requested changes Aug 13, 2018

View reviewed changes

add require_all (close pyeve#417)

4b5c50c

pohmelie closed this Aug 13, 2018

pohmelie force-pushed the add_require_all branch from 9e4fcdb to 4b5c50c Compare August 13, 2018 16:58

pohmelie deleted the add_require_all branch August 13, 2018 17:04

pohmelie mentioned this pull request Aug 13, 2018

add require_all (close #417) #434

Merged

		assert_success({'foo': 'bar'}, schema, validator)


		def test_require_all_override_by_subdoc_require_all():


		>>> v.validate({'a_dict': {'address': 'foobar'}}, schema)
		True


		By default any keys defined in the schema are not required:

		.. doctest::

add require_all validator argument #421

add require_all validator argument #421

Conversation

pohmelie commented Jul 7, 2018 • edited Loading

pohmelie commented Jul 7, 2018

funkyfuture commented Jul 7, 2018

funkyfuture left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funkyfuture Jul 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

funkyfuture Jul 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

pohmelie commented Jul 12, 2018 • edited Loading

funkyfuture Jul 12, 2018 • edited Loading

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 commented Jul 23, 2018

pohmelie commented Jul 24, 2018

funkyfuture commented Jul 25, 2018

pohmelie commented Jul 25, 2018 • edited Loading

funkyfuture commented Jul 29, 2018

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

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 commented Aug 8, 2018

pohmelie commented Aug 9, 2018

funkyfuture commented Aug 13, 2018

Choose a reason for hiding this comment

funkyfuture left a comment

Choose a reason for hiding this comment

pohmelie commented Aug 13, 2018

pohmelie commented Aug 13, 2018 • edited Loading

funkyfuture commented Aug 13, 2018

add `require_all` validator argument #421

add `require_all` validator argument #421

pohmelie commented Jul 7, 2018 •

edited

Loading

funkyfuture left a comment •

edited

Loading

funkyfuture Jul 8, 2018 •

edited

Loading

funkyfuture Jul 8, 2018 •

edited

Loading

pohmelie commented Jul 12, 2018 •

edited

Loading

funkyfuture Jul 12, 2018 •

edited

Loading

pohmelie commented Jul 25, 2018 •

edited

Loading

pohmelie commented Aug 13, 2018 •

edited

Loading