Used tracdb instead of Trac RPC endpoint in dashboard

dashboard/models.py

@@ -1,7 +1,6 @@
 import ast
 import calendar
 import datetime
-import xmlrpc.client
 
 import requests
 from django.conf import settings
@@ -10,6 +9,8 @@
 from django.db import connections, models
 from django_hosts.resolvers import reverse
 
+from tracdb.models import Ticket
+
 METRIC_PERIOD_INSTANT = "instant"
 METRIC_PERIOD_DAILY = "daily"
 METRIC_PERIOD_WEEKLY = "weekly"
@@ -124,8 +125,8 @@ class TracTicketMetric(Metric):
     query = models.TextField()
 
     def fetch(self):
-        s = xmlrpc.client.ServerProxy(settings.TRAC_RPC_URL)
-        return len(s.ticket.query(self.query + "&max=0"))
+        queryset = Ticket.objects.from_querystring(self.query)
+        return queryset.count()
 
     def link(self):
         return f"{settings.TRAC_URL}query?{self.query}&desc=1&order=changetime"

dashboard/tests.py

@@ -8,6 +8,9 @@
 from django.test import RequestFactory, TestCase
 from django_hosts.resolvers import reverse
 
+from tracdb.models import Ticket
+from tracdb.testutils import TracDBCreateDatabaseMixin
+
 from .models import (
     METRIC_PERIOD_DAILY,
     METRIC_PERIOD_WEEKLY,
@@ -74,17 +77,21 @@ def test_get_absolute_url(self):
         self.assertTrue(url_path in self.instance.get_absolute_url())
 
 
-class TracTicketMetricTestCase(TestCase, MetricMixin):
-    fixtures = ["dashboard_test_data"]
+class TracTicketMetricTestCase(TracDBCreateDatabaseMixin, TestCase):
+    databases = {"default", "trac"}
 
-    def setUp(self):
-        super().setUp()
-        self.instance = TracTicketMetric.objects.last()
+    def test_fetch(self):
+        Ticket.objects.create(status="new", priority="high")
+        Ticket.objects.create(status="new", priority="low")
+        Ticket.objects.create(status="closed", priority="high")
+        Ticket.objects.create(status="closed", priority="low")
+        metric = TracTicketMetric.objects.create(
+            slug="test-tracticketmetric",
+            name="test tracticketmetric",
+            query="status=!closed&priority=high",
+        )
 
-    @mock.patch("xmlrpc.client.ServerProxy")
-    def test_fetch(self, mock_server_proxy):
-        self.instance.fetch()
-        self.assertTrue(mock_server_proxy.client.query.assert_called_with)
+        self.assertEqual(metric.fetch(), 1)
 
 
 class GithubItemCountMetricTestCase(TestCase, MetricMixin):

djangoproject/settings/common.py

@@ -299,7 +299,6 @@
 THUMBNAIL_ALTERNATIVE_RESOLUTIONS = [2]
 
 # dashboard settings
-TRAC_RPC_URL = "https://code.djangoproject.com/rpc"
 TRAC_URL = "https://code.djangoproject.com/"
 
 DEFAULT_AUTO_FIELD = "django.db.models.AutoField"

tracdb/migrations/0002_remove_db_views.py

@@ -0,0 +1,21 @@
+# Generated by Django 3.2.23 on 2024-02-12 16:43
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("tracdb", "0001_initial"),
+    ]
+
+    operations = [
+        migrations.AlterModelTable(
+            name="attachment",
+            table="attachment",
+        ),
+        migrations.AlterModelTable(
+            name="wiki",
+            table="wiki",
+        ),
+    ]

tracdb/models.py

@@ -4,47 +4,49 @@
 Initially generated by inspectdb then modified heavily by hand, often by
 consulting http://trac.edgewall.org/wiki/TracDev/DatabaseSchema.
 
-These are far from perfect: many (most?) Trac tables have composite primary
-keys, which Django can't represent. This means a lot of built-in Django stuff
-(the admin, for example) won't work at all with these models. I haven't
-investigated just how deeply down thess failures go, but I suspect all sorts
-of things just won't work.
+A few notes on tables that're left out and why:
 
