Changing from list to set in `exclude` raises errors, and is not documented.

[אורי]

The commit:
​https://github.com/django/django/commit/1ea7e3157d1f9b4db71e768d75ea57e47dbd49f9

My use case:

I used def clean_fields in a mixin inherited by some of my models:

class CleanAndValidateAllFieldsMixin(object):
    def clean_fields(self, exclude=None):
        """
        Allows to have different slug and username validators for Entity and User.
        """
        if (exclude is None):
            exclude = []

        self.clean_all_fields(exclude=exclude)

        try:
            super().clean_fields(exclude=exclude)
        except ValidationError as e:
            errors = e.error_dict
        else:
            errors = {}

        self.validate_all_fields(errors=errors, exclude=exclude)

    def clean_all_fields(self, exclude=None):
        pass

In some of the models that use it, I defined clean_fields and added values to exclude:

    def clean_fields(self, exclude=None):
        self.normalize_slug_and_username()
        self.validate_username_for_slug()
        self.validate_username_required()
        self.validate_username_unique()

        if (exclude is None):
            exclude = []

        # Reserved username can be less than 6 characters, and any alphanumeric sequence.
        exclude += ['username', 'slug']

        return super().clean_fields(exclude=exclude)

    def clean_fields(self, exclude=None):
        """
        Allows to have different slug and username validators for Entity and User.
        """
        if (exclude is None):
            exclude = []

        # If special username is true, don't validate username.
        if (self.special_username):
            self.normalize_slug_and_username()
            self.validate_username_for_slug()
            self.validate_username_required()
            self.validate_username_unique()
            exclude += ['username', 'slug']

        return super().clean_fields(exclude=exclude)

The results: model tests fail with Django 4.1 alpha:

