From 0d621b68e0ff2685e3ef0711d46156a71000918f Mon Sep 17 00:00:00 2001 From: Bart Schuurmans Date: Fri, 29 Mar 2024 21:12:59 +0100 Subject: [PATCH] Reorder operations in save() overrides Accessing many-to-many relations before saving is no longer allowed. Reorder all operations consistently: 1. Validations 2. Modify own fields 3. Perform save by calling super().save() 4. Modify related objects and clear caches Especially clearing caches should be done after actually saving, otherwise the old data can be re-added immediately by another request before the new data is written. --- bookwyrm/models/author.py | 2 +- bookwyrm/models/book.py | 19 ++++++++++--------- bookwyrm/models/readthrough.py | 6 ++++-- bookwyrm/models/relationship.py | 6 ++++-- bookwyrm/models/shelf.py | 8 ++++++-- bookwyrm/models/site.py | 8 +++++--- bookwyrm/models/status.py | 3 ++- 7 files changed, 32 insertions(+), 20 deletions(-) diff --git a/bookwyrm/models/author.py b/bookwyrm/models/author.py index abe78dafb..8ea1858fd 100644 --- a/bookwyrm/models/author.py +++ b/bookwyrm/models/author.py @@ -50,7 +50,7 @@ class Author(BookDataModel): if self.isni: self.isni = re.sub(r"\s", "", self.isni) - return super().save(*args, **kwargs) + super().save(*args, **kwargs) @property def isni_link(self): diff --git a/bookwyrm/models/book.py b/bookwyrm/models/book.py index 8e957b717..6fc447228 100644 --- a/bookwyrm/models/book.py +++ b/bookwyrm/models/book.py @@ -103,7 +103,7 @@ class BookDataModel(ObjectMixin, BookWyrmModel): else: self.origin_id = self.remote_id self.remote_id = None - return super().save(*args, **kwargs) + super().save(*args, **kwargs) # pylint: disable=arguments-differ def broadcast(self, activity, sender, software="bookwyrm", **kwargs): @@ -323,7 +323,7 @@ class Book(BookDataModel): if not isinstance(self, (Edition, Work)): raise ValueError("Books should be added as Editions or Works") - return super().save(*args, **kwargs) + super().save(*args, **kwargs) def get_remote_id(self): """editions and works both use "book" instead of model_name""" @@ -400,10 +400,11 @@ class Work(OrderedCollectionPageMixin, Book): def save(self, *args, **kwargs): """set some fields on the edition object""" + super().save(*args, **kwargs) + # set rank for edition in self.editions.all(): edition.save() - return super().save(*args, **kwargs) @property def default_edition(self): @@ -526,16 +527,16 @@ class Edition(Book): # set rank self.edition_rank = self.get_rank() - # clear author cache - if self.id: - for author_id in self.authors.values_list("id", flat=True): - cache.delete(f"author-books-{author_id}") - # Create sort title by removing articles from title if self.sort_title in [None, ""]: self.sort_title = self.guess_sort_title() - return super().save(*args, **kwargs) + super().save(*args, **kwargs) + + # clear author cache + if self.id: + for author_id in self.authors.values_list("id", flat=True): + cache.delete(f"author-books-{author_id}") @transaction.atomic def repair(self): diff --git a/bookwyrm/models/readthrough.py b/bookwyrm/models/readthrough.py index 4911c715b..910b2a7a9 100644 --- a/bookwyrm/models/readthrough.py +++ b/bookwyrm/models/readthrough.py @@ -32,13 +32,15 @@ class ReadThrough(BookWyrmModel): def save(self, *args, **kwargs): """update user active time""" - cache.delete(f"latest_read_through-{self.user_id}-{self.book_id}") - self.user.update_active_date() # an active readthrough must have an unset finish date if self.finish_date or self.stopped_date: self.is_active = False + super().save(*args, **kwargs) + cache.delete(f"latest_read_through-{self.user_id}-{self.book_id}") + self.user.update_active_date() + def create_update(self): """add update to the readthrough""" if self.progress: diff --git a/bookwyrm/models/relationship.py b/bookwyrm/models/relationship.py index 3386a02dc..745ff78b6 100644 --- a/bookwyrm/models/relationship.py +++ b/bookwyrm/models/relationship.py @@ -38,14 +38,16 @@ class UserRelationship(BookWyrmModel): def save(self, *args, **kwargs): """clear the template cache""" - clear_cache(self.user_subject, self.user_object) super().save(*args, **kwargs) + clear_cache(self.user_subject, self.user_object) + def delete(self, *args, **kwargs): """clear the template cache""" - clear_cache(self.user_subject, self.user_object) super().delete(*args, **kwargs) + clear_cache(self.user_subject, self.user_object) + class Meta: """relationships should be unique""" diff --git a/bookwyrm/models/shelf.py b/bookwyrm/models/shelf.py index 4b4e3cd8d..77c2d26d9 100644 --- a/bookwyrm/models/shelf.py +++ b/bookwyrm/models/shelf.py @@ -44,6 +44,7 @@ class Shelf(OrderedCollectionMixin, BookWyrmModel): """set the identifier""" super().save(*args, priority=priority, **kwargs) if not self.identifier: + # this needs the auto increment ID from the save() above self.identifier = self.get_identifier() super().save(*args, **kwargs, broadcast=False) @@ -103,7 +104,11 @@ class ShelfBook(CollectionItemMixin, BookWyrmModel): def save(self, *args, priority=BROADCAST, **kwargs): if not self.user: self.user = self.shelf.user - if self.id and self.user.local: + + is_update = self.id is not None + super().save(*args, priority=priority, **kwargs) + + if is_update and self.user.local: # remove all caches related to all editions of this book cache.delete_many( [ @@ -111,7 +116,6 @@ class ShelfBook(CollectionItemMixin, BookWyrmModel): for book in self.book.parent_work.editions.all() ] ) - super().save(*args, priority=priority, **kwargs) def delete(self, *args, **kwargs): if self.id and self.user.local: diff --git a/bookwyrm/models/site.py b/bookwyrm/models/site.py index 36e6bb128..89d6ef395 100644 --- a/bookwyrm/models/site.py +++ b/bookwyrm/models/site.py @@ -139,13 +139,15 @@ class SiteSettings(SiteModel): def save(self, *args, **kwargs): """if require_confirm_email is disabled, make sure no users are pending, if enabled, make sure invite_question_text is not empty""" + if not self.invite_question_text: + self.invite_question_text = "What is your favourite book?" + + super().save(*args, **kwargs) + if not self.require_confirm_email: User.objects.filter(is_active=False, deactivation_reason="pending").update( is_active=True, deactivation_reason=None ) - if not self.invite_question_text: - self.invite_question_text = "What is your favourite book?" - super().save(*args, **kwargs) class Theme(SiteModel): diff --git a/bookwyrm/models/status.py b/bookwyrm/models/status.py index d0c1e639b..5b953d077 100644 --- a/bookwyrm/models/status.py +++ b/bookwyrm/models/status.py @@ -459,9 +459,10 @@ class Review(BookStatus): def save(self, *args, **kwargs): """clear rating caches""" + super().save(*args, **kwargs) + if self.book.parent_work: cache.delete(f"book-rating-{self.book.parent_work.id}") - super().save(*args, **kwargs) class ReviewRating(Review):