From patchwork Wed Jan 10 00:40:23 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stephen Finucane X-Patchwork-Id: 857860 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3zGVZd5QR6z9sBW for ; Wed, 10 Jan 2018 11:42:01 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="key not found in DNS" (0-bit key; unprotected) header.d=that.guru header.i=@that.guru header.b="GFogIyP8"; dkim-atps=neutral Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 3zGVZc6gZHzF0cZ for ; Wed, 10 Jan 2018 11:42:00 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dkim=fail reason="key not found in DNS" (0-bit key; unprotected) header.d=that.guru header.i=@that.guru header.b="GFogIyP8"; dkim-atps=neutral X-Original-To: patchwork@lists.ozlabs.org Delivered-To: patchwork@lists.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=that.guru (client-ip=23.83.222.30; helo=caracal.ash.relay.mailchannels.net; envelope-from=stephen@that.guru; receiver=) Authentication-Results: lists.ozlabs.org; dkim=fail reason="key not found in DNS" (0-bit key; unprotected) header.d=that.guru header.i=@that.guru header.b="GFogIyP8"; dkim-atps=neutral Received: from caracal.ash.relay.mailchannels.net (caracal.ash.relay.mailchannels.net [23.83.222.30]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3zGVY33nxXzF0RJ for ; Wed, 10 Jan 2018 11:40:38 +1100 (AEDT) X-Sender-Id: mxroute|x-authuser|stephen@that.guru Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id 3ACCC5C0917 for ; Wed, 10 Jan 2018 00:40:33 +0000 (UTC) Received: from one.mxroute.com (unknown [100.96.29.6]) (Authenticated sender: mxroute) by relay.mailchannels.net (Postfix) with ESMTPA id 950935C07DF for ; Wed, 10 Jan 2018 00:40:32 +0000 (UTC) X-Sender-Id: mxroute|x-authuser|stephen@that.guru Received: from one.mxroute.com (one-outgoing.mxroute.com [172.18.44.71]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384) by 0.0.0.0:2500 (trex/5.11.3); Wed, 10 Jan 2018 00:40:33 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: mxroute|x-authuser|stephen@that.guru X-MailChannels-Auth-Id: mxroute X-Plucky-Gusty: 27fa57275f0249a5_1515544833041_3184859791 X-MC-Loop-Signature: 1515544833041:2837410224 X-MC-Ingress-Time: 1515544833040 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=that.guru; s=default; h=References:In-Reply-To:Message-Id:Date:Subject:Cc:To:From: Sender:Reply-To:MIME-Version:Content-Type:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=5haq5/soocsuA4QkJnPGNhwvdqTgA0tmO+WHt4vtiro=; b=GFogIyP8ZS0sS8FSxUkig/at4n C5XFVKxA4oQw+pQnqH4IfN/46R1XLk2jHl2i0Ihko57wKUDvKAvlfFHDcnbeuivCzVkyfP2c7/xi1 e1CRgjZ0aWRxMD2vN9lZd80T+Y9R9zNvSIsYRbwvoUN46DudYWtu+urr9Q+0BuOJxWQQenUeAsw9i AVsBQh+fqFGEgCgO2I0+xK6kO57vmz2xt/ypFVAoCId3D7Czxf3GhnA0WqBMrt/XXxp82eVi3FnTr RCz8CTtc3kyk+acuH2yw/mBp30x1TyIkMJDXEtVAm/b13iqtofkoZ+J1ZlWWQVY8y+SQ8eEahF8kB wNtyiRwA==; From: Stephen Finucane To: patchwork@lists.ozlabs.org Subject: [RFC 3/4] models: Migrate event fields to JSON field Date: Wed, 10 Jan 2018 00:40:23 +0000 Message-Id: <20180110004024.5551-4-stephen@that.guru> X-Mailer: git-send-email 2.14.3 In-Reply-To: <20180110004024.5551-1-stephen@that.guru> References: <20180110004024.5551-1-stephen@that.guru> X-AuthUser: stephen@that.guru X-BeenThere: patchwork@lists.ozlabs.org X-Mailman-Version: 2.1.24 Precedence: list List-Id: Patchwork development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Patchwork" This allows us to remove swathes of ForeignKeys and greatly speed up the '/events' endpoint. This comes at the disadvantage of preventing us indexing the embedded fields and would result in different responses if the serializers ever change; however, we don't do the former at the moment and it's unlikely that the latter will happen regularly (and can be addressed by a migration if absolutely necessary). Note that to generated the JSON, we use the same serializers we normally use for embedded responses. This has the advantage of minimizing the amount of new code needed and handles a lot of the ugliness of serialization. However, this does necessitate using Django REST Framework for events. This has been addressed in a previous patch. Signed-off-by: Stephen Finucane Closes-bug: #153 ("'/events' is slow") --- patchwork/api/event.py | 110 +++++++++++++-------- patchwork/fields.py | 32 ++++++ .../migrations/0021_add_event_payload_field.py | 21 ++++ ...22_migrate_data_from_event_fields_to_payload.py | 67 +++++++++++++ .../migrations/0023_remove_old_event_fields.py | 43 ++++++++ patchwork/models.py | 28 +----- patchwork/signals.py | 48 +++++++++ patchwork/tests/test_events.py | 110 +++++++++++++-------- 8 files changed, 354 insertions(+), 105 deletions(-) create mode 100644 patchwork/migrations/0021_add_event_payload_field.py create mode 100644 patchwork/migrations/0022_migrate_data_from_event_fields_to_payload.py create mode 100644 patchwork/migrations/0023_remove_old_event_fields.py diff --git a/patchwork/api/event.py b/patchwork/api/event.py index 0d97af22..8f25eb4f 100644 --- a/patchwork/api/event.py +++ b/patchwork/api/event.py @@ -19,9 +19,12 @@ from collections import OrderedDict +from rest_framework.fields import JSONField from rest_framework.generics import ListAPIView +from rest_framework.reverse import reverse from rest_framework.serializers import ModelSerializer -from rest_framework.serializers import SerializerMethodField +from rest_framework.serializers import ReadOnlyField +from rest_framework.serializers import Serializer from patchwork.api.embedded import CheckSerializer from patchwork.api.embedded import CoverLetterSerializer @@ -30,48 +33,79 @@ from patchwork.api.embedded import ProjectSerializer from patchwork.api.embedded import SeriesSerializer from patchwork.api.embedded import UserSerializer from patchwork.api.filters import EventFilter -from patchwork.api.patch import StateField from patchwork.models import Event -class EventSerializer(ModelSerializer): +class StateField(ReadOnlyField): - project = ProjectSerializer(read_only=True) - patch = PatchSerializer(read_only=True) - series = SeriesSerializer(read_only=True) - cover = CoverLetterSerializer(read_only=True) + def to_representation(self, obj): + return '-'.join(obj.name.lower().split()) + + +class PayloadSerializer(Serializer): + + cover = CoverLetterSerializer(required=False) + patch = PatchSerializer(required=False) + series = SeriesSerializer(required=False) previous_state = StateField() current_state = StateField() - previous_delegate = UserSerializer() - current_delegate = UserSerializer() - created_check = SerializerMethodField() - created_check = CheckSerializer() - - _category_map = { - Event.CATEGORY_COVER_CREATED: ['cover'], - Event.CATEGORY_PATCH_CREATED: ['patch'], - Event.CATEGORY_PATCH_COMPLETED: ['patch', 'series'], - Event.CATEGORY_PATCH_STATE_CHANGED: ['patch', 'previous_state', - 'current_state'], - Event.CATEGORY_PATCH_DELEGATED: ['patch', 'previous_delegate', - 'current_delegate'], - Event.CATEGORY_CHECK_CREATED: ['patch', 'created_check'], - Event.CATEGORY_SERIES_CREATED: ['series'], - Event.CATEGORY_SERIES_COMPLETED: ['series'], - } + previous_delegate = UserSerializer(required=False) + current_delegate = UserSerializer(required=False) + check = CheckSerializer(required=False) + + +class EventSerializer(ModelSerializer): + + project = ProjectSerializer(read_only=True) + payload = JSONField(read_only=True) def to_representation(self, instance): data = super(EventSerializer, self).to_representation(instance) - payload = OrderedDict() - kept_fields = self._category_map[instance.category] + [ - 'id', 'category', 'project', 'date'] - for field in [x for x in data]: - if field not in kept_fields: - del data[field] - elif field in self._category_map[instance.category]: - field_name = 'check' if field == 'created_check' else field - payload[field_name] = data.pop(field) + # TODO(stephenfin): Remove this once we have migrations in place + if not data['payload']: + return data + + request = self.context['request'] + payload = data['payload'] + + # set the 'url' and 'mbox' fields for any embedded 'cover', + # 'patch', and 'series' fields + for field in ('cover', 'patch', 'series'): + if not payload.get(field): + continue + + for subfield, lookup, url_name in ( + ('url', 'pk', 'api-{}-detail'), + ('mbox', '{}_id', '{}-mbox')): + payload[field][subfield] = reverse( + url_name.format(field), + request=request, + kwargs={ + lookup.format(field): payload[field]['id'], + }) + + # set the 'url' field for any embedded '*_delegate' fields + for field in ('previous_delegate', 'current_delegate'): + if not payload.get(field): + continue + + payload[field]['url'] = reverse( + 'api-user-detail', + request=request, + kwargs={ + 'pk': payload[field]['id'] + }) + + # set the 'url' field for any embedded 'check' fields + if payload.get('check'): + payload['check']['url'] = reverse( + 'api-check-detail', + request=request, + kwargs={ + 'patch_id': payload['patch']['id'], + 'check_id': payload['check']['id'] + }) data['payload'] = payload @@ -79,9 +113,7 @@ class EventSerializer(ModelSerializer): class Meta: model = Event - fields = ('id', 'category', 'project', 'date', 'patch', 'series', - 'cover', 'previous_state', 'current_state', - 'previous_delegate', 'current_delegate', 'created_check') + fields = ('id', 'category', 'project', 'date', 'payload') read_only_fields = fields @@ -95,8 +127,4 @@ class EventList(ListAPIView): ordering = '-date' def get_queryset(self): - return Event.objects.all()\ - .select_related('project', 'patch', 'series', 'cover', - 'previous_state', 'current_state', - 'previous_delegate', 'current_delegate', - 'created_check') + return Event.objects.all().select_related('project') diff --git a/patchwork/fields.py b/patchwork/fields.py index 502558be..8062e70a 100644 --- a/patchwork/fields.py +++ b/patchwork/fields.py @@ -20,7 +20,9 @@ from __future__ import absolute_import +from collections import OrderedDict import hashlib +import json from django.db import models from django.utils import six @@ -44,3 +46,33 @@ class HashField(models.CharField): def db_type(self, connection=None): return 'char(%d)' % self.n_bytes + + +class JSONField(models.TextField): + """A basic field for loading/storing JSON. + + We use this rather than Django's own JSONField because we don't need to + index this field and said field does not support MySQL yet. + """ + + def get_prep_value(self, value): + # This is necessary for Django 1.8.x, which called 'smart_text' from + # 'django.utils.encoding' on all data + return value + + def get_db_prep_value(self, value, connection, prepared=False): + value = super(JSONField, self).get_db_prep_value( + value, connection, prepared) + return json.dumps(value) + + def from_db_value(self, value, *args, **kwargs): + if value is None: + return value + + return json.loads(value, object_pairs_hook=OrderedDict) + + def to_python(self, value): + if value is None or isinstance(value, OrderedDict): + return value + + return json.loads(value, object_pairs_hook=OrderedDict) diff --git a/patchwork/migrations/0021_add_event_payload_field.py b/patchwork/migrations/0021_add_event_payload_field.py new file mode 100644 index 00000000..e362ed03 --- /dev/null +++ b/patchwork/migrations/0021_add_event_payload_field.py @@ -0,0 +1,21 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models + +import patchwork.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('patchwork', '0020_tag_show_column'), + ] + + operations = [ + migrations.AddField( + model_name='event', + name='payload', + field=patchwork.fields.JSONField(blank=True, null=True), + ), + ] diff --git a/patchwork/migrations/0022_migrate_data_from_event_fields_to_payload.py b/patchwork/migrations/0022_migrate_data_from_event_fields_to_payload.py new file mode 100644 index 00000000..760e26f2 --- /dev/null +++ b/patchwork/migrations/0022_migrate_data_from_event_fields_to_payload.py @@ -0,0 +1,67 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +import json + +from django.db import migrations, models + +from patchwork.api.event import PayloadSerializer +from patchwork.models import Event +from patchwork.notifications import expire_events + + +# TODO(stephenfin): Move this to 'api/event' and into Serializer +class Payload(object): + + def __init__(self, **kwargs): + for kwarg in kwargs: + setattr(self, kwarg, kwargs[kwarg]) + + +def generate_payload(apps, schema_editor): + # NOTE(stephenfin): We could convert this to native SQL in the future + # by using e.g. 'JSON_OBJECT' for MySQL + EventBase = apps.get_model('patchwork', 'Event') + + expire_events() + + for event in EventBase.objects.all(): + category = event.category + # TODO(stephenfin): This logic should be moved into the EventSerializer + # class + if category == Event.CATEGORY_COVER_CREATED: + payload_obj = Payload(cover=event.cover) + elif category == Event.CATEGORY_PATCH_CREATED: + payload_obj = Payload(patch=event.patch) + elif category == Event.CATEGORY_PATCH_STATE_CHANGED: + payload_obj = Payload(patch=event.patch, + previous_state=event.previous_state, + current_state=event.previous_state) + elif category == Event.CATEGORY_PATCH_DELEGATED: + payload_obj = Payload(patch=event.patch, + previous_delegate=event.previous_delegate, + current_delegate=event.current_delegate) + elif category == Event.CATEGORY_PATCH_COMPLETED: + payload_obj = Payload(patch=event.patch) + elif category == Event.CATEGORY_CHECK_CREATED: + payload_obj = Payload(patch=event.patch, check=event.created_check) + elif category == Event.CATEGORY_SERIES_CREATED: + payload_obj = Payload(series=event.series) + elif category == Event.CATEGORY_SERIES_COMPLETED: + payload_obj = Payload(series=event.series) + + payload = json.dumps(PayloadSerializer(payload_obj).data) + event.payload = payload + event.save() + + +class Migration(migrations.Migration): + + dependencies = [ + ('patchwork', '0021_add_event_payload_field'), + ] + + operations = [ + migrations.RunPython(generate_payload, + atomic=True), + ] diff --git a/patchwork/migrations/0023_remove_old_event_fields.py b/patchwork/migrations/0023_remove_old_event_fields.py new file mode 100644 index 00000000..1a3c70e1 --- /dev/null +++ b/patchwork/migrations/0023_remove_old_event_fields.py @@ -0,0 +1,43 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.8 on 2018-01-10 00:16 +from __future__ import unicode_literals + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('patchwork', '0022_migrate_data_from_event_fields_to_payload'), + ] + + operations = [ + migrations.AlterModelOptions( + name='patch', + options={'base_manager_name': 'objects', 'verbose_name_plural': 'Patches'}, + ), + migrations.AlterModelOptions( + name='series', + options={'ordering': ('date',), 'verbose_name_plural': 'Series'}, + ), + migrations.RemoveField( + model_name='event', + name='created_check', + ), + migrations.RemoveField( + model_name='event', + name='current_delegate', + ), + migrations.RemoveField( + model_name='event', + name='current_state', + ), + migrations.RemoveField( + model_name='event', + name='previous_delegate', + ), + migrations.RemoveField( + model_name='event', + name='previous_state', + ), + ] diff --git a/patchwork/models.py b/patchwork/models.py index 11886f1a..00674ac8 100644 --- a/patchwork/models.py +++ b/patchwork/models.py @@ -36,6 +36,7 @@ from django.utils.functional import cached_property from patchwork.compat import is_authenticated from patchwork.compat import reverse from patchwork.fields import HashField +from patchwork.fields import JSONField from patchwork.hasher import hash_diff if settings.ENABLE_REST_API: @@ -913,32 +914,9 @@ class Event(models.Model): on_delete=models.CASCADE, help_text='The cover letter that this event was created for.') - # fields for 'patch-state-changed' events + # additional data - previous_state = models.ForeignKey( - State, related_name='+', null=True, blank=True, - on_delete=models.CASCADE) - current_state = models.ForeignKey( - State, related_name='+', null=True, blank=True, - on_delete=models.CASCADE) - - # fields for 'patch-delegate-changed' events - - previous_delegate = models.ForeignKey( - User, related_name='+', null=True, blank=True, - on_delete=models.CASCADE) - current_delegate = models.ForeignKey( - User, related_name='+', null=True, blank=True, - on_delete=models.CASCADE) - - # fields or 'patch-check-created' events - - created_check = models.ForeignKey( - Check, related_name='+', null=True, blank=True, - on_delete=models.CASCADE) - - # TODO(stephenfin): Validate that the correct fields are being set by way - # of a 'clean' method + payload = JSONField(null=True, blank=True) def __repr__(self): return " None @@ -164,24 +178,36 @@ class PatchChangedTest(_BaseTestCase): patch.save() events = _get_events(patch=patch) + self.assertEqual(events.count(), 4) self.assertEqual(events[3].category, Event.CATEGORY_PATCH_DELEGATED) - self.assertEventFields(events[3], previous_delegate=delegate_b) + payload = events[3].payload + self.assertIn('previous_delegate', payload) + self.assertEqual(payload['previous_delegate']['id'], delegate_b.id) + self.assertIn('current_delegate', payload) + self.assertIsNone(payload['current_delegate']) -class CheckCreateTest(_BaseTestCase): + +class CheckCreateTest(TestCase): def test_check_created(self): check = utils.create_check() - events = _get_events(created_check=check) - self.assertEqual(events.count(), 1) - self.assertEqual(events[0].category, Event.CATEGORY_CHECK_CREATED) - self.assertEqual(events[0].project, check.patch.project) - self.assertEventFields(events[0]) + # we don't care about the CATEGORY_PATCH_CREATED event here + events = _get_events(patch=check.patch) + self.assertEqual(events.count(), 2) + # we don't care about the CATEGORY_PATCH_CREATED event here + self.assertEqual(events[1].category, Event.CATEGORY_CHECK_CREATED) + self.assertEqual(events[1].project, check.patch.project) + + payload = events[1].payload + self.assertIn('check', payload) + self.assertEqual(payload['check']['id'], check.id) -class CoverCreateTest(_BaseTestCase): + +class CoverCreateTest(TestCase): def test_cover_created(self): cover = utils.create_cover() @@ -189,10 +215,13 @@ class CoverCreateTest(_BaseTestCase): self.assertEqual(events.count(), 1) self.assertEqual(events[0].category, Event.CATEGORY_COVER_CREATED) self.assertEqual(events[0].project, cover.project) - self.assertEventFields(events[0]) + + payload = events[0].payload + self.assertIn('cover', payload) + self.assertEqual(payload['cover']['id'], cover.id) -class SeriesCreateTest(_BaseTestCase): +class SeriesCreateTest(TestCase): def test_series_created(self): series = utils.create_series() @@ -200,4 +229,7 @@ class SeriesCreateTest(_BaseTestCase): self.assertEqual(events.count(), 1) self.assertEqual(events[0].category, Event.CATEGORY_SERIES_CREATED) self.assertEqual(events[0].project, series.project) - self.assertEventFields(events[0]) + + payload = events[0].payload + self.assertIn('series', payload) + self.assertEqual(payload['series']['id'], series.id)