From e4646bcee05144679c80e59099c48eb9bfb7b072 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 14 Feb 2024 17:05:41 -0500 Subject: [PATCH] refactor: rename fields from "key" to "_key" We're doing this because "key" is a reserved keyword for MySQL. While Django will escape this for us automatically, we've historically seen downstream tooling break when column names use reserved keywords. Also, it makes things harder for people who are querying the database directly and might not realize that they need to esacape column names. In any event, this has caused enough pain in the past to warrant a check in edx-platform's CI, where it checks column names against a whole list of reserved keywords for several systems: https://github.com/openedx/edx-django-release-util/blob/master/release_util/management/commands/default_reserved_keywords.yml It's likely that we'll want rename at least some of these columns later, but this lets us introduce them into the system without worrying that they'll break anything. --- .../0002_alter_componentversioncontent_key.py | 20 +++++++++++++++ openedx_learning/core/components/models.py | 8 +++++- ...0002_alter_learningpackage_key_and_more.py | 25 +++++++++++++++++++ openedx_learning/core/publishing/models.py | 16 ++++++++++-- openedx_learning/lib/fields.py | 4 +-- 5 files changed, 68 insertions(+), 5 deletions(-) create mode 100644 openedx_learning/core/components/migrations/0002_alter_componentversioncontent_key.py create mode 100644 openedx_learning/core/publishing/migrations/0002_alter_learningpackage_key_and_more.py diff --git a/openedx_learning/core/components/migrations/0002_alter_componentversioncontent_key.py b/openedx_learning/core/components/migrations/0002_alter_componentversioncontent_key.py new file mode 100644 index 00000000..ee12970d --- /dev/null +++ b/openedx_learning/core/components/migrations/0002_alter_componentversioncontent_key.py @@ -0,0 +1,20 @@ +# Generated by Django 4.2.10 on 2024-02-14 22:02 + +from django.db import migrations + +import openedx_learning.lib.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_components', '0001_initial'), + ] + + operations = [ + migrations.AlterField( + model_name='componentversioncontent', + name='key', + field=openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, db_column='_key', max_length=500), + ), + ] diff --git a/openedx_learning/core/components/models.py b/openedx_learning/core/components/models.py index 59dc7700..b3d38463 100644 --- a/openedx_learning/core/components/models.py +++ b/openedx_learning/core/components/models.py @@ -237,7 +237,13 @@ class ComponentVersionContent(models.Model): content = models.ForeignKey(Content, on_delete=models.RESTRICT) uuid = immutable_uuid_field() - key = key_field() + + # "key" is a reserved word for MySQL, so we're temporarily using the column + # name of "_key" to avoid breaking downstream tooling. A possible + # alternative name for this would be "path", since it's most often used as + # an internal file path. However, we might also want to put special + # identifiers that don't map as cleanly to file paths at some point. + key = key_field(db_column="_key") # Long explanation for the ``learner_downloadable`` field: # diff --git a/openedx_learning/core/publishing/migrations/0002_alter_learningpackage_key_and_more.py b/openedx_learning/core/publishing/migrations/0002_alter_learningpackage_key_and_more.py new file mode 100644 index 00000000..0a4c3b48 --- /dev/null +++ b/openedx_learning/core/publishing/migrations/0002_alter_learningpackage_key_and_more.py @@ -0,0 +1,25 @@ +# Generated by Django 4.2.10 on 2024-02-14 22:02 + +from django.db import migrations + +import openedx_learning.lib.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('oel_publishing', '0001_initial'), + ] + + operations = [ + migrations.AlterField( + model_name='learningpackage', + name='key', + field=openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, db_column='_key', max_length=500), + ), + migrations.AlterField( + model_name='publishableentity', + name='key', + field=openedx_learning.lib.fields.MultiCollationCharField(db_collations={'mysql': 'utf8mb4_bin', 'sqlite': 'BINARY'}, db_column='_key', max_length=500), + ), + ] diff --git a/openedx_learning/core/publishing/models.py b/openedx_learning/core/publishing/models.py index 3f668a9f..c6e82560 100644 --- a/openedx_learning/core/publishing/models.py +++ b/openedx_learning/core/publishing/models.py @@ -38,7 +38,14 @@ class LearningPackage(models.Model): # type: ignore[django-manager-missing] id = models.AutoField(primary_key=True) uuid = immutable_uuid_field() - key = key_field() + + # "key" is a reserved word for MySQL, so we're temporarily using the column + # name of "_key" to avoid breaking downstream tooling. There's an open + # question as to whether this field needs to exist at all, or whether the + # top level library key it's currently used for should be entirely in the + # LibraryContent model. + key = key_field(db_column="_key") + title = case_insensitive_char_field(max_length=500, blank=False) # TODO: We should probably defer this field, since many things pull back @@ -172,7 +179,12 @@ class PublishableEntity(models.Model): on_delete=models.CASCADE, related_name="publishable_entities", ) - key = key_field() + + # "key" is a reserved word for MySQL, so we're temporarily using the column + # name of "_key" to avoid breaking downstream tooling. Consider renaming + # this later. + key = key_field(db_column="_key") + created = manual_date_time_field() created_by = models.ForeignKey( settings.AUTH_USER_MODEL, diff --git a/openedx_learning/lib/fields.py b/openedx_learning/lib/fields.py index 95e41bef..99b7d525 100644 --- a/openedx_learning/lib/fields.py +++ b/openedx_learning/lib/fields.py @@ -109,7 +109,7 @@ def immutable_uuid_field() -> models.UUIDField: ) -def key_field() -> MultiCollationCharField: +def key_field(**kwargs) -> MultiCollationCharField: """ Externally created Identifier fields. @@ -120,7 +120,7 @@ def key_field() -> MultiCollationCharField: Other apps should *not* make references to these values directly, since these values may in theory change (even if this is rare in practice). """ - return case_sensitive_char_field(max_length=500, blank=False) + return case_sensitive_char_field(max_length=500, blank=False, **kwargs) def hash_field() -> models.CharField: