diff --git a/.gitignore b/.gitignore index aca3221..09ebb5e 100644 --- a/.gitignore +++ b/.gitignore @@ -15,6 +15,9 @@ __pycache__/ codekit-config.json .pycharm_helpers/ +# Virtual environment +.venv + # Distribution / packaging .Python env/ diff --git a/CHANGELOG.rst b/CHANGELOG.rst index c6a4ce9..76d0741 100644 --- a/CHANGELOG.rst +++ b/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 diff --git a/opaque_keys/__init__.py b/opaque_keys/__init__.py index 278bc6e..f5f6c12 100644 --- a/opaque_keys/__init__.py +++ b/opaque_keys/__init__.py @@ -14,7 +14,7 @@ from stevedore.enabled import EnabledExtensionManager -__version__ = '3.0.0' +__version__ = '3.1.0' class InvalidKeyError(Exception): diff --git a/opaque_keys/edx/django/models.py b/opaque_keys/edx/django/models.py index 15c8e25..23f6a10 100644 --- a/opaque_keys/edx/django/models.py +++ b/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 diff --git a/opaque_keys/edx/django/tests/models.py b/opaque_keys/edx/django/tests/models.py index 23f7924..3cb2a5c 100644 --- a/opaque_keys/edx/django/tests/models.py +++ b/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) diff --git a/opaque_keys/edx/django/tests/test_models.py b/opaque_keys/edx/django/tests/test_models.py index 4465b01..4ffe40f 100644 --- a/opaque_keys/edx/django/tests/test_models.py +++ b/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()