iommirocks / iommi

Your first pick for a django power cord
http://iommi.rocks
BSD 3-Clause "New" or "Revised" License
718 stars 47 forks source link

Seems like `auto` doesn't care about model field validators #421

Closed berycz closed 7 months ago

berycz commented 1 year ago

in a model field (FileField) I have validators=[validate_file_extension] and I'm able to upload files which aren't allowed in the validator. I suppose the auto thingy doesn't take validators from model fields

berycz commented 1 year ago

I suppose it should be as default Field.is_valid?

from django.core.validators import EMPTY_VALUES

#....

    @staticmethod
    @refinable
    def is_valid(form: 'Form', field: 'Field', parsed_data: Any, **_) -> Tuple[bool, str]:
        # language=rst
        """
        Validation function. Should return a tuple of `(bool, reason_for_failure_if_bool_is_false)` or raise ValidationError.

        .. code-block:: python

            form = Form.create(
                auto__model=Artist,
                fields__name__is_valid=lambda parsed_data, **_: (parsed_data.startswith('H'), 'Must start with H!'),
            )

            # @test
            show_output(form.bind(request=req('post', **{'-submit': '', 'name': 'blizzard of ozz'})))
            # @end
        """
        # this code is based on django form field validators
        #   https://github.com/django/django/blob/main/django/forms/fields.py#L184
        if self.model_field and self.model_field.validators:
            if value not in EMPTY_VALUES:
                for validator in model_field.validators:
                    validator(parsed_data)

        return True, ''
yurkobb commented 1 year ago

I'd say most users would probably assume model field validators would apply, analogous to django's ModelForm.

There is a risk though that implementing this would break some user code that (perhaps unknowingly) relied on the absence of validation. If we consider this significant, perhaps adding a parameter to Form, e.g. validate_model_fields that would allow explicitly enabling this behavior per field (by specifying a list of field names) or for all fields (by passing in a True or ['__all__']) would provide a safer upgrade path.

I'd vote for just adding the model field validation unconditionally thought.