Opened 23 months ago

Closed 22 months ago

Last modified 22 months ago

#33724 closed Bug (fixed)

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

Reported by: אורי Owned by: Mariusz Felisiak
Component: Database layer (models, ORM) Version: 4.1
Severity: Release blocker Keywords:
Cc: Carlton Gibson Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (15)

comment:1 by Mariusz Felisiak, 23 months ago

Component: UncategorizedDatabase layer (models, ORM)
Severity: NormalRelease blocker
Summary: Django 4.1 alpha - changing from list to set in `exclude` raises errors, and is not documentedChanging from list to set in `exclude` raises errors, and is not documented.
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

Agreed, we should document this change, maybe:

  • docs/ref/models/instances.txt

    diff --git a/docs/ref/models/instances.txt b/docs/ref/models/instances.txt
    index 9aa9486f4a..b844ac3ea4 100644
    a b The first step ``full_clean()`` performs is to clean each individual field.  
    252252.. method:: Model.clean_fields(exclude=None)
    253253
    254254This method will validate all fields on your model. The optional ``exclude``
    255 argument lets you provide a list of field names to exclude from validation. It
    256 will raise a :exc:`~django.core.exceptions.ValidationError` if any fields fail
    257 validation.
     255argument lets you provide a set or list of field names to exclude from
     256validation. It will raise a :exc:`~django.core.exceptions.ValidationError` if
     257any fields fail validation.
    258258
    259259The second step ``full_clean()`` performs is to call :meth:`Model.clean()`.
    260260This method should be overridden to perform custom validation on your model.
    uniqueness constraints defined via :attr:`.Field.unique`,  
    355355:attr:`.Field.unique_for_date`, :attr:`.Field.unique_for_month`,
    356356:attr:`.Field.unique_for_year`, or :attr:`Meta.unique_together
    357357<django.db.models.Options.unique_together>` on your model instead of individual
    358 field values. The optional ``exclude`` argument allows you to provide a list of
    359 field names to exclude from validation. It will raise a
     358field values. The optional ``exclude`` argument allows you to provide a set or
     359list of field names to exclude from validation. It will raise a
    360360:exc:`~django.core.exceptions.ValidationError` if any fields fail validation.
    361361
    362362:class:`~django.db.models.UniqueConstraint`\s defined in the
    Finally, ``full_clean()`` will check any other constraints on your model.  
    380380
    381381This method validates all constraints defined in
    382382:attr:`Meta.constraints <django.db.models.Options.constraints>`. The
    383 optional ``exclude`` argument allows you to provide a list of field names to
    384 exclude from validation. It will raise a
     383optional ``exclude`` argument allows you to provide a set or list of field
     384names to exclude from validation. It will raise a
    385385:exc:`~django.core.exceptions.ValidationError` if any constraints fail
    386386validation.
    387387
  • docs/releases/4.1.txt

    diff --git a/docs/releases/4.1.txt b/docs/releases/4.1.txt
    index 7b922256ec..447824ab7f 100644
    a b Miscellaneous  
    561561  ``URLResolver._callback_strs``, and ``URLPattern.lookup_str()`` are
    562562  moved to ``django.contrib.admindocs.utils``.
    563563
     564* Custom :meth:`.Model.clean_fields`, :meth:`.Model.validate_unique`, and
     565  :meth:`.Model.validate_constraints` methods must handle the ``exclude``
     566  values passed as ``set``.
     567
    564568.. _deprecated-features-4.1:
    565569
    566570Features 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')

in reply to:  1 ; comment:2 by אורי, 23 months ago

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.

in reply to:  2 ; comment:3 by Mariusz Felisiak, 23 months ago

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)
    ...

in reply to:  3 comment:4 by אורי, 23 months ago

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.

comment:5 by אורי, 23 months ago

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

comment:6 by Claude Paroz, 23 months ago

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.

in reply to:  6 comment:7 by אורי, 23 months ago

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.

comment:8 by אורי, 23 months ago

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).

in reply to:  6 comment:9 by Mariusz Felisiak, 23 months ago

Cc: Carlton Gibson added

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

    diff --git a/docs/ref/models/instances.txt b/docs/ref/models/instances.txt
    index 9aa9486f4a..8601a19e64 100644
    a b The first step ``full_clean()`` performs is to clean each individual field.  
    252252.. method:: Model.clean_fields(exclude=None)
    253253
    254254This method will validate all fields on your model. The optional ``exclude``
    255 argument lets you provide a list of field names to exclude from validation. It
    256 will raise a :exc:`~django.core.exceptions.ValidationError` if any fields fail
    257 validation.
     255argument lets you provide a ``set`` of field names to exclude from validation.
     256It will raise a :exc:`~django.core.exceptions.ValidationError` if any fields
     257fail validation.
    258258
    259259The second step ``full_clean()`` performs is to call :meth:`Model.clean()`.
    260260This method should be overridden to perform custom validation on your model.
    uniqueness constraints defined via :attr:`.Field.unique`,  
    355355:attr:`.Field.unique_for_date`, :attr:`.Field.unique_for_month`,
    356356:attr:`.Field.unique_for_year`, or :attr:`Meta.unique_together
    357357<django.db.models.Options.unique_together>` on your model instead of individual
    358 field values. The optional ``exclude`` argument allows you to provide a list of
    359 field names to exclude from validation. It will raise a
     358field values. The optional ``exclude`` argument allows you to provide a ``set``
     359of field names to exclude from validation. It will raise a
    360360:exc:`~django.core.exceptions.ValidationError` if any fields fail validation.
    361361
    362362:class:`~django.db.models.UniqueConstraint`\s defined in the
    Finally, ``full_clean()`` will check any other constraints on your model.  
    380380
    381381This method validates all constraints defined in
    382382:attr:`Meta.constraints <django.db.models.Options.constraints>`. The
    383 optional ``exclude`` argument allows you to provide a list of field names to
     383optional ``exclude`` argument allows you to provide a ``set`` of field names to
    384384exclude from validation. It will raise a
    385385:exc:`~django.core.exceptions.ValidationError` if any constraints fail
    386386validation.
  • docs/releases/4.1.txt

    diff --git a/docs/releases/4.1.txt b/docs/releases/4.1.txt
    index 7b922256ec..a1ec53e2ac 100644
    a b Miscellaneous  
    561561  ``URLResolver._callback_strs``, and ``URLPattern.lookup_str()`` are
    562562  moved to ``django.contrib.admindocs.utils``.
    563563
     564* Custom :meth:`.Model.clean_fields`, :meth:`.Model.validate_unique`, and
     565  :meth:`.Model.validate_constraints` methods now convert an ``exclude``
     566  value to the ``set``.
     567
    564568.. _deprecated-features-4.1:
    565569

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

comment:10 by Claude Paroz, 23 months ago

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.

comment:11 by Carlton Gibson, 23 months ago

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

comment:12 by Mariusz Felisiak, 23 months ago

Has patch: set
Owner: changed from nobody to Mariusz Felisiak
Status: newassigned

comment:13 by Carlton Gibson, 22 months ago

Triage Stage: AcceptedReady for checkin

comment:14 by GitHub <noreply@…>, 22 months ago

Resolution: fixed
Status: assignedclosed

In 90aabd73:

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

Thanks אורי for the report.

Follow up to 1ea7e3157d1f9b4db71e768d75ea57e47dbd49f9.

comment:15 by Mariusz Felisiak <felisiak.mariusz@…>, 22 months ago

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

Note: See TracTickets for help on using tickets.
Back to Top