From 317cf5fcf5b064af54dc3115a766ccaaf9891107 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Tue, 15 Nov 2022 14:12:00 -0800 Subject: [PATCH] Generate fewer add_status_tasks Previously, every time a status was saved, a task would start to add it to people's timelines. This meant there were a ton of duplicate tasks that were potentially heavy to run. Now, the Status model has a "ready" field which indicates that it's worth updating the timelines. It defaults to True, which prevents statuses from accidentally not being added due to ready state. The ready state is explicitly set to false in the view, which is the source of most of the noise for that task. --- bookwyrm/activitystreams.py | 6 ++ bookwyrm/migrations/0164_status_ready.py | 18 ++++++ bookwyrm/models/status.py | 8 ++- bookwyrm/tests/activitystreams/test_tasks.py | 1 + .../tests/views/inbox/test_inbox_create.py | 60 ++++++++++++++++++- bookwyrm/tests/views/test_status.py | 57 +++++++++++++++++- bookwyrm/views/status.py | 6 +- 7 files changed, 148 insertions(+), 8 deletions(-) create mode 100644 bookwyrm/migrations/0164_status_ready.py diff --git a/bookwyrm/activitystreams.py b/bookwyrm/activitystreams.py index f8312f063..1765f7e34 100644 --- a/bookwyrm/activitystreams.py +++ b/bookwyrm/activitystreams.py @@ -298,6 +298,12 @@ def add_status_on_create(sender, instance, created, *args, **kwargs): remove_status_task.delay(instance.id) return + # To avoid creating a zillion unnecessary tasks caused by re-saving the model, + # check if it's actually ready to send before we go. We're trusting this was + # set correctly by the inbox or view + if not instance.ready: + return + # when creating new things, gotta wait on the transaction transaction.on_commit( lambda: add_status_on_create_command(sender, instance, created) diff --git a/bookwyrm/migrations/0164_status_ready.py b/bookwyrm/migrations/0164_status_ready.py new file mode 100644 index 000000000..fd8d49972 --- /dev/null +++ b/bookwyrm/migrations/0164_status_ready.py @@ -0,0 +1,18 @@ +# Generated by Django 3.2.16 on 2022-11-15 21:40 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("bookwyrm", "0163_merge_0160_auto_20221101_2251_0162_importjob_task_id"), + ] + + operations = [ + migrations.AddField( + model_name="status", + name="ready", + field=models.BooleanField(default=True), + ), + ] diff --git a/bookwyrm/models/status.py b/bookwyrm/models/status.py index 1546a4b5b..19eab584d 100644 --- a/bookwyrm/models/status.py +++ b/bookwyrm/models/status.py @@ -63,6 +63,9 @@ class Status(OrderedCollectionPageMixin, BookWyrmModel): activitypub_field="inReplyTo", ) thread_id = models.IntegerField(blank=True, null=True) + # statuses get saved a few times, this indicates if they're set + ready = models.BooleanField(default=True) + objects = InheritanceManager() activity_serializer = activitypub.Note @@ -83,8 +86,7 @@ class Status(OrderedCollectionPageMixin, BookWyrmModel): if not self.reply_parent: self.thread_id = self.id - - super().save(broadcast=False, update_fields=["thread_id"]) + super().save(broadcast=False, update_fields=["thread_id"]) def delete(self, *args, **kwargs): # pylint: disable=unused-argument """ "delete" a status""" @@ -399,7 +401,7 @@ class ReviewRating(Review): def save(self, *args, **kwargs): if not self.rating: raise ValueError("ReviewRating object must include a numerical rating") - return super().save(*args, **kwargs) + super().save(*args, **kwargs) @property def pure_content(self): diff --git a/bookwyrm/tests/activitystreams/test_tasks.py b/bookwyrm/tests/activitystreams/test_tasks.py index af05cf18f..2e89f6ccc 100644 --- a/bookwyrm/tests/activitystreams/test_tasks.py +++ b/bookwyrm/tests/activitystreams/test_tasks.py @@ -7,6 +7,7 @@ from bookwyrm import activitystreams, models class Activitystreams(TestCase): """using redis to build activity streams""" + # pylint: disable=invalid-name def setUp(self): """use a test csv""" with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch( diff --git a/bookwyrm/tests/views/inbox/test_inbox_create.py b/bookwyrm/tests/views/inbox/test_inbox_create.py index cc1b8d508..e61b9313a 100644 --- a/bookwyrm/tests/views/inbox/test_inbox_create.py +++ b/bookwyrm/tests/views/inbox/test_inbox_create.py @@ -3,13 +3,69 @@ import json import pathlib from unittest.mock import patch -from django.test import TestCase +from django.test import TestCase, TransactionTestCase from bookwyrm import models, views from bookwyrm.activitypub import ActivitySerializerError -# pylint: disable=too-many-public-methods +# pylint: disable=too-many-public-methods, invalid-name +class TransactionInboxCreate(TransactionTestCase): + """readthrough tests""" + + def setUp(self): + """basic user and book data""" + with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch( + "bookwyrm.activitystreams.populate_stream_task.delay" + ), patch("bookwyrm.lists_stream.populate_lists_task.delay"): + self.local_user = models.User.objects.create_user( + "mouse@example.com", + "mouse@mouse.com", + "mouseword", + local=True, + localname="mouse", + ) + self.local_user.remote_id = "https://example.com/user/mouse" + self.local_user.save(broadcast=False, update_fields=["remote_id"]) + with patch("bookwyrm.models.user.set_remote_server.delay"): + self.remote_user = models.User.objects.create_user( + "rat", + "rat@rat.com", + "ratword", + local=False, + remote_id="https://example.com/users/rat", + inbox="https://example.com/users/rat/inbox", + outbox="https://example.com/users/rat/outbox", + ) + + self.create_json = { + "id": "hi", + "type": "Create", + "actor": "hi", + "to": ["https://www.w3.org/ns/activitystreams#public"], + "cc": ["https://example.com/user/mouse/followers"], + "object": {}, + } + models.SiteSettings.objects.create() + + def test_create_status_transaction(self, *_): + """the "it justs works" mode""" + datafile = pathlib.Path(__file__).parent.joinpath( + "../../data/ap_quotation.json" + ) + status_data = json.loads(datafile.read_bytes()) + + models.Edition.objects.create( + title="Test Book", remote_id="https://example.com/book/1" + ) + activity = self.create_json + activity["object"] = status_data + + with patch("bookwyrm.activitystreams.add_status_task.apply_async") as mock: + views.inbox.activity_task(activity) + self.assertEqual(mock.call_count, 2) + + @patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async") @patch("bookwyrm.activitystreams.add_book_statuses_task.delay") class InboxCreate(TestCase): diff --git a/bookwyrm/tests/views/test_status.py b/bookwyrm/tests/views/test_status.py index 022c246c4..203ec57dd 100644 --- a/bookwyrm/tests/views/test_status.py +++ b/bookwyrm/tests/views/test_status.py @@ -2,21 +2,74 @@ import json from unittest.mock import patch from django.core.exceptions import PermissionDenied -from django.test import TestCase +from django.test import TestCase, TransactionTestCase from django.test.client import RequestFactory from bookwyrm import forms, models, views from bookwyrm.views.status import find_mentions from bookwyrm.settings import DOMAIN + from bookwyrm.tests.validate_html import validate_html +# pylint: disable=invalid-name +@patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async") +class StatusTransactions(TransactionTestCase): + """Test full database transactions""" + + def setUp(self): + """we need basic test data and mocks""" + self.factory = RequestFactory() + with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch( + "bookwyrm.activitystreams.populate_stream_task.delay" + ), patch("bookwyrm.lists_stream.populate_lists_task.delay"): + self.local_user = models.User.objects.create_user( + "mouse@local.com", + "mouse@mouse.com", + "mouseword", + local=True, + localname="mouse", + remote_id="https://example.com/users/mouse", + ) + self.another_user = models.User.objects.create_user( + f"nutria@{DOMAIN}", + "nutria@nutria.com", + "password", + local=True, + localname="nutria", + ) + + work = models.Work.objects.create(title="Test Work") + self.book = models.Edition.objects.create( + title="Example Edition", + remote_id="https://example.com/book/1", + parent_work=work, + ) + + def test_create_status_saves(self, *_): + """This view calls save multiple times""" + view = views.CreateStatus.as_view() + form = forms.CommentForm( + { + "content": "hi", + "user": self.local_user.id, + "book": self.book.id, + "privacy": "public", + } + ) + request = self.factory.post("", form.data) + request.user = self.local_user + + with patch("bookwyrm.activitystreams.add_status_task.apply_async") as mock: + view(request, "comment") + + self.assertEqual(mock.call_count, 2) + @patch("bookwyrm.suggested_users.rerank_suggestions_task.delay") @patch("bookwyrm.activitystreams.populate_stream_task.delay") @patch("bookwyrm.lists_stream.populate_lists_task.delay") @patch("bookwyrm.activitystreams.remove_status_task.delay") @patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async") -# pylint: disable=invalid-name # pylint: disable=too-many-public-methods class StatusViews(TestCase): """viewing and creating statuses""" diff --git a/bookwyrm/views/status.py b/bookwyrm/views/status.py index dc6bbc579..42f903c05 100644 --- a/bookwyrm/views/status.py +++ b/bookwyrm/views/status.py @@ -82,13 +82,16 @@ class CreateStatus(View): return HttpResponseBadRequest() return redirect("/") - status = form.save(request) + status = form.save(request, commit=False) + status.ready = False # save the plain, unformatted version of the status for future editing status.raw_content = status.content if hasattr(status, "quote"): status.raw_quote = status.quote status.sensitive = status.content_warning not in [None, ""] + # the status has to be saved now before we can add many to many fields + # like mentions status.save(broadcast=False) # inspect the text for user tags @@ -119,6 +122,7 @@ class CreateStatus(View): if hasattr(status, "quote"): status.quote = to_markdown(status.quote) + status.ready = True status.save(created=created) # update a readthrough, if needed