Used composite PKs for tracdb instead of hacks

Author: bmispelon

djangoproject/settings/common.py

@@ -175,7 +175,6 @@
 SESSION_COOKIE_HTTPONLY = True
 
 SILENCED_SYSTEM_CHECKS = [
-    "fields.W342",  # tracdb has ForeignKey(unique=True) in lieu of multi-col PKs
     "security.W008",  # SSL redirect is handled by nginx
     "security.W009",  # SECRET_KEY is setup through Ansible secrets
 ]

tracdb/models.py

@@ -9,38 +9,6 @@
     * All the session and permission tables: they're just not needed.
     * Enum: I don't know what this is or what it's for.
     * NodeChange: Ditto.
-
-These models are far from perfect but are Good Enough(tm) to get some useful data out.
-
-One important mismatch between these models and the Trac database has to do with
-composite primary keys. Trac uses them for several tables, but Django does not support
-them yet (ticket #373).
-
-These are the tables that use them:
-
-    * ticket_custom (model TicketCustom)
-    * ticket_change (model TicketChange)
-    * wiki (model Wiki)
-    * attachment (model Attachment)
-
-To make these work with Django (for some definition of the word "work") we mark only
-one of their field as being the primary key (primary_key=True).
-This is obviously incorrect but — somewhat suprisingly — it doesn't break **everything**
-and the little that does actually work is good enough for what we're trying to do:
-
-    * Model.objects.create(...) correctly creates the object in the db
-    * Most queryset/manager methods work (in particular filter(), exclude(), all()
-      and count())
-
-On the other hand, here's what absolutely DOES NOT work (the list is sadly not
-exhaustive):
-
-    * Updating a model instance with save() will update ALL ROWS that happen to share
-      the value for the field used as the "fake" primary key if they exist (resulting
-      in a DBError)
-    * The admin won't work (the "pk" field shortcut can't be used reliably since it can
-      return multiple rows)
-
 """
 
 from datetime import date
@@ -161,11 +129,11 @@ def __str__(self):
 
 
 class TicketCustom(models.Model):
+    pk = models.CompositePrimaryKey("ticket", "name")
     ticket = models.ForeignKey(
         Ticket,
         related_name="custom_fields",
         db_column="ticket",
-        primary_key=True,  # XXX See note at the top about composite pk
         on_delete=models.DO_NOTHING,
     )
     name = models.TextField()
@@ -180,11 +148,11 @@ def __str__(self):
 
 
 class TicketChange(models.Model):
+    pk = models.CompositePrimaryKey("ticket", "_time", "field")
     ticket = models.ForeignKey(
         Ticket,
         related_name="changes",
         db_column="ticket",
-        primary_key=True,  # XXX See note at the top about composite pk
         on_delete=models.DO_NOTHING,
     )
     author = models.TextField()
@@ -292,9 +260,8 @@ def __str__(self):
 
 
 class Wiki(models.Model):
-    name = models.TextField(
-        primary_key=True
-    )  # XXX See note at the top about composite pk
+    pk = models.CompositePrimaryKey("name", "version")
+    name = models.TextField()
     version = models.IntegerField()
     _time = models.BigIntegerField(db_column="time")
     time = time_property("_time")
@@ -312,10 +279,9 @@ def __str__(self):
 
 
 class Attachment(models.Model):
+    pk = models.CompositePrimaryKey("type", "id", "filename")
     type = models.TextField()
-    id = models.TextField(
-        primary_key=True
-    )  # XXX See note at the top about composite pk
+    id = models.TextField()
     filename = models.TextField()
     size = models.IntegerField()
     _time = models.BigIntegerField(db_column="time")

tracdb/testutils.py

@@ -1,60 +1,5 @@
-from copy import deepcopy
-
 from django.apps import apps
-from django.db import connections, models
-
-# There's more models with a fake composite pk, but we don't test them at the moment.
-_MODELS_WITH_FAKE_COMPOSITE_PK = {"ticketcustom"}
-
-
-def _get_pk_field(model):
-    """
-    Return the pk field for the given model.
-    Raise ValueError if none or more than one is found.
-    """
-    pks = [field for field in model._meta.get_fields() if field.primary_key]
-    if len(pks) == 0:
-        raise ValueError(f"No primary key field found for model {model._meta.label}")
-    elif len(pks) > 1:
-        raise ValueError(
-            f"Found more than one primary key field for model {model._meta.label}"
-        )
-    else:
-        return pks[0]
-
-
-def _replace_primary_key_field_with_autofield(model, schema_editor):
-    """
-    See section about composite pks in the docstring for models.py to get some context
-    for this.
-
-    In short, some models define a field as `primary_key=True` but that field is not
-    actually a primary key. In particular that field is not supposed to be unique, which
-    interferes with our tests.
-
-    For those models, we remove the `primary_key` flag from the field, and we add a
-    new `testid` autofield. This makes the models easier to manipulate in the tests.
-    """
-    old_pk_field = _get_pk_field(model)
-    del old_pk_field.unique
-    new_pk_field = deepcopy(old_pk_field)
-    new_pk_field.primary_key = False
-    schema_editor.alter_field(
-        model=model, old_field=old_pk_field, new_field=new_pk_field
-    )
-
-    autofield = models.AutoField(primary_key=True)
-    autofield.set_attributes_from_name("testid")
-    schema_editor.add_field(model=model, field=autofield)
-
-
-def _create_db_table_for_model(model, schema_editor):
-    """
-    Use the schema editor API to create the db table for the given (unmanaged) model.
-    """
-    schema_editor.create_model(model)
-    if model._meta.model_name in _MODELS_WITH_FAKE_COMPOSITE_PK:
-        _replace_primary_key_field_with_autofield(model, schema_editor)
+from django.db import connections
 
 
 def create_db_tables_for_unmanaged_models(schema_editor):
@@ -65,7 +10,7 @@ def create_db_tables_for_unmanaged_models(schema_editor):
     for model in appconfig.get_models():
         if model._meta.managed:
             continue
-        _create_db_table_for_model(model, schema_editor)
+        schema_editor.create_model(model)
 
 
 def destroy_db_tables_for_unmanaged_models(schema_editor):