Copy declared validators to serialiser fields

[carltongibson]

Replaces #5411. Closes #5410.

• Rebases work by @rvignesh89.
• Fixes lint errors.
• Add for "other" field types, (those with tests from 31eba20)

[carltongibson]

@rvignesh89 I don't know if you're up for helping finish this one off, but if so PRs against this branch for the failing test cases would be great! (No stress if not 🙂)

Thanks for the input!

[rvignesh89]

Ahh I totally forgot about this PR that I made! Should definitely be able to finish it up this this week.

[carltongibson]

@xordoquy @rpkilby Could you have a look at this please folks?

• It's fiddly: there's two strategies depending on whether the field has a default validator or not. (If it does it's necessary to use error_messages rather than declaring the validator, or else you end up with two.
• I need to add 826fd76 in order to get around ordering issues in the TestSerializer repr in tests/test_model_serializer.py::TestRegularFieldMappings::test_field_options. What I can't quite work out is why this never came up before? (Occasional failures depending on key ordering...)

I think this should be about the limits of efforts we make here. (It's fiddly.)

[carltongibson]

And a mix of failures/passes on the build... Hmmm.

[xordoquy]

That's quite something !
Will take some time this evening to review that.

[carltongibson]

Looking at the failures it seems they come down to Unicode strings on Py2 and needing to evaluate a lazy object on Django 1.10. Both of those should be workable, though the 1.10 issue will be more of a fiddle.

My original questions remain though, so if you can make the time to look at those I’d be grateful.

[rpkilby]

I'll need to review this further, but I'd argue this should be bumped to the 3.8 milestone.

[carltongibson]

... I'd argue this should be bumped to the 3.8 milestone.

Yeah. I could go either way there. Every fix is a behaviour change but is this one just patching up something that should already work or adding a new feature? (As I say, I'm not attached to either way.)

Main thing (first) is if you could cast you eye over the change and, even more, the weirdness around 826fd76.

[rvignesh89]

A little embarrassed that I couldn't finish this as I said. Nevertheless, @carltongibson since you are taking this up I just wanted to bring up a case that I was considering when I stopped halfway.

It's when the user extends the message in a MinlengthValidator/MaxlengthValidator with their own version of a message and then use that in the models. Something like this,

class CustomMinLengthValidator(MinLengthValidator):
    message = ungettext_lazy(
        'Please ensure this value has at least %(limit_value)d character (it has %(show_value)d).',
        'Please ensure this value has at least %(limit_value)d characters (it has %(show_value)d).',
        'limit_value')

class Review(models.Model):
    text = models.CharField(
        max_length=100,
        validators=[CustomMinLengthValidator(limit_value=5)]
    )

This is what I was referring to in this comment. This case made the changes for MinLength & MaxLengthValidators more complex than I had initially thought. Is this something that you are deliberately ignoring?

[carltongibson]

Hi @rvignesh89. The issue here is with validators that are provided by default, e.g. an email validator on an EmailField, or a Max length validator on a CharField, and so on.

In that case you can’t simply set your custom validator as you have done. Rather you need to create a field subclass that replaces the default validators. (Otherwise you end up with duplicate validators! Which is what happened to me running your tests, which I’m guessing is the weirdness you ran into.)

The thing you can do is use error_messages to customise the message, which is all we’re doing in this case. So I went for that. 🙂

I’m going to see what some of the other Core Team say here but I’m worried that adding this behaviour may open the way to a lot of little edge case bugs. Bar the repr weirdness I have the tests passing but they’re just the simplest cases.

If it turns out that it’s too tricky to get this right we may decide to skip it and accept a known limitation here. (I can think it would be easy enough to set the right messages in a serialiser’s init method if we go this route.)

[rpkilby]

I have my reservations about this approach. In the current implementation, declared and generated serializer fields will use the same error messages. With this PR, generated fields will use those from the model field. I favor the consistency that is currently present.

[carltongibson]

Having sat on this for the weekend I'm going say we should drop this.

Whilst it would be nice to automatically pick up declared validators from models, the cost-benefit to do this covering all the possible cases doesn't seem worth it. It looks to me like a lot of complication for a marginal nice-to-have.

Rather I think we should point users to overriding their serializer's __init__ and mapping the validators they want to copy across there. (__new__ would also be an option.) It would be simple enough to write functions to do this and add a base serialiser class to wrap up any repetition if it were a project wide requirement. This would allow users to handle their limited number of cases without us needing to add code for every possible thing that could come up.