readonly conflicts with default (and rule_filter)

[dkellner]

This allows using default and readonly on the same field. See #268 for details.

Furthermore, this PR introduces a new feature called rule_filter to solve #268 in an (in my opinion) elegant way. I'm not sure if this feature will be ever used externally, but just in case I wrote some documentation for it. Briefly speaking, it allows to do the following:

>>> schema = {'name': {'type': 'string', 'maxlength': 10}}
>>> v = Validator(schema, rule_filter=lambda f: f != 'maxlength')
>>> v.validate({'name': 'Johanna-Maria'}, schema)
True

[funkyfuture]

i have several concerns here. i hope to write them up until Monday.

[nicolaiarocci]

This looks good to me!

[funkyfuture]

just a short summary of my concerns:

while i think this pr approaches a valid problem, i think it's still not sufficent and may be a burden in the end.

the whole normalization/validation/rules dispatching logic/andwewantitalltobecustomizable thing needs proper attention for too long. it shouldn't be continued to just fix things up when lacks of design become obvious. introducing new attributes now that may later be unnecessary doesn't make it easier as they will have to be dealt with for legacy reasons.

my current plan is to dig through issues and code (history) on Friday to come up with proposals in a broader frame. it will simply take some hours, i couldn't spare yet.

and most certainly, lambdas as configuration values should be avoided.

[dkellner]

I understand your concerns and am of course happy with finding a better solution. We don't have to rush here. Some things I noticed when digging into this:

• Validation and normalization are completely separate, but the methods are in the same class. Maybe split the functionality in two (or even more) classes?
• Recursing down the document structure is implemented in two different ways for validation and normalization. Maybe this can be done the same way.

Maybe we can have one base class for doing all the recursing stuff ("walking" the document) and calling somehow registered rule handlers? This class could be the base for both a Validator class and a Normalizer class and then we could combine two Validators and one Normalizer to get what we need, which is validating some fields before normalization and then validate again afterwards.

A lot of maybes here and I am nowhere as deep into codebase and prior design decisions as you are @funkyfuture, so I think it's best to leave this up to you and maybe I can help with the actual implementation later.

[funkyfuture]

first thing i will do is to comprehend the problems with the current implementation, then propose design changes. i'll be happy to get your feedback on that as you already dove into parts of a solution.

[funkyfuture]

okay, i'm clearly overthinking things here. at least for the particular issue in question.

what about the following? let's consider readonly a normalization rule as it is used to control a document's shape that will be used further down the line in business logic. thus its evaluation can be placed before default's in the normalization block. makes that sense?

if this approach worked, further cluttering of evaluation blocks and more properties that determine evaluation order can be avoided. (yes, afair i introduced those, but i consider them rather hacky.)

nonetheless i got some impulses that resulted in ideas for a possible 'next generation' of how we define rules (dependencies and other properties) and control their evaluation order. but having ideas is one thing, having time to act on them is another.

[dkellner]

I'm not sure about readonly being a normalization rule as it will never change the document. All other normalization rules do that. Furthermore this would break backwards compatibility as readonly would not be checked anymore if somebody calls validate with normalize=False.

If your concern for this issue is mainly the introduction of a new feature (rule_filter), how about considering it just for internal use and not document it (for the public API)? This way we could allow using readonly with default without breaking anything and at the same time not introduce a new feature that we have to support in future releases.

[nicolaiarocci]

I'm merging this. Eve (which is the main reason why Cerberus was born in the first place 😄) needs support for readonly with default. We can always refine later, if needed.

[dkellner]

Maybe we can at least remove the section in the documentation so we're not pointing users to use a feature we're not sure about? I'd leave the comments/docstrings in the code as they will help future coders to understand what rule_filter does.

[nicolaiarocci]

Maybe we can at least remove the section in the documentation so we're not pointing users to use a feature we're not sure about? I'd leave the comments/docstrings in the code as they will help future coders to understand what rule_filter does.

👍

[dkellner]

I will do this and rebase on master tonight!

[funkyfuture]

imo, you could even drop that property and 'hardcode' the evaluation order.

what are you going to do with the testcases? (they have to be refactored anyway.) though they make hardly sense and are irritating with a non-public feature, they are very usable for a later finer-grained customization mechanic. maybe mark them with @pytest.skip for now?

I'm merging this. Eve (which is the main reason why Cerberus was born in the first place 😄) needs support for readonly with default.

but Cerberus has emancipated from Eve, there are numerous other uses in the real world. and the users have contributed for a more general library. the readonly rule is actually so specific that it should be defined in a custom validator.

Eve shall prosper, and Cerberus shall be capable to support this. but imo its heritage is not an argument for exclusive hot fixes and workarounds. we should take Eve's needs as an impulse to improve a generally usable, configurable validation library that is unique in the Python ecosystem.

[nicolaiarocci]

the readonly rule is actually so specific that it should be defined in a custom validator.

@dkellner do you think that would be feasible?

[dkellner]

imo, you could even drop that property and 'hardcode' the evaluation order.

