diff --git a/dashboard/models.py b/dashboard/models.py index cf2bb0eff..33e60770e 100644 --- a/dashboard/models.py +++ b/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" diff --git a/dashboard/tests.py b/dashboard/tests.py index 6619c6ae8..1d357ffc3 100644 --- a/dashboard/tests.py +++ b/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): diff --git a/djangoproject/settings/common.py b/djangoproject/settings/common.py index 52f2b0605..bd5cd1887 100644 --- a/djangoproject/settings/common.py +++ b/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" diff --git a/tracdb/migrations/0002_remove_db_views.py b/tracdb/migrations/0002_remove_db_views.py new file mode 100644 index 000000000..563426fea --- /dev/null +++ b/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", + ), + ] diff --git a/tracdb/models.py b/tracdb/models.py index 50c474000..380db3955 100644 --- a/tracdb/models.py +++ b/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,17 +71,64 @@ 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( @@ -87,6 +136,7 @@ class Ticket(models.Model): 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,6 +163,8 @@ class Ticket(models.Model): description = models.TextField() keywords = models.TextField() + objects = TicketQuerySet.as_manager() + class Meta: db_table = "ticket" managed = False @@ -118,26 +172,13 @@ class Meta: 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): diff --git a/tracdb/tests.py b/tracdb/tests.py index e5613efac..e2b8f4d52 100644 --- a/tracdb/tests.py +++ b/tracdb/tests.py @@ -1,8 +1,159 @@ +from operator import attrgetter + from django.test import TestCase -from .models import Revision +from .models import Revision, Ticket, TicketCustom +from .testutils import TracDBCreateDatabaseMixin class TestModels(TestCase): def test_router(self): self.assertEqual(Revision.objects.db, "trac") + + +class TicketTestCase(TracDBCreateDatabaseMixin, TestCase): + databases = {"trac"} + + def _create_ticket(self, custom=None, **kwargs): + """ + A factory to create a Ticket, with optional TicketCustom instances attached. + """ + if custom is None: + custom = {} + + ticket = Ticket.objects.create(**kwargs) + TicketCustom.objects.bulk_create( + [ + TicketCustom(ticket=ticket, name=name, value=value) + for name, value in custom.items() + ] + ) + return ticket + + def assertTicketsEqual( + self, queryset, expected, transform=attrgetter("summary"), ordered=False + ): + """ + A wrapper around assertQuerysetEqual with some useful defaults + """ + self.assertQuerysetEqual( + queryset, expected, transform=transform, ordered=ordered + ) + + def test_ticket_table_exist_in_testdb(self): + self._create_ticket(summary="test", custom={"x": "y"}) + self.assertTicketsEqual(Ticket.objects.all(), ["test"]) + self.assertQuerysetEqual( + TicketCustom.objects.all(), + [("x", "y")], + transform=attrgetter("name", "value"), + ordered=False, + ) + + def test_with_custom(self): + self._create_ticket(summary="test1", custom={"x": "A", "y": "B"}) + self._create_ticket(summary="test2", custom={"z": "C"}) + + self.assertQuerysetEqual( + Ticket.objects.with_custom().order_by("summary"), + [ + ("test1", {"x": "A", "y": "B"}), + ("test2", {"z": "C"}), + ], + transform=attrgetter("summary", "custom"), + ) + + def test_with_custom_lookup(self): + self._create_ticket(summary="test", custom={"x": "A", "y": "B"}) + + self.assertTicketsEqual( + Ticket.objects.with_custom().filter(custom__x="A"), + [("test")], + ) + + def test_with_custom_lookup_multiple(self): + self._create_ticket(summary="test1", custom={"x": "A", "y": "A"}) + self._create_ticket(summary="test2", custom={"x": "A", "y": "B"}) + self._create_ticket(summary="test3", custom={"x": "B", "y": "A"}) + + self.assertTicketsEqual( + Ticket.objects.with_custom().filter(custom__x="A", custom__y="A"), + [("test1")], + ) + + def test_from_querystring_model_field(self): + self._create_ticket(summary="test1", severity="high") + self._create_ticket(summary="test2", severity="low") + + self.assertTicketsEqual( + Ticket.objects.from_querystring("severity=high"), + ["test1"], + ) + + def test_from_querystring_model_field_multiple(self): + self._create_ticket(summary="test1", severity="high", resolution="new") + self._create_ticket(summary="test2", severity="high", resolution="fixed") + self._create_ticket(summary="test3", severity="low", resolution="new") + self._create_ticket(summary="test4", severity="low", resolution="fixed") + + self.assertTicketsEqual( + Ticket.objects.from_querystring("severity=high&resolution=new"), + ["test1"], + ) + + def test_from_querystring_model_field_negative(self): + self._create_ticket(summary="test1", severity="high") + self._create_ticket(summary="test2", severity="low") + + self.assertTicketsEqual( + Ticket.objects.from_querystring("severity=!high"), + ["test2"], + ) + + def test_from_querystring_model_field_negative_multiple(self): + self._create_ticket(summary="test1", severity="high", resolution="new") + self._create_ticket(summary="test2", severity="high", resolution="fixed") + self._create_ticket(summary="test3", severity="low", resolution="new") + self._create_ticket(summary="test4", severity="low", resolution="fixed") + + self.assertTicketsEqual( + Ticket.objects.from_querystring("severity=!low&resolution=!fixed"), + ["test1"], + ) + + def test_from_querystring_custom_field(self): + self._create_ticket(summary="test1", custom={"stage": "unreviewed"}) + self._create_ticket(summary="test2", custom={"stage": "reviewed"}) + + self.assertTicketsEqual( + Ticket.objects.from_querystring("stage=reviewed"), + ["test2"], + ) + + def test_from_querystring_custom_field_negative(self): + self._create_ticket(summary="test1", custom={"stage": "unreviewed"}) + self._create_ticket(summary="test2", custom={"stage": "reviewed"}) + + self.assertTicketsEqual( + Ticket.objects.from_querystring("stage=!reviewed"), + ["test1"], + ) + + def test_from_querystring_model_and_custom_field_together(self): + self._create_ticket( + summary="test1", severity="high", custom={"stage": "unreviewed"} + ) + self._create_ticket( + summary="test2", severity="high", custom={"stage": "reviewed"} + ) + self._create_ticket( + summary="test3", severity="low", custom={"stage": "unreviewed"} + ) + self._create_ticket( + summary="test4", severity="low", custom={"stage": "reviewed"} + ) + + self.assertTicketsEqual( + Ticket.objects.from_querystring("severity=high&stage=unreviewed"), + ["test1"], + ) diff --git a/tracdb/testutils.py b/tracdb/testutils.py new file mode 100644 index 000000000..1a9698932 --- /dev/null +++ b/tracdb/testutils.py @@ -0,0 +1,80 @@ +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) + 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) + + +def create_db_tables_for_unmanaged_models(schema_editor): + """ + Create tables for all unmanaged models in the tracdb app. + """ + appconfig = apps.get_app_config("tracdb") + for model in appconfig.get_models(): + if model._meta.managed: + continue + _create_db_table_for_model(model, schema_editor) + + +class TracDBCreateDatabaseMixin: + """ + A TestCase mixin that creates test tables for all the tracdb apps. + Make sure you have databases = {"trac"} defined on your TestCase subclass. + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + with connections["trac"].schema_editor() as schema_editor: + create_db_tables_for_unmanaged_models(schema_editor)