Skip to content

Commit

Permalink
refactor: rename fields from "key" to "_key"
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
ormsbee committed Feb 14, 2024
1 parent af77fd2 commit e4646bc
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 5 deletions.
Original file line number Diff line number Diff line change
@@ -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),
),
]
8 changes: 7 additions & 1 deletion openedx_learning/core/components/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
#
Expand Down
Original file line number Diff line number Diff line change
@@ -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),
),
]
16 changes: 14 additions & 2 deletions openedx_learning/core/publishing/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions openedx_learning/lib/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def immutable_uuid_field() -> models.UUIDField:
)


def key_field() -> MultiCollationCharField:
def key_field(**kwargs) -> MultiCollationCharField:
"""
Externally created Identifier fields.
Expand All @@ -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:
Expand Down

0 comments on commit e4646bc

Please sign in to comment.