-However, they're Good Enough(tm) to let me pull some basic (read-only) data out,
-and that's all I really need.
-
-Some potential TODOs:
-
-    * Add some convienance manager functions to deal with ticket_custom. Right
-      now you can query with a join::
-
-            Ticket.objects.filter(custom_fields__name='ui_ux',
-                                  custom_fields__value='1')
+    * 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.
 
-      Perhaps we might be able to get something like::
+These models are far from perfect but are Good Enough(tm) to get some useful data out.
 
-            Ticket.objects.with_custom(ui_ux=True)
+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).
 
-      Or even a custom .filter() that intercepts and figures it out?
+These are the tables that use them:
 
-    * Trac stores SVN repository revisions as '0000003744' grar. This
-      makes querying awkward. There's probably some tricky manager manger
-      that we could do here.
+    * ticket_custom (model TicketCustom)
+    * ticket_change (model TicketChange)
+    * wiki (model Wiki)
+    * attachment (model Attachment)
 
-    * The whole Revision model will fall apart if we ever had a second
-      repository to Trac.
+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:
 
-And a few notes on tables that're left out and why:
+    * Model.objects.create(...) correctly creates the object in the db
+    * Most queryset/manager methods work (in particular filter(), exclude(), all()
+      and count())
 
-    * All the session and permission tables: they're just not needd.
+On the other hand, here's what absolutely DOES NOT work (the list is sadly not
+exhaustive):
 
