Opened 8 years ago

Last modified 5 weeks ago

#25594 new New feature

Difficult to customize model field default_validators and have them used on both model and form fields

Reported by: Marcin Nowak Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Ülgen Sarıkavak Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

There is incosistency between model's URLField and modelform's URLField validation.
Form's URLField does not use model's field validation.

Form and model fields have declared class properties with own validators, whose defaults to validators.URLValidator(). But when you customize validator for model field, the form is still using own (and bad in this context) instance. As a result validation differs for form and model. And this is an incosistency between modelforms and models.

Change History (11)

comment:1 by Tim Graham, 8 years ago

Component: FormsDatabase layer (models, ORM)
Summary: ModelForm`s URLField does not use model`s URLField validatorDifficult to customize model field default_validators and have them used on both model and form fields
Triage Stage: UnreviewedAccepted
Type: BugNew feature
Version: 1.8master

This problem exists for more than just URLField so I'm going to retitle the ticket to reflect that. Using the use case from #25593 of customizing a URLValidator's accepted schemes, my idea would be to allow something like:

URLField(validators=[URLValidator(schemes=[])])

and have that validator be used in place of the default validator on both the model and form field. Thinking out loud: would it be acceptable to replace any default_validators with values from validators if the value from validators is a subclass of a value in default_validators? I guess for something like RegexValidator you may want to have multiple regular expressions so we probably need a different way to say "use this validator to replace the default validators". Maybe a default_validators argument to model fields? This change seems tricky and likely needs a discussion on the DevelopersMailingList to nail down the API details, but I agree it'd be a worthwhile improvement.

comment:2 by Claude Paroz, 8 years ago

Thinking out loud, too: provide a NO_DEFAULT_VALIDATORS constant/class, when added to the validators list prevent using the field's default validators.

comment:3 by Curtis Maloney, 8 years ago

From looking at the code, I suspect the existing reasoning would be as follows:

  • Model fields have default validators, and devs can append to this list
  • Form fields have default validators, and devs can append to this list
  • When performing a clean on a ModelForm, it will invoke Model validation

So, if you reconfigure your model field to be more restrictive, this isn't a problem as it will catch the issue.

However, if you make it more permissive, this will cause problems in your form.

comment:4 by Claude Paroz, 8 years ago

Has patch: set
Patch needs improvement: set

I created a pull request with my suggestions: PR.

It's not completely ready mainly because more tests should be added. But I'd like to hear opinions on the design before going too far. It has some backwards compatible issues (one test needed changes). I will ask for comments on the mailing list.

comment:5 by Arthur Pemberton, 6 years ago

My ticket was marked as a duplicate of this, I'm not sure I agree, but I'll re-post here:

Example URL: "myscheme://server/group-name/item-name"

Problem 1: models.URLField needs to be subclassed to change allowed schemes
Problem 2: forms.URLField needs to be subclassed to change allowed schemes
Problem 3: validators.URLValidator needs to be subclassed to change regex to allow netloc that is a hostname without a domain ad TLD

At the very least, making these changes should be documented better. The message accompanying ValidationError gives no hint as to the source issue with a URL.

comment:6 by Federico Jaramillo Martínez, 4 years ago

From a design perspective, wouldn't it be clearer to have a new arguments for every forms.Field and models.Field that disables the default validators?
Something like disable_default_validators=False.

When instantiating a field, one can set it to True, and this will disable all default_validators, and if it is set on a model field, will be propagated to the form field.

This way, the developer is in full control of the validators, both for the model field and the form field. Less magic, and more clear what is going on.

Version 1, edited 4 years ago by Federico Jaramillo Martínez (previous) (next) (diff)

comment:7 by Federico Jaramillo Martínez, 4 years ago

If my proposed solution sounds good, I can work on it.

comment:8 by Mariusz Felisiak, 14 months ago

Fixing this ticket would also fix #25594 which is only applies to propagating MinValueValidator and MaxValueValidator validators.

comment:9 by Claude Paroz, 14 months ago

You meant to mention #26834 ?

in reply to:  9 comment:10 by Mariusz Felisiak, 14 months ago

Replying to Claude Paroz:

You meant to mention #26834 ?

Yes!

comment:11 by Ülgen Sarıkavak, 5 weeks ago

Cc: Ülgen Sarıkavak added
Note: See TracTickets for help on using tickets.
Back to Top