Skip to content
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

Validator ideas #10

Open
mjtamlyn opened this issue Apr 3, 2016 · 7 comments
Open

Validator ideas #10

mjtamlyn opened this issue Apr 3, 2016 · 7 comments

Comments

@mjtamlyn
Copy link
Owner

mjtamlyn commented Apr 3, 2016

class Validator(object):
    def validate(self, value):
        pass

    def clean(self, value):
        self.validate(value)
        return value

class IntegerCleaner(Validator):
    def clean(self, value):
        try:
            value = int(value)
        except ValueError:
            raise ValidationError(...)
        return value

class FunctionValidator(Validator):
    def __init__(self, func):
        self.func = func
    def validate(self, value):
        func(value)

Not 100% sure about the split between clean and validate but there is a difference between the two. Perhaps instead we have cleaners and validators, one of which changes and one of which doesn't, but they're chainable together.

Related but not necessarily belonging in this tree is idea of a shaper which is a multivalued cleaner which returns different data shape. Then again perhaps these should be in the validator tree so subsequent validators can use the changed shape. Shaping is also about input/output though so these lines are blurry.

@LilyFoote
Copy link
Collaborator

I like this. A clear distinction between validation and cleaning may also be useful for distinguishing validation that can only happen on the backend and validation that may also run on the frontend.

In your structure above, it seems to me that any Validator that only overrides validate is a candidate for a frontend mirror, whereas one that overrides clean is not.

Perhaps some data transformations can considered cheap and idempotent, like int(value), so maybe they can be mirrored too. But this would require that they don't break earlier validators too, so this is a lot more complex to allow.

@mjtamlyn
Copy link
Owner Author

mjtamlyn commented Apr 3, 2016

The problem is, if a "plain" validator depends on a cleaning one, then you need to mirror them all.

@LilyFoote
Copy link
Collaborator

Yeah... it's probably impossible to handle in the generic case. That shouldn't stop us thinking about how to support a subset though.

@LilyFoote
Copy link
Collaborator

One really shiny thing that a tree/dag of validators/cleaners allows is a ValidationError will only prevent dependent validators from running. This is much nicer than the current case where a Form.clean method only runs if all other validators pass. This is nicer than checking manually in Form.clean which fields have been successfully cleaned and which have failed validation.

@LilyFoote
Copy link
Collaborator

I think my crossed-out comment does apply to a DRF Serializer.validate method.

The structure also allows raising more than one cross-field ValidationError without needing custom error handling within the Form.clean or Serializer.validate method.

@mjtamlyn
Copy link
Owner Author

mjtamlyn commented Apr 4, 2016

Custom error handling isn't so necessary there now we have add_error. This does lead to the idea that a ValidationError could just be bound to two fields and displayed however the renderer wants. That said, sometimes although the validation crosses fields, the error is explicitly about one of the fields.

Another edge case I've just thought of which I'm not sure how well it fits, and definitely isn't clearly mirrorable, is validation which depends on a hidden aspect of the bound intial object(s). It would seem a bit clunky to require constructing the serializer using the instance we are going to operate on AND then having to pass it to the initial step. It kinda opens up a question about read/write patterns - if we have a read only field which is marked as hidden for renderers then we can cross field validate using it later. The balance between read/write different behaviour on the same object and having distinct objects for read/write is an interesting one.

@MoritzS
Copy link
Collaborator

MoritzS commented May 18, 2016

As I already mentioned in #27, I really dislike how accessing form.errors in Django currently triggers a full validation (or clean).

I agree that separating cleaners and validators makes sense but I think it would be ok to have them both subclass from the same base class and dynamically detect if they overrode clean() or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants