feat: CourseKeyField can now be optionally case-sensitive, has default max_length [FC-0117]

.gitignore

@@ -15,6 +15,9 @@ __pycache__/
 codekit-config.json
 .pycharm_helpers/
 
+# Virtual environment
+.venv
+
 # Distribution / packaging
 .Python
 env/

CHANGELOG.rst

@@ -1,3 +1,12 @@
+# 3.1.0
+
+* The Django OpaqueKeyField subclasses like CourseKeyField now specify a default
+  max_length of 255 characters (previously, no default was specified).
+* The Django OpaqueKeyField subclasses now are case-insensitive on SQLite by
+  default, to match the behavior they had on MySQL.
+* The Django OpaqueKeyField subclasses can now be made case-sensitive by passing
+  the new case_sensitive=True parameter (recommended).
+
 # 3.0.0
 
 * Breaking: LibraryItemKey is replaced by CollectionKey and ContainerKey

opaque_keys/__init__.py

@@ -14,7 +14,7 @@
 
 from stevedore.enabled import EnabledExtensionManager
 
-__version__ = '3.0.0'
+__version__ = '3.1.0'
 
 
 class InvalidKeyError(Exception):

opaque_keys/edx/django/models.py

@@ -9,7 +9,7 @@
 
 try:
     from django.core.exceptions import ValidationError
-    from django.db.models import CharField
+    from django.db.models import CharField, Field
     from django.db.models.lookups import IsNull
 except ImportError:  # pragma: no cover
     # Django is unavailable, none of the classes below will work,
@@ -96,8 +96,9 @@ class OpaqueKeyField(CreatorMixin, CharField):
 
     def __init__(self, *args, **kwargs):
         if self.KEY_CLASS is None:
-            raise ValueError('Must specify KEY_CLASS in OpaqueKeyField subclasses')
-
+            raise ValueError('Must specify KEY_CLASS in OpaqueKeyField subclasses')  # pragma: no cover
+        kwargs.setdefault("max_length", 255)  # Default max length for all opaque key fields
+        self.case_sensitive = kwargs.pop("case_sensitive", False)  # see self.db_parameters() for details
         super().__init__(*args, **kwargs)
 
     def to_python(self, value):  # pylint: disable=missing-function-docstring
@@ -164,6 +165,44 @@ def run_validators(self, value):
 
         return super().run_validators(value)
 
+    def db_parameters(self, connection):
+        """
+        Return database parameters for this field. This adds collation info, to
+        make the key field case-sensitive (optionally).
+
+        Previously, these fields were case-sensitive on SQLite and
+        case-insensitive on MySQL, which was not ideal. Now they are generally
+        case-insensitive by default (for backwards compatibility), and
+        case-sensitive if case_sensitive=True is specified (recommended!).
+        """
+        db_params = Field.db_parameters(self, connection)
+
+        if connection.vendor == "sqlite":
+            db_params["collation"] = "BINARY" if self.case_sensitive else "NOCASE"
+        elif connection.vendor == "mysql":  # pragma: no cover
+            db_params["collation"] = "utf8mb4_bin" if self.case_sensitive else "utf8mb4_unicode_ci"
+            # We're using utf8mb4_unicode_ci to keep MariaDB compatibility, since their collation support diverges after
+            # this. MySQL is now on utf8mb4_0900_ai_ci based on Unicode 9, while MariaDB has uca1400_ai_ci (Unicode 14).
+        elif connection.vendor == "postgresql":  # pragma: no cover
+            # PostgreSQL only provides case-sensitive collations by default. To create a case-insensitive column,
+            # we'd first need to use a migration to create a custom case-insensitive collation, but we don't use
+            # migrations for this opaque-keys library. Anyhow, using case-sensitivity across the board is less
+            # likely to cause bugs than the opposite.
+            pass
+
+        return db_params
+
+    def deconstruct(self):
+        """
+        How to serialize our Field for the migration file.
+
+        Just add our custom "case_sensitive" field if needed.
+        """
+        name, path, args, kwargs = super().deconstruct()
+        if self.case_sensitive:
+            kwargs["case_sensitive"] = True
+        return name, path, args, kwargs
+
 
 class OpaqueKeyFieldEmptyLookupIsNull(IsNull):
     """
@@ -191,6 +230,10 @@ class LearningContextKeyField(OpaqueKeyField):
     CourseKeyField instead, but if you are writing something more generic that
     could apply to any learning context (libraries, etc.), use this instead of
     CourseKeyField.
+
+    It is highly recommended to specify `case_sensitive=True`, because we
+    generally want the performance, exactness, and consistency of case-sensitive
+    values, but the default behavior is case-insensitive (except on PostgreSQL).
     """
     description = "A LearningContextKey object, saved to the DB in the form of a string"
     KEY_CLASS = LearningContextKey
@@ -205,6 +248,10 @@ class LearningContextKeyField(OpaqueKeyField):
 class CourseKeyField(OpaqueKeyField):
     """
     A django Field that stores a CourseKey object as a string.
+
+    It is highly recommended to specify `case_sensitive=True`, because we
+    generally want the performance, exactness, and consistency of case-sensitive
+    values, but the default behavior is case-insensitive (except on PostgreSQL).
     """
     description = "A CourseKey object, saved to the DB in the form of a string"
     KEY_CLASS = CourseKey
@@ -216,6 +263,10 @@ class CourseKeyField(OpaqueKeyField):
 class UsageKeyField(OpaqueKeyField):
     """
     A django Field that stores a UsageKey object as a string.
+
+    It is highly recommended to specify `case_sensitive=True`, because we
+    generally want the performance, exactness, and consistency of case-sensitive
+    values, but the default behavior is case-insensitive (except on PostgreSQL).
     """
     description = "A Location object, saved to the DB in the form of a string"
     KEY_CLASS = UsageKey