I don't think we can (easily) hardcode the evaluation order. We need a way to pass the information to child validators that they should only process certain rules (the ones to process before normalization). We need to validate all readonly fields (including subdocuments) before normalization, because normalization will recurse itself, too. Of course we can implement the recursion again without leveraging child validators, but I'd consider that rather hacky.

what are you going to do with the testcases? (they have to be refactored anyway.) though they make hardly sense and are irritating with a non-public feature, they are very usable for a later finer-grained customization mechanic. maybe mark them with @pytest.skip for now?

I'd keep them even for a non-public feature. The test-cases for using readonly with default are hardly enough to cover everything rule_filter can do, so maybe we won't notice if it breaks due to future changes. We can remove them when we toss rule_filter itself? I don't mind the rewrite for py.test ;).

the readonly rule is actually so specific that it should be defined in a custom validator.

Can we really use custom validators or custom rules here? They are both called after normalization. As far as I know, there is currently no "native" way to validate before normalization.

@funkyfuture By the way, I absolutely agree with you that there are use cases for Cerberus apart from Eve and that changes we introduce here should be reasonable in that regard. I myself use it in a non-Eve project, too.

---

In my opinion, it all leads to the question if we see pre-normalization-validation as a feature in Cerberus itself, or if we think this should be handled by the application (e.g. by splitting the schema and using two different Validators). Personally I would prefer the former, as it would allow to have one schema to describe your data even if it contains auto-generated fields the user should not be able to set. How do you think about this?

[nicolaiarocci]

In my opinion, it all leads to the question if we see pre-normalization-validation as a feature in Cerberus itself, or if we think this should be handled by the application (e.g. by splitting the schema and using two different Validators). Personally I would prefer the former, as it would allow to have one schema to describe your data even if it contains auto-generated fields the user should not be able to set. How do you think about this?

I agree that it should/could be a Cerberus feature.

[funkyfuture]

@dkellner have you considered to include the readonly logic in an overriding method that handles default?

[dkellner]

Yes I did but I have the same concerns as implementing it as a "standalone" normalization rule (see #272 (comment)). Nevertheless I will play around with that idea later today or tomorrow and actually show some code. Maybe it's a "good" compromise solution.

[dkellner]

OK, here's some code: https://github.com/dkellner/cerberus/tree/normalize-readonly . I've implemented readonly as normalization rule, as suggested by @funkyfuture before (#272 (comment)). I actually quite like the solution, except the (admittedly subtle) breaking change that readonly won't be validated anymore if somebody is using normalize=False. And the (maybe philosophical) question if we can consider readonly a normalization rule regarding its nature of not changing the document.

If you both agree that this is a way to solve this problem here, I'm going to update the docs in that branch and create a PR.

[nicolaiarocci]

Well, it works and... implementation is pretty straightforward. I would tag it an acceptable compromise.

[funkyfuture]

some points:

there's a misunderstanding, i didn't mean that readonly should be a normalization rule, but its logic could be integrated in the handling of default. so, in this case, the readonly key in the schema would provide extra config for default (like allow_unknown to schema for example).

you can extend the readonly (validation) rule so it doesn't run a second time if a readonly error has already been filed by the default rule, using the schema or document error tree. if there is no error, it possibly (in case that a default rule was processed) has to run a second time. imo it's okay as it is a compromise to put functionality into a framework that isn't fit for it and the readonly logic is lightweight.

as one of cerberus' paradigm is to be non-blocking, i don't agree that a failing normalization skips validation.

the tests consider nested schemas and expected errors, perfect! :-D

a diff-view for reviewing would be great.

[dkellner]

@funkyfuture Ah, got it. :) One more question about the non-blocking paradigm: so right now it stops validating if some "priority" rule fails, and readonly is one of them. If readonly now is a normalization rule, should we stick to that behaviour and implement a special case for that? I find the current behaviour somewhat confusing (when validation will stop and when it will just continue adding more errors to an already invalid field).

[funkyfuture]

to clarify, i don't think readonly needs to be abandoned as validation rule. (i think it should be abandoned at all, because it's eve-specific. but there's that legacy thing.)

i mean the following as pseudo-code:

def normalize_default(...):
    if do_readonly_logic(...) == 'failed':
        self.error(..., READONLY_FIELD, ...)
    else:
        do_default_logic(...)

def validate_readonly(...):
    # don't re-evaluate if an error was already submitted by default during normalization
   if READONLY_FIELD in self.document_error_tree.fetch_errors_from(self.document_path + (field,)):
        return
   ...

the aborting priority_rules make sense as a user can assert that a value is not None (nullable) and has a certain type (type), so the following tests do not fail due to TypeError or ValueError. the reasoning for readonly is that a field that shouldn't be there at all, doesn't need to be validated.

[dkellner]

Hm, but this is not enough. You should not run readonly validation at all if there was normalization before - it will always fail on fields having both default and readonly (which is the whole point of this issue). But I will look at the code again, remove the part that skips validation after failed normalization and try to find a nice way to make a failing readonly stop validation for that field.

[funkyfuture]

okay, you can test whether default is defined for that field in validation as well.

[nicolaiarocci]

See #282