From fbb9d75cc804dd579222d3c30c2c160d21a57d76 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 17 Jul 2023 06:55:49 -0700 Subject: [PATCH 1/6] Avoid server error when encountering broken edition If an edition is missing its work, this change allows the page to still load without a server error; this is important because otherwise the book will break every page it appears on, including the feed page. --- bookwyrm/templatetags/rating_tags.py | 4 ++++ bookwyrm/tests/templatetags/test_rating_tags.py | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/bookwyrm/templatetags/rating_tags.py b/bookwyrm/templatetags/rating_tags.py index cc23d3308a..367463a8f0 100644 --- a/bookwyrm/templatetags/rating_tags.py +++ b/bookwyrm/templatetags/rating_tags.py @@ -12,6 +12,10 @@ @register.filter(name="rating") def get_rating(book, user): """get the overall rating of a book""" + # this shouldn't happen, but it CAN + if not book.parent_work: + return None + return cache.get_or_set( f"book-rating-{book.parent_work.id}", lambda u, b: models.Review.objects.filter( diff --git a/bookwyrm/tests/templatetags/test_rating_tags.py b/bookwyrm/tests/templatetags/test_rating_tags.py index 5abfa471aa..8c07eeb8f3 100644 --- a/bookwyrm/tests/templatetags/test_rating_tags.py +++ b/bookwyrm/tests/templatetags/test_rating_tags.py @@ -71,6 +71,12 @@ def test_get_rating(self, *_): ) self.assertEqual(rating_tags.get_rating(self.book, self.local_user), 5) + def test_get_rating_broken_edition(self, *_): + """Don't have a server error if an edition is missing a work""" + broken_book = models.Edition.objects.create(title="Test") + broken_book.parent_work = None + self.assertIsNone(rating_tags.get_rating(broken_book, self.local_user)) + def test_get_user_rating(self, *_): """get a user's most recent rating of a book""" with patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async"): From eee4e30e25f50b468c65ad27fec25bde4cd25491 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 17 Jul 2023 11:20:36 -0700 Subject: [PATCH 2/6] Adds managment command to repair editions in bad state --- .../management/commands/repair_editions.py | 21 +++++++++++++++++++ bookwyrm/models/activitypub_mixin.py | 12 +++++++++++ bookwyrm/models/book.py | 21 +++++++++++++++++++ 3 files changed, 54 insertions(+) create mode 100644 bookwyrm/management/commands/repair_editions.py diff --git a/bookwyrm/management/commands/repair_editions.py b/bookwyrm/management/commands/repair_editions.py new file mode 100644 index 0000000000..d43ad6ca63 --- /dev/null +++ b/bookwyrm/management/commands/repair_editions.py @@ -0,0 +1,21 @@ +""" Repair editions with missing works """ +from django.core.management.base import BaseCommand +from bookwyrm import models + + +class Commmand(BaseCommand): + """command-line options""" + + help = "Repairs an edition that is in a broken state" + + # pylint: disable=unused-argument + def handle(self, *args, **options): + """Find and repair broken editions""" + # Find broken editions + editions = models.Edition.objects.filter(parent_work__isnull=True) + self.stdout.write(f"Repairing {editions.count()} edition(s):") + + # Do repair + for edition in editions: + edition.repair() + self.stdout.write(".", ending="") diff --git a/bookwyrm/models/activitypub_mixin.py b/bookwyrm/models/activitypub_mixin.py index d1ca3747ac..3139e2de8a 100644 --- a/bookwyrm/models/activitypub_mixin.py +++ b/bookwyrm/models/activitypub_mixin.py @@ -126,6 +126,18 @@ def find_existing(cls, data): # there OUGHT to be only one match return match.first() + def get_dedpulication_field_json(self): + """A json blob of deduplication fields (like remote_id, or ISBN)""" + data = {} + for field in self._meta.get_fields(): + if ( + not hasattr(field, "deduplication_field") + or not field.deduplication_field + ): + continue + data[field.name] = getattr(self, field.name) + return data + def broadcast(self, activity, sender, software=None, queue=BROADCAST): """send out an activity""" broadcast_task.apply_async( diff --git a/bookwyrm/models/book.py b/bookwyrm/models/book.py index c25f8fee2b..88d5682881 100644 --- a/bookwyrm/models/book.py +++ b/bookwyrm/models/book.py @@ -2,6 +2,7 @@ from itertools import chain import re +from django.apps import apps from django.contrib.postgres.search import SearchVectorField from django.contrib.postgres.indexes import GinIndex from django.core.cache import cache @@ -380,6 +381,26 @@ def save(self, *args, **kwargs): return super().save(*args, **kwargs) + def repair(self): + """If an edition is in a bad state (missing a work), let's fix that""" + # made sure it actually NEEDS reapir + if self.parent_work: + return + + # try to find a duplicate of this edition first + model = apps.get_model("bookwyrm.Edition", require_ready=True) + data = self.get_dedpulication_field_json() + existing_match = model.find_existing(data) + + # assign this edition to the parent of the duplicate edition + new_work = existing_match.parent_work + # if not, create a new work for it + if not new_work: + new_work = models.Work.objects.create(title=self.title) + + self.parent_work = new_work + self.save(update_fields=["parent_work"], broadcast=False) + @classmethod def viewer_aware_objects(cls, viewer): """annotate a book query with metadata related to the user""" From 8b88de624d04167f3e910e925a2a68622bae9449 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Mon, 17 Jul 2023 20:00:45 -0700 Subject: [PATCH 3/6] Adds test and fixes logic errors --- bookwyrm/models/book.py | 8 ++++---- bookwyrm/tests/models/test_book_model.py | 10 ++++++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/bookwyrm/models/book.py b/bookwyrm/models/book.py index 88d5682881..6c40061b97 100644 --- a/bookwyrm/models/book.py +++ b/bookwyrm/models/book.py @@ -393,10 +393,10 @@ def repair(self): existing_match = model.find_existing(data) # assign this edition to the parent of the duplicate edition - new_work = existing_match.parent_work - # if not, create a new work for it - if not new_work: - new_work = models.Work.objects.create(title=self.title) + if existing_match and existing_match.parent_work: + new_work = existing_match.parent_work + else: + new_work = Work.objects.create(title=self.title) self.parent_work = new_work self.save(update_fields=["parent_work"], broadcast=False) diff --git a/bookwyrm/tests/models/test_book_model.py b/bookwyrm/tests/models/test_book_model.py index 825f42b87c..a615a76afa 100644 --- a/bookwyrm/tests/models/test_book_model.py +++ b/bookwyrm/tests/models/test_book_model.py @@ -143,3 +143,13 @@ def test_populate_sort_title(self): for article in articles ) self.assertTrue(all(book.sort_title == "test edition" for book in books)) + + def test_repair_edition(self): + """Fix editions with no works""" + edition = models.Edition.objects.create(title="test") + self.assertIsNone(edition.parent_work) + + edition.repair() + edition.refresh_from_db() + + self.assertEqual(edition.parent_work.title, "test") From ccf3a4c5c193319238265125e6e1e12deb981bf7 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Tue, 18 Jul 2023 19:33:02 -0700 Subject: [PATCH 4/6] Skip trying to match editions It's rare that it will be useful, and it was a huge hassle. --- bookwyrm/models/activitypub_mixin.py | 12 ------------ bookwyrm/models/book.py | 13 +++---------- bookwyrm/tests/models/test_book_model.py | 5 +++-- 3 files changed, 6 insertions(+), 24 deletions(-) diff --git a/bookwyrm/models/activitypub_mixin.py b/bookwyrm/models/activitypub_mixin.py index 3139e2de8a..d1ca3747ac 100644 --- a/bookwyrm/models/activitypub_mixin.py +++ b/bookwyrm/models/activitypub_mixin.py @@ -126,18 +126,6 @@ def find_existing(cls, data): # there OUGHT to be only one match return match.first() - def get_dedpulication_field_json(self): - """A json blob of deduplication fields (like remote_id, or ISBN)""" - data = {} - for field in self._meta.get_fields(): - if ( - not hasattr(field, "deduplication_field") - or not field.deduplication_field - ): - continue - data[field.name] = getattr(self, field.name) - return data - def broadcast(self, activity, sender, software=None, queue=BROADCAST): """send out an activity""" broadcast_task.apply_async( diff --git a/bookwyrm/models/book.py b/bookwyrm/models/book.py index 6c40061b97..6a779b0f63 100644 --- a/bookwyrm/models/book.py +++ b/bookwyrm/models/book.py @@ -2,7 +2,6 @@ from itertools import chain import re -from django.apps import apps from django.contrib.postgres.search import SearchVectorField from django.contrib.postgres.indexes import GinIndex from django.core.cache import cache @@ -381,22 +380,16 @@ def save(self, *args, **kwargs): return super().save(*args, **kwargs) + @transaction.atomic def repair(self): """If an edition is in a bad state (missing a work), let's fix that""" # made sure it actually NEEDS reapir if self.parent_work: return - # try to find a duplicate of this edition first - model = apps.get_model("bookwyrm.Edition", require_ready=True) - data = self.get_dedpulication_field_json() - existing_match = model.find_existing(data) - # assign this edition to the parent of the duplicate edition - if existing_match and existing_match.parent_work: - new_work = existing_match.parent_work - else: - new_work = Work.objects.create(title=self.title) + new_work = Work.objects.create(title=self.title) + new_work.authors.set(self.authors.all()) self.parent_work = new_work self.save(update_fields=["parent_work"], broadcast=False) diff --git a/bookwyrm/tests/models/test_book_model.py b/bookwyrm/tests/models/test_book_model.py index a615a76afa..6dd83b7641 100644 --- a/bookwyrm/tests/models/test_book_model.py +++ b/bookwyrm/tests/models/test_book_model.py @@ -24,8 +24,7 @@ def setUp(self): title="Example Work", remote_id="https://example.com/book/1" ) self.first_edition = models.Edition.objects.create( - title="Example Edition", - parent_work=self.work, + title="Example Edition", parent_work=self.work, isbn_10="1111111111" ) self.second_edition = models.Edition.objects.create( title="Another Example Edition", @@ -147,9 +146,11 @@ def test_populate_sort_title(self): def test_repair_edition(self): """Fix editions with no works""" edition = models.Edition.objects.create(title="test") + edition.authors.set([models.Author.objects.create(name="Author Name")]) self.assertIsNone(edition.parent_work) edition.repair() edition.refresh_from_db() self.assertEqual(edition.parent_work.title, "test") + self.assertEqual(edition.parent_work.authors.count(), 1) From eae06602a990de56ab3b5e6e5ffe968f8f7cc520 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Fri, 21 Jul 2023 14:38:28 -0700 Subject: [PATCH 5/6] Fixes test data --- bookwyrm/tests/models/test_book_model.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bookwyrm/tests/models/test_book_model.py b/bookwyrm/tests/models/test_book_model.py index 6dd83b7641..590b7b8738 100644 --- a/bookwyrm/tests/models/test_book_model.py +++ b/bookwyrm/tests/models/test_book_model.py @@ -24,7 +24,7 @@ def setUp(self): title="Example Work", remote_id="https://example.com/book/1" ) self.first_edition = models.Edition.objects.create( - title="Example Edition", parent_work=self.work, isbn_10="1111111111" + title="Example Edition", parent_work=self.work ) self.second_edition = models.Edition.objects.create( title="Another Example Edition", From 455b0c82eafa2edff623109c06cf4a62c0d22e29 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Tue, 1 Aug 2023 20:53:06 -0700 Subject: [PATCH 6/6] Fixes typo and outdated comment --- bookwyrm/management/commands/repair_editions.py | 2 +- bookwyrm/models/book.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/bookwyrm/management/commands/repair_editions.py b/bookwyrm/management/commands/repair_editions.py index d43ad6ca63..304cd5e51f 100644 --- a/bookwyrm/management/commands/repair_editions.py +++ b/bookwyrm/management/commands/repair_editions.py @@ -3,7 +3,7 @@ from bookwyrm import models -class Commmand(BaseCommand): +class Command(BaseCommand): """command-line options""" help = "Repairs an edition that is in a broken state" diff --git a/bookwyrm/models/book.py b/bookwyrm/models/book.py index 6a779b0f63..c213bf9ce4 100644 --- a/bookwyrm/models/book.py +++ b/bookwyrm/models/book.py @@ -387,7 +387,6 @@ def repair(self): if self.parent_work: return - # assign this edition to the parent of the duplicate edition new_work = Work.objects.create(title=self.title) new_work.authors.set(self.authors.all())