@@ -227,6 +278,10 @@ class UsageKeyField(OpaqueKeyField):
 class ContainerKeyField(OpaqueKeyField):
     """
     A django Field that stores a ContainerKey object as a string.
+
+    It is highly recommended to specify `case_sensitive=True`, because we
+    generally want the performance, exactness, and consistency of case-sensitive
+    values, but the default behavior is case-insensitive (except on PostgreSQL).
     """
     description = "A Location object, saved to the DB in the form of a string"
     KEY_CLASS = ContainerKey
@@ -238,6 +293,10 @@ class ContainerKeyField(OpaqueKeyField):
 class CollectionKeyField(OpaqueKeyField):
     """
     A django Field that stores a CollectionKey object as a string.
+
+    It is highly recommended to specify `case_sensitive=True`, because we
+    generally want the performance, exactness, and consistency of case-sensitive
+    values, but the default behavior is case-insensitive (except on PostgreSQL).
     """
     description = "A Location object, saved to the DB in the form of a string"
     KEY_CLASS = CollectionKey
@@ -249,6 +308,10 @@ class CollectionKeyField(OpaqueKeyField):
 class LocationKeyField(UsageKeyField):
     """
     A django Field that stores a UsageKey object as a string.
+
+    It is highly recommended to specify `case_sensitive=True`, because we
+    generally want the performance, exactness, and consistency of case-sensitive
+    values, but the default behavior is case-insensitive (except on PostgreSQL).
     """
 
     def __init__(self, *args, **kwargs):
@@ -259,6 +322,10 @@ def __init__(self, *args, **kwargs):
 class BlockTypeKeyField(OpaqueKeyField):
     """
     A django Field that stores a BlockTypeKey object as a string.
+
+    It is highly recommended to specify `case_sensitive=True`, because we
+    generally want the performance, exactness, and consistency of case-sensitive
+    values, but the default behavior is case-insensitive (except on PostgreSQL).
     """
     description = "A BlockTypeKey object, saved to the DB in the form of a string."
     KEY_CLASS = BlockTypeKey

opaque_keys/edx/django/tests/models.py

@@ -66,8 +66,9 @@ class ComplexModel(Model):
     """A Django Model for testing Course/Usage/Location/BlockType Key fields."""
 
     id = CharField(primary_key=True, max_length=255)  # pylint: disable=invalid-name
-    course_key = CourseKeyField(max_length=255, validators=[is_edx])
-    block_type_key = BlockTypeKeyField(max_length=255, blank=True)
-    usage_key = UsageKeyField(max_length=255, blank=False)
-    collection_key = CollectionKeyField(max_length=255, blank=False)
-    container_key = ContainerKeyField(max_length=255, blank=False)
+    course_key = CourseKeyField(validators=[is_edx])
+    course_key_cs = CourseKeyField(case_sensitive=True, max_length=100)
+    block_type_key = BlockTypeKeyField(blank=True)
+    usage_key = UsageKeyField(blank=False)
+    collection_key = CollectionKeyField(blank=False)
+    container_key = ContainerKeyField(blank=False)

opaque_keys/edx/django/tests/test_models.py

@@ -73,6 +73,7 @@ def setUp(self):
         self.model = ComplexModel(
             id='foobar',
             course_key=self.course_key,
+            course_key_cs=self.course_key,
             usage_key=self.usage_key,
             collection_key=self.collection_key,
             container_key=self.container_key,
@@ -91,6 +92,43 @@ def test_fetch_from_db_with_str(self):
         fetched = ComplexModel.objects.filter(course_key=str(self.course_key)).first()
         self.assertEqual(fetched, self.model)
 
+    def test_fetch_from_db_with_str_case_insensitive(self):
+        """Fetching keys should be case-insensitive by default for backwards compatibility"""
+        assert str(self.course_key).lower() != str(self.course_key)
+        fetched = ComplexModel.objects.get(course_key=str(self.course_key).lower())
+        assert str(fetched.course_key) == str(self.model.course_key)  # fetched has the correct capitalization though
+
+    def test_fetch_from_db_with_str_case_sensitive(self):
+        """Fetching keys should be case-sensitive when case_sensitive=True"""
+        assert str(self.course_key).lower() != str(self.course_key)
+        with self.assertRaises(ComplexModel.DoesNotExist):
+            ComplexModel.objects.get(course_key_cs=str(self.course_key).lower())
+        # But this works:
+        fetched = ComplexModel.objects.get(course_key_cs=str(self.course_key))
+        assert fetched.course_key == self.course_key
+
+    def test_custom_max_length(self):
+        """
+        Test that fields can override max_length to be different from the default of 255
+        """
+        key_100 = CourseKey.from_string(
+            "course-v1:fiftyfiftyfiftyfiftyfiftyfiftyfiftyfiftyfiftyfifty+thishasten+twenty-eight-more-characters",
+        )
+        key_101 = CourseKey.from_string(
+            "course-v1:fiftyfiftyfiftyfiftyfiftyfiftyfiftyfiftyfiftyfifty+thishasten+twenty-eight-more-charactersX",
+        )
+        # Note that ComplexModel.course_key_cs specifies max_length=100. So this should work:
+        assert len(str(key_100)) == 100
+        self.model.course_key_cs = key_100
+        self.model.full_clean()
+        # But this should fail:
+        assert len(str(key_101)) == 101
+        self.model.course_key_cs = key_101
+        with self.assertRaises(ValidationError):
+            self.model.full_clean()
+        # Note: we do not test the actual database constraints on length, because SQLite does not enforce it.
+        # But the Django validation above reflects the same limits, and MySQL should enforce them.
+
     def test_validation_no_errors(self):
         self.model.clean_fields()