Fixed #27910 -- Added enumeration helpers for use in Field.choices

[ngnpope]

Ticket #27910
Supersedes PR #11223

Improvements over the previous pull request:

• Simplified definition by moving everything from ChoiceEnum into ChoiceEnumMeta.
• Added additional properties for fetching labels, values, and names.
• Used enum.unique() to prevent duplicate values.
• Moved all documentation from django.db.models.enums into docs/ref/models/fields.txt.
• Improved and expanded documentation in docs/ref/models/fields.txt.
• Improved and expanded tests in tests/model_fields/test_choiceenum.py.
• Improved messages in exceptions.

Things left to do:

• Update documentation in docs/topics/db/models.txt.
• Add documentation for the .validate() method.
• Check whether the Functional API works; add tests and documentation.
• Check handling of blank values in choices, e.g. '' or None.
  • Handling None is potentially problematic as were restricted to int or str.

[ngnpope]

@shaib: Could you explain the use case for the .validate() method?

@felixxm: This is largely done. The main issue remaining is handling of blank values using None which doesn't work because ChoiceIntEnum subclasses int and ChoiceStrEnum subclasses str. For int, using None blows up, and for str, using None coerces it to 'None'. I've added some expected failure test cases. We have the following options:

1. See if we can define some sort of composite NullableInt and NullableStr types.
2. Use some sentinel values that are valid that can be forced to be None somehow.
3. Document that overriding the empty value to use a custom label is not supported.

It would be a shame if 3 were the only option here.

[felixxm]

@pope1ni Thanks 👍 I will review docs and tests in the next week.

[shaib]

@shaib: Could you explain the use case for the .validate() method?

It was motivated by https://groups.google.com/d/msgid/django-developers/CAL13Cg_KEtgmNzRo%3DC8cnCiNsukr5YOCTv3MF8AybV%3D%3DiUXP0g%40mail.gmail.com

@felixxm: This is largely done. The main issue remaining is handling of blank values using None which doesn't work because ChoiceIntEnum subclasses int and ChoiceStrEnum subclasses str. For int, using None blows up, and for str, using None coerces it to 'None'. I've added some expected failure test cases. We have the following options:

1. See if we can define some sort of composite `NullableInt` and `NullableStr` types.

2. Use some sentinel values that are valid that can be forced to be `None` somehow.

No, there's no need for these. Take a look at the loop starting at line 14 of enums.py -- it clearly shows that the values assigned to enum members in the source don't need to be valid members of the type (tuples are not ints). We can decide that e.g. if we reach line 26 with value is None then, instead of dict.__setitem__(classdict, key, value) we do dict.__delitem__(classdict, key). The only difference is that the properties will need to use cls._value2label_map_.keys() instead of cls.__members__.values().

I think an even better variation of this idea is, instead of using the None value as sentinel, to use a special member name -- say, __empty__ or __no_choice__; this way, the user doesn't need to repeat None, doesn't have to invent a name that will never be used, and gets the right clue that this is not going to be a member of the enum.

[ngnpope]

I've pushed a fixup! which removes validate() and implements __contains__() to make things work as expected according to James' comment:

>>> class Suit(models.ChoiceIntEnum):
...     DIAMOND = 1, _('Diamond')
...     SPADE = 2, _('Spade')
...     HEART = 3, _('Heart')
...     CLUB = 4, _('Club')
...
>>> Suit.DIAMOND in Suit
True
>>> 1 in Suit
True
>>> 0 in Suit
False

I will look into the label for the empty value soon.

[ngnpope]

@felixxm @shaib I think that last major things have been addressed here and we're ready for review.

[shaib]

Main code and docs look good to me. Haven't gone through tests in detail, but they seem pretty extensive too.

[ngnpope]

Have rebased this again. Is there anything else to do @carltongibson / @felixxm?

[frnhr]

Perhaps add tests and/or docs for usage with other types of fields, besides str and int? Because choices is defined on Field after all, available to all field types.

I am trying to get it working with a DateField but meeting little success. Maybe I'm missing something obvious?

class ChoiceDateEnum(datetime.date, enum.Enum, metaclass=ChoiceEnumMeta):
    """Class for creating an enum date choices."""

    def _generate_next_value_(name, start, count, last_values):
        return name  # what goes here?


class StartChoices(ChoiceDateEnum):
    mid2019 = datetime.date(2019, 6, 1), 'Middle of 2019'
    start2019 = datetime.date(2019, 1, 1), 'Start of 2019'
    mid2018 = datetime.date(2018, 6, 1), 'Middle of 2018'
    # ...


class MyHalfYearModel(models.Model):
    start = models.DateField(choices=StartChoices.choices, null=True, blank=True)
    # ...

This produces TypeError: an integer is required (got type datetime.date) at import time. Full traceback: https://dpaste.de/EU1V

---

Original (old-style) choices:

    START_CHOICES = (
        (datetime.date(2019, 6, 1), 'Middle of 2019'),
        (datetime.date(2019, 1, 1), 'Start of 2019'),
        (datetime.date(2018, 6, 1), 'Middle of 2018'),
        # ...
    )

[shaib]

@frnhr I'm getting the same error when trying to define a date-based enum with no relation to ChoiceEnum:

>>> from enum import Enum
>>> from datetime import date
>>> class MyEnum(date, Enum):
...   A = date(2014, 1, 1)
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.7/enum.py", line 218, in __new__
    enum_member = __new__(enum_class, *args)
TypeError: an integer is required (got type datetime.date)

So I'm guessing there's little we can do here. I would try something like using the str() of the dates as values, but that would need to be taken care of in ChoiceDateEnum's metaclass.

In my taste, this use-case falls a little out of what Django should actively support.

[frnhr]

Fair enough, but we should at least mention this in the docs. I doubt that I will be the only one attempting to use it with a field that is not int or str.

Especially because it looks like it should work, and it is documented right next to the old-style choices which do work for all field types.

[shaib]

Just to clarify my PG comment: This was all about the database engines. The fact that Django forces you to specify a max_length on a CharField is unrelated.

On Postgres, the internal representation of different text types is the same (that's why the performance is the same). So when you change from varchar(5) to varchar(10) all that changes is the validation rules. This is an easy migration on the database side, the only change is in metadata. There can be validations and castings, which can be lengthy, but there can be all kinds of mitigations and the whole thing is generally not very painful.

Oracle, for example (at least in the versions of 6-7 years ago when I was using it) was actually keeping rows together, and allocating the right number of characters in each row for each field. Thus, a similar change implies a change in the layout of the table's storage, which can be very expensive -- and could mean a long downtime for the migration.

SQLite isn't usually discussed much in terms of effects on production systems, but just for good measure -- last I checked, every such change implied a complete recreation of the table.

[ngnpope]

Ok. Changes made. Thanks Shai.

[ngnpope]

OK, agreed. Let's leave it to a system check. Will look to try and add one later in a separate PR because this is always a source of pain.

Created PR #11742.

[felixxm]

@pope1ni I pushed minor edits, rebased from master, simplified tests, and moved choices tests to a separate directory.

I noticed that these lines are not covered, I don't think that we still need this catch or if we really want to catch ValueError then we should do this in

super().__new__(metacls, classname, bases, classdict)

I'm working on docs.

[ngnpope]

Thanks for the polishing Felix.

I just noticed a couple of little things:

[ngnpope]

I noticed that these lines are not covered, I don't think that we still need this catch...

So originally when we only covered int and str, we restricted this to expect value, label and there were some tests for too few or too many items. Because enum values are not an instance of the concrete data type, but a tuple of arguments passed to the constructor of that concrete data type, we changed tack. As such, yes, these lines are obsolete - we rely on the underlying data type to blow up if the arguments are invalid. We're just assuming that the final item in the tuple for the member value is going to be the label if it is a (lazy) string.

[felixxm]

@pope1ni OK, we are almost there, I pushed all edits, but I also noticed one issue with migrations that I would try to fix, i.e. default is not evaluated and migrations crashes, e.g.

    field=models.CharField(choices=[('FR', 'Freshman'), ('SO', 'Sophomore'), ('JR', 'Junior'), ('SR', 'Senior'), ('GR', 'Graduate'), ('XX', 'Xxx')], default=test_one.models.YearInSchool('FR'), max_length=2),
AttributeError: module 'test_one.models' has no attribute 'YearInSchool'

the old-fashioned choices evaluates properly to the default='FR'.

[shaib]

That's not an issue with default per se, it's an issue with defining the choices enum within the model class, and the auto-migration-composer not handling nested classes.

[ngnpope]

I think that could be handled by overriding __repr__ to use __qualname__? This will also work if default is defined to be YearInSchool.FRESHMAN.value.

[felixxm]

@shaib Yes, I was able to fix this quite easily with:

diff --git a/django/db/migrations/serializer.py b/django/db/migrations/serializer.py
index 1f1b3f4f20..c6bfd90621 100644
--- a/django/db/migrations/serializer.py
+++ b/django/db/migrations/serializer.py
@@ -45,6 +45,9 @@ class BaseSimpleSerializer(BaseSerializer):
     def serialize(self):
         return repr(self.value), set()
 
+class ChoicesSerializer(BaseSerializer):
+    def serialize(self):
+        return repr(self.value.value), set()
 
 class DateTimeSerializer(BaseSerializer):
     """For datetime.*, except datetime.datetime."""
@@ -279,6 +282,7 @@ class Serializer:
         set: SetSerializer,
         tuple: TupleSerializer,
         dict: DictionarySerializer,
+        models.Choices: ChoicesSerializer,
         enum.Enum: EnumSerializer,
         datetime.datetime: DatetimeDatetimeSerializer,
         (datetime.date, datetime.timedelta, datetime.time): DateTimeSerializer,

after that it evaluates to default='FR'. What do you think?

[ngnpope]

What do you think?

Good idea. Should seamlessly work for custom choices then too. Can you try with one that subclasses datetime.date or ipaddress.IPv4Address to see if imports are resolved appropriately?

[felixxm]

Yes, I'm preparing tests.

[felixxm]

I pushed fix.

[felixxm]

Thanks y'all 🚀 Many thanks to Nick and Shai 🙇‍♂️

Now we can celebrate 🍾 💃 🚀 🎊

[shaib]

Many thanks, @pope1ni and @felixxm! When I started this, I never thought it would make it to the list of major features of the release... but you have grown it bigger than I expected. It is a reason for celebration indeed.

[ngnpope]

Thanks both of you!