======================================================================
ERROR: test_username_too_long_exception_4 (speedy.core.accounts.tests.test_models.ReservedUsernameHebrewTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "...\speedy\core\accounts\tests\test_models.py", line 395, in test_username_too_long_exception_4
    reserved_username.save()
  File "...\speedy\core\base\models.py", line 21, in save
    return super().save(*args, **kwargs)
  File "...\speedy\core\base\models.py", line 12, in save
    self.full_clean()
  File "...\.venv_3.9\lib\site-packages\django\db\models\base.py", line 1464, in full_clean
    self.clean_fields(exclude=exclude)
  File "...\speedy\core\accounts\models.py", line 182, in clean_fields
    exclude += ['username', 'slug']
TypeError: unsupported operand type(s) for +=: 'set' and 'list'

----------------------------------------------------------------------

Is exclude a set now (instead of a list) and where is it documented? If it's not documented, please document it. I didn't find it documented on ​https://docs.djangoproject.com/en/dev/releases/4.1/.

What is the best written code to change the line exclude += ['username', 'slug'] in my code? Is it exclude |= set(['username', 'slug']) or exclude |= {'username', 'slug'}? Or should I convert to list and then back to set?

What is the reason exclude was changed to a set?

How should exclude be defined in clean_fields and what should I do if I receive exclude is None?

Thanks,
Uri.

[Mariusz Felisiak]

Agreed, we should document this change, maybe:

docs/ref/models/instances.txt

@@ -252 +252 @@
 .. method:: Model.clean_fields(exclude=None)
 
 This method will validate all fields on your model. The optional ``exclude``
-argument lets you provide a list of field names to exclude from validation. It
-will raise a :exc:`~django.core.exceptions.ValidationError` if any fields fail
-validation.
+argument lets you provide a set or list of field names to exclude from
+validation. It will raise a :exc:`~django.core.exceptions.ValidationError` if
+any fields fail validation.
 
 The second step ``full_clean()`` performs is to call :meth:`Model.clean()`.
 This method should be overridden to perform custom validation on your model.
@@ -355 +355 @@
 :attr:`.Field.unique_for_date`, :attr:`.Field.unique_for_month`,
 :attr:`.Field.unique_for_year`, or :attr:`Meta.unique_together
 <django.db.models.Options.unique_together>` on your model instead of individual
-field values. The optional ``exclude`` argument allows you to provide a list of
-field names to exclude from validation. It will raise a
+field values. The optional ``exclude`` argument allows you to provide a set or
+list of field names to exclude from validation. It will raise a
 :exc:`~django.core.exceptions.ValidationError` if any fields fail validation.
 
 :class:`~django.db.models.UniqueConstraint`\s defined in the
@@ -380 +380 @@
 
 This method validates all constraints defined in
 :attr:`Meta.constraints <django.db.models.Options.constraints>`. The
-optional ``exclude`` argument allows you to provide a list of field names to
-exclude from validation. It will raise a
+optional ``exclude`` argument allows you to provide a set or list of field
+names to exclude from validation. It will raise a
 :exc:`~django.core.exceptions.ValidationError` if any constraints fail
 validation.
 

docs/releases/4.1.txt

@@ -561 +561 @@
   ``URLResolver._callback_strs``, and ``URLPattern.lookup_str()`` are
   moved to ``django.contrib.admindocs.utils``.
 
+* Custom :meth:`.Model.clean_fields`, :meth:`.Model.validate_unique`, and
+  :meth:`.Model.validate_constraints` methods must handle the ``exclude``
+  values passed as ``set``.
+
 .. _deprecated-features-4.1:
 
 Features deprecated in 4.1

What is the reason exclude was changed to a set?

exclude is used only for containment checks, so we decided to use set for performance reasons.

What is the best written code to change the line exclude += ['username', 'slug'] in my code? Is it exclude |= set(['username', 'slug']) or exclude |= {'username', 'slug'}? Or should I convert to list and then back to set?

Personally I would use exclude |= {'username', 'slug'} or

exclude.add('username')
exclude.add('slug')

[אורי]

Replying to Mariusz Felisiak:

...

What is the best written code to change the line exclude += ['username', 'slug'] in my code? Is it exclude |= set(['username', 'slug']) or exclude |= {'username', 'slug'}? Or should I convert to list and then back to set?

Personally I would use exclude |= {'username', 'slug'} or

exclude.add('username')
exclude.add('slug')

Thank you. Should I assume exclude is a set from Django 4.1, or should I convert it to set? Especially since in Django 4.0 it's a list.

[Mariusz Felisiak]

Thank you. Should I assume exclude is a set from Django 4.1, or should I convert it to set? Especially since in Django 4.0 it's a list.

exclude can be a list or set, it depends from where clean_fields() is called, so it's safer to handle both types, e.g.

def clean_fields(self, exclude=None):
    if exclude is None:
        exclude = set()
    else:
        exclude = set(exclude)
    ...

[אורי]

Replying to Mariusz Felisiak:

exclude can be a list or set, it depends from where clean_fields() is called, so it's safer to handle both types, e.g.

def clean_fields(self, exclude=None):
    if exclude is None:
        exclude = set()
    else:
        exclude = set(exclude)
    ...

Thank you.

[אורי]

Hi, just to let you know - if I either convert exclude to a set or convert it to a list, and then add my values, then it works with all versions of Django. So the problem was just that I assumed it's a list, instead of converting it to a set or list. Maybe it's better to recommend it in the documentation - convert exclude to a set or list if it's not None.

Commit - ​https://github.com/speedy-net/speedy-net/commit/c76c82bc0b40ed06f11a27cd6f0e4deab2a70d47

[Claude Paroz]

Still I wonder if the performance gain is worth the loss of consistency. I wouldn't mind if exclude was defined to always be a set, but having to code defensively by expecting list or set is a loss in my point of view.

[אורי]

Replying to Claude Paroz:

Still I wonder if the performance gain is worth the loss of consistency. I wouldn't mind if exclude was defined to always be a set, but having to code defensively by expecting list or set is a loss in my point of view.

I think logically it should be a set, because we don't need the same value twice and the order doesn't matter. But we should decide how to handle old code where it was a list. Maybe define it a set from now on, and write in the documentation that it must be a set. But I don't see any problem with including the following 4 lines every time we use exclude:

    if exclude is None:
        exclude = set()
    else:
        exclude = set(exclude)

By the way, since these 4 lines are repeated, maybe we can define a util function that takes exclude as an argument, and return a set. Either set() (if exclude is None) or set(exclude) otherwise.

[אורי]

Commit - ​https://github.com/speedy-net/speedy-net/commit/d0cd6791b717a9e3c874fe6ea489032830617030

I defined a convert_to_set util function. If you want you can take this function and use it in Django. Or maybe add it to the documentation that users can use it to convert exclude to a set (with one line).

[Mariusz Felisiak]

Replying to Claude Paroz:

Still I wonder if the performance gain is worth the loss of consistency. I wouldn't mind if exclude was defined to always be a set, but having to code defensively by expecting list or set is a loss in my point of view.

All built-in methods do exactly the same number of conversion as before 1ea7e3157d1f9b4db71e768d75ea57e47dbd49f9 just using set(...) instead of list(...). IMO we can document set as the preferable option, e.g.

docs/ref/models/instances.txt

@@ -252 +252 @@
 .. method:: Model.clean_fields(exclude=None)
 
 This method will validate all fields on your model. The optional ``exclude``
-argument lets you provide a list of field names to exclude from validation. It
-will raise a :exc:`~django.core.exceptions.ValidationError` if any fields fail
-validation.
+argument lets you provide a ``set`` of field names to exclude from validation.
+It will raise a :exc:`~django.core.exceptions.ValidationError` if any fields
+fail validation.
 
 The second step ``full_clean()`` performs is to call :meth:`Model.clean()`.
 This method should be overridden to perform custom validation on your model.
@@ -355 +355 @@
 :attr:`.Field.unique_for_date`, :attr:`.Field.unique_for_month`,
 :attr:`.Field.unique_for_year`, or :attr:`Meta.unique_together
 <django.db.models.Options.unique_together>` on your model instead of individual
-field values. The optional ``exclude`` argument allows you to provide a list of
-field names to exclude from validation. It will raise a
+field values. The optional ``exclude`` argument allows you to provide a ``set``
+of field names to exclude from validation. It will raise a
 :exc:`~django.core.exceptions.ValidationError` if any fields fail validation.
 
 :class:`~django.db.models.UniqueConstraint`\s defined in the
@@ -380 +380 @@
 
 This method validates all constraints defined in
 :attr:`Meta.constraints <django.db.models.Options.constraints>`. The
-optional ``exclude`` argument allows you to provide a list of field names to
+optional ``exclude`` argument allows you to provide a ``set`` of field names to
 exclude from validation. It will raise a
 :exc:`~django.core.exceptions.ValidationError` if any constraints fail
 validation.

docs/releases/4.1.txt

@@ -561 +561 @@
   ``URLResolver._callback_strs``, and ``URLPattern.lookup_str()`` are
   moved to ``django.contrib.admindocs.utils``.
 
+* Custom :meth:`.Model.clean_fields`, :meth:`.Model.validate_unique`, and
+  :meth:`.Model.validate_constraints` methods now convert an ``exclude``
+  value to the ``set``.
+
 .. _deprecated-features-4.1:
 

Also, passing lists still works, but would not be documented anymore. What do you think?

[Claude Paroz]

All built-in methods do exactly the same number of conversion as before 1ea7e3157d1f9b4db71e768d75ea57e47dbd49f9 just using set(...) instead of list(...).

The problem is not in Django code, the problem is in user code when you have to do smart custom things with exclude, and you don't know if you get a list or a set, so you are forced to cast the variable each time before starting to manipulate it. If different sort of types in the same category (typically iterables) can be seen as a feature, I don't like the list/set potential mix.

IMO we can document set as the preferable option, e.g.

+1, I think this is going in the right direction.

[Carlton Gibson]

Yes, agreed +1 (since you CC'd me :)

[Mariusz Felisiak]

​PR

[GitHub <noreply@…>]

In 90aabd73:

Fixed #33724 -- Doc'd exclude argument changes in model validation.

Thanks אורי for the report.

Follow up to 1ea7e3157d1f9b4db71e768d75ea57e47dbd49f9.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 3d4bab2:

[4.1.x] Fixed #33724 -- Doc'd exclude argument changes in model validation.

Thanks אורי for the report.

Follow up to 1ea7e3157d1f9b4db71e768d75ea57e47dbd49f9.
Backport of 90aabd730a2a434c227faf8a927b0e2ccd67e291 from main