-    * Enum: I don't know what this is or what it's for.
-
-    * NodeChange: Ditto.
+    * 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)
 
 """
 
 import datetime
+from functools import reduce
+from operator import and_, or_
+from urllib.parse import parse_qs
 
 from django.db import models
 
@@ -69,24 +71,72 @@ def __get__(self, instance, owner):
         if instance is None:
             return self
         timestamp = getattr(instance, self.fieldname)
+        if timestamp is None:
+            return None
         return _epoc + datetime.timedelta(microseconds=timestamp)
 
 
+class JSONBObjectAgg(models.Aggregate):
+    function = "JSONB_OBJECT_AGG"  # PostgreSQL only.
+    output_field = models.JSONField()
+
+
+class TicketQuerySet(models.QuerySet):
+    def with_custom(self):
+        """
+        Annotate the "custom" properties as a json blob.
+        """
+        return self.annotate(
+            custom=JSONBObjectAgg("custom_fields__name", "custom_fields__value")
+        )
+
+    def from_querystring(self, querystring):
+        parsed = parse_qs(querystring)
+        model_fields = {f.name for f in self.model._meta.get_fields()}
+        custom_lookup_required = False
+        filter_kwargs, exclude_kwargs = {}, {}
+
+        for field, (value,) in parsed.items():
+            if field not in model_fields:
+                custom_lookup_required = True
+                field = f"custom__{field}"
+            if value.startswith("!"):
+                exclude_kwargs[field] = value[1:]
+            else:
+                filter_kwargs[field] = value
+
+        queryset = self
+        if custom_lookup_required:
+            queryset = queryset.with_custom()
+
+        if exclude_kwargs:
+            # negative values needed to be OR-ed for exclude
+            q = reduce(or_, [models.Q(**{k: v}) for k, v in exclude_kwargs.items()])
+            queryset = queryset.exclude(q)
+        if filter_kwargs:
+            # whereas positive values are AND-ed
+            q = reduce(and_, [models.Q(**{k: v}) for k, v in filter_kwargs.items()])
+            queryset = queryset.filter(q)
+
+        return queryset
+
+
 class Ticket(models.Model):
-    id = models.IntegerField(primary_key=True)
+    id = models.AutoField(primary_key=True)
     type = models.TextField()
 
-    _time = models.BigIntegerField(db_column="time")
+    _time = models.BigIntegerField(db_column="time", null=True)
     time = time_property("_time")
 
-    _changetime = models.BigIntegerField(db_column="changetime")
+    _changetime = models.BigIntegerField(db_column="changetime", null=True)
     changetime = time_property("_changetime")
 
     component = models.ForeignKey(
         "Component",
         related_name="tickets",
         db_column="component",
         on_delete=models.DO_NOTHING,
+        null=True,
     )
     severity = models.TextField()
     owner = models.TextField()
@@ -97,12 +147,14 @@ class Ticket(models.Model):
         related_name="tickets",
         db_column="version",
         on_delete=models.DO_NOTHING,
+        null=True,
     )
     milestone = models.ForeignKey(
         "Milestone",
         related_name="tickets",
         db_column="milestone",
         on_delete=models.DO_NOTHING,
+        null=True,
     )
     priority = models.TextField()
     status = models.TextField()
@@ -111,33 +163,22 @@ class Ticket(models.Model):
     description = models.TextField()
     keywords = models.TextField()
 
+    objects = TicketQuerySet.as_manager()
+
     class Meta:
         db_table = "ticket"
         managed = False
 
     def __str__(self):
         return f"#{self.id}: {self.summary}"
 
-    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
-
-        # Munge custom fields onto this object. This sucks since it implies
-        # querying will work (it won't!) and that writing will work (ditto).
-        # Also notice that *nasty* mapping of Trac's "booleanish" things to
-        # real booleans. This can fail in a bunch of ways, but not in our
-        # particular install.
-        for name, value in self.custom_fields.values_list("name", "value"):
-            if value in ("0", "1"):
-                value = bool(int(value))
-            setattr(self, name, value)
-
 
 class TicketCustom(models.Model):
     ticket = models.ForeignKey(
         Ticket,
         related_name="custom_fields",
         db_column="ticket",
-        primary_key=True,
+        primary_key=True,  # XXX See note at the top about composite pk
         on_delete=models.DO_NOTHING,
     )
     name = models.TextField()
@@ -156,7 +197,7 @@ class TicketChange(models.Model):
         Ticket,
         related_name="changes",
         db_column="ticket",
-        primary_key=True,
+        primary_key=True,  # XXX See note at the top about composite pk
         on_delete=models.DO_NOTHING,
     )
     author = models.TextField()
@@ -237,11 +278,14 @@ def get_queryset(self):
         return qs.filter(repos=self.repo_id)
 
 
+# Django's Trac uses a single repository with id 1 in the database
+# These models will not work if another repository is ever added
+# (but that seems unlikely at this point)
 SINGLE_REPO_ID = 1
 
 
 class Revision(models.Model):
-    repos = models.IntegerField()
+    repos = models.IntegerField(default=SINGLE_REPO_ID)
     rev = models.TextField(primary_key=True)
 
     _time = models.BigIntegerField(db_column="time")
@@ -260,14 +304,10 @@ def __str__(self):
         return "[{}] {}".format(self.rev, self.message.split("\n", 1)[0])
 
 
-# The Wiki table uses a composite primary key (name, version). Since
-# Django doesn't support this, this model sits on top of a simple view.
-# CREATE VIEW "wiki_django_view" AS
-#    SELECT "name" || '.' || "version" AS "django_id", *
-#    FROM wiki;
 class Wiki(models.Model):
-    django_id = models.TextField(primary_key=True)
-    name = models.TextField()
+    name = models.TextField(
+        primary_key=True
+    )  # XXX See note at the top about composite pk
     version = models.IntegerField()
     _time = models.BigIntegerField(db_column="time")
     time = time_property("time")
@@ -277,22 +317,18 @@ class Wiki(models.Model):
     readonly = models.IntegerField()
 
     class Meta:
-        db_table = "wiki_django_view"
+        db_table = "wiki"
         managed = False
 
     def __str__(self):
         return f"{self.name} (v{self.version})"
 
 
-# Same story as for Wiki: attachment's PK is (type, id, filename), so again
-# there's a simple view this is on top of.
-# CREATE VIEW "attachment_django_view" AS
-#    SELECT "type" || '.' || "id" || '.' || "filename" AS "django_id", *
-#    FROM attachment;
 class Attachment(models.Model):
-    django_id = models.TextField(primary_key=True)
     type = models.TextField()
-    id = models.TextField()
+    id = models.TextField(
+        primary_key=True
+    )  # XXX See note at the top about composite pk
     filename = models.TextField()
     size = models.IntegerField()
     _time = models.BigIntegerField(db_column="time")
@@ -301,7 +337,7 @@ class Attachment(models.Model):
     author = models.TextField()
 
     class Meta:
-        db_table = "attachment_django_view"
+        db_table = "attachment"
         managed = False
 
     def __str__(self):