From 94cfd767f94924e8189a3f0e029f56a19541e7a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=9Clgen=20Sar=C4=B1kavak?= Date: Fri, 24 Jan 2025 16:41:00 +0300 Subject: [PATCH 1/2] Upgrade Django version to 5.2 --- .pre-commit-config.yaml | 1 + requirements/common.txt | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index a89afbda5..c5199236c 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -36,6 +36,7 @@ repos: rev: "1.22.2" hooks: - id: django-upgrade + # TODO: Update to 5.2 when django-upgrade supports it args: [--target-version, "5.1"] - repo: https://github.com/psf/black rev: 24.10.0 diff --git a/requirements/common.txt b/requirements/common.txt index 60a92072a..fa850c42c 100644 --- a/requirements/common.txt +++ b/requirements/common.txt @@ -7,7 +7,7 @@ django-push @ git+https://github.com/brutasse/django-push.git@22fda99641cfbd2f30 django-read-only==1.18.0 django-recaptcha==4.0.0 django-registration-redux==2.13 -Django==5.1.5 +Django==5.2a1 docutils==0.21.2 feedparser==6.0.11 Jinja2==3.1.5 From 1cf525c12d5cbf162290dc5d40e298aa426f3777 Mon Sep 17 00:00:00 2001 From: Baptiste Mispelon Date: Mon, 27 Jan 2025 13:45:26 +0100 Subject: [PATCH 2/2] Used actual composite PKs for Trac models --- djangoproject/settings/common.py | 1 - tracdb/models.py | 46 ++++--------------------- tracdb/testutils.py | 59 ++------------------------------ 3 files changed, 8 insertions(+), 98 deletions(-) diff --git a/djangoproject/settings/common.py b/djangoproject/settings/common.py index 7ba635b60..2736c5a76 100644 --- a/djangoproject/settings/common.py +++ b/djangoproject/settings/common.py @@ -172,7 +172,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 ] diff --git a/tracdb/models.py b/tracdb/models.py index 67a91990b..6dc819344 100644 --- a/tracdb/models.py +++ b/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") diff --git a/tracdb/testutils.py b/tracdb/testutils.py index 8ac6192e8..13a31e41a 100644 --- a/tracdb/testutils.py +++ b/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):