From 54aa2f51f421e846416ed88dcbcdb292c1db9f85 Mon Sep 17 00:00:00 2001 From: Thomas Sileo Date: Mon, 19 Sep 2022 20:31:54 +0200 Subject: [PATCH] Improve replies counter handling --- app/boxes.py | 65 ++++++++++++++++++++++++++++++++++++++------ app/models.py | 4 +-- tests/test_outbox.py | 20 +++++++++----- 3 files changed, 72 insertions(+), 17 deletions(-) diff --git a/app/boxes.py b/app/boxes.py index b475a50..34f36ee 100644 --- a/app/boxes.py +++ b/app/boxes.py @@ -130,7 +130,11 @@ async def send_delete(db_session: AsyncSession, ap_object_id: str) -> None: db_session, outbox_object_to_delete.in_reply_to ) if replied_object: - replied_object.replies_count = replied_object.replies_count - 1 + new_replies_count = await _get_replies_count( + db_session, replied_object.ap_id + ) + + replied_object.replies_count = new_replies_count if replied_object.replies_count < 0: logger.warning("negative replies count for {replied_object.ap_id}") replied_object.replies_count = 0 @@ -344,7 +348,7 @@ async def fetch_conversation_root( is_root: bool = False, ) -> str: """Some softwares do not set the context/conversation field (like Misskey). - This means we have to track conversation ourselves. To do set, we fetch + This means we have to track conversation ourselves. To do so, we fetch the root of the conversation and either: - use the context field if set - or build a custom conversation ID @@ -366,7 +370,12 @@ async def fetch_conversation_root( db_session, ap.get_actor_id(raw_reply) ) in_reply_to_object = RemoteObject(raw_reply, actor=raw_reply_actor) - except (ap.ObjectNotFoundError, ap.ObjectIsGoneError, ap.NotAnObjectError): + except ( + ap.ObjectNotFoundError, + ap.ObjectIsGoneError, + ap.NotAnObjectError, + ap.ObjectUnavailableError, + ): return await fetch_conversation_root(db_session, obj, is_root=True) except httpx.HTTPStatusError as http_status_error: if 400 <= http_status_error.response.status_code < 500: @@ -1020,6 +1029,29 @@ async def _handle_delete_activity( await db_session.flush() +async def _get_replies_count( + db_session: AsyncSession, + replied_object_ap_id: str, +) -> int: + return ( + await db_session.scalar( + select(func.count(models.InboxObject.id)).where( + func.json_extract(models.InboxObject.ap_object, "$.inReplyTo") + == replied_object_ap_id, + models.InboxObject.is_deleted.is_(False), + ) + ) + ) + ( + await db_session.scalar( + select(func.count(models.OutboxObject.id)).where( + func.json_extract(models.OutboxObject.ap_object, "$.inReplyTo") + == replied_object_ap_id, + models.OutboxObject.is_deleted.is_(False), + ) + ) + ) + + async def _revert_side_effect_for_deleted_object( db_session: AsyncSession, delete_activity: models.InboxObject, @@ -1040,20 +1072,28 @@ async def _revert_side_effect_for_deleted_object( # also needs to be forwarded is_delete_needs_to_be_forwarded = True + new_replies_count = await _get_replies_count( + db_session, replied_object.ap_id + ) + await db_session.execute( update(models.OutboxObject) .where( models.OutboxObject.id == replied_object.id, ) - .values(replies_count=models.OutboxObject.replies_count - 1) + .values(replies_count=new_replies_count) ) else: + new_replies_count = await _get_replies_count( + db_session, replied_object.ap_id + ) + await db_session.execute( update(models.InboxObject) .where( models.InboxObject.id == replied_object.id, ) - .values(replies_count=models.InboxObject.replies_count - 1) + .values(replies_count=new_replies_count) ) if deleted_ap_object.ap_type == "Like" and deleted_ap_object.activity_object_ap_id: @@ -1484,8 +1524,9 @@ async def _handle_create_activity( logger.warning( f"Got a Delete for {ro.ap_id} from {delete_object.actor.ap_id}??" ) + return None else: - logger.info("Got a Delete for this object, deleting activity") + logger.info("Already received a Delete for this object, deleting activity") create_activity.is_deleted = True await db_session.flush() return None @@ -1591,20 +1632,28 @@ async def _process_note_object( replied_object, # type: ignore # outbox check below ) else: + new_replies_count = await _get_replies_count( + db_session, replied_object.ap_id + ) + await db_session.execute( update(models.OutboxObject) .where( models.OutboxObject.id == replied_object.id, ) - .values(replies_count=models.OutboxObject.replies_count + 1) + .values(replies_count=new_replies_count) ) else: + new_replies_count = await _get_replies_count( + db_session, replied_object.ap_id + ) + await db_session.execute( update(models.InboxObject) .where( models.InboxObject.id == replied_object.id, ) - .values(replies_count=models.InboxObject.replies_count + 1) + .values(replies_count=new_replies_count) ) # This object is a reply of a local object, we may need to forward it diff --git a/app/models.py b/app/models.py index 30600d1..0708341 100644 --- a/app/models.py +++ b/app/models.py @@ -75,7 +75,7 @@ class InboxObject(Base, BaseObject): ap_actor_id = Column(String, nullable=False) ap_type = Column(String, nullable=False, index=True) - ap_id = Column(String, nullable=False, unique=True, index=True) + ap_id: Mapped[str] = Column(String, nullable=False, unique=True, index=True) ap_context = Column(String, nullable=True) ap_published_at = Column(DateTime(timezone=True), nullable=False) ap_object: Mapped[ap.RawObject] = Column(JSON, nullable=False) @@ -160,7 +160,7 @@ class OutboxObject(Base, BaseObject): public_id = Column(String, nullable=False, index=True) ap_type = Column(String, nullable=False, index=True) - ap_id = Column(String, nullable=False, unique=True, index=True) + ap_id: Mapped[str] = Column(String, nullable=False, unique=True, index=True) ap_context = Column(String, nullable=True) ap_object: Mapped[ap.RawObject] = Column(JSON, nullable=False) diff --git a/tests/test_outbox.py b/tests/test_outbox.py index 04a511e..5159e8f 100644 --- a/tests/test_outbox.py +++ b/tests/test_outbox.py @@ -77,23 +77,29 @@ def test_send_delete__reverts_side_effects( # with a note that has existing replies inbox_note = setup_inbox_note(actor) - inbox_note.replies_count = 1 + # with a bogus counter + inbox_note.replies_count = 5 db.commit() - # and a local reply - outbox_note = setup_outbox_note( + # and 2 local replies + setup_outbox_note( + to=[ap.AS_PUBLIC], + cc=[LOCAL_ACTOR.followers_collection_id], # type: ignore + in_reply_to=inbox_note.ap_id, + ) + outbox_note2 = setup_outbox_note( to=[ap.AS_PUBLIC], cc=[LOCAL_ACTOR.followers_collection_id], # type: ignore in_reply_to=inbox_note.ap_id, ) - inbox_note.replies_count = inbox_note.replies_count + 1 db.commit() + # When deleting one of the replies response = client.post( "/admin/actions/delete", data={ "redirect_url": "http://testserver/", - "ap_object_id": outbox_note.ap_id, + "ap_object_id": outbox_note2.ap_id, "csrf_token": generate_csrf_token(), }, cookies=generate_admin_session_cookies(), @@ -108,14 +114,14 @@ def test_send_delete__reverts_side_effects( select(models.OutboxObject).where(models.OutboxObject.ap_type == "Delete") ).scalar_one() assert outbox_object.ap_type == "Delete" - assert outbox_object.activity_object_ap_id == outbox_note.ap_id + assert outbox_object.activity_object_ap_id == outbox_note2.ap_id # And an outgoing activity was queued outgoing_activity = db.execute(select(models.OutgoingActivity)).scalar_one() assert outgoing_activity.outbox_object_id == outbox_object.id assert outgoing_activity.recipient == ra.inbox_url - # And the replies count of the replied object was decremented + # And the replies count of the replied object was refreshed correctly db.refresh(inbox_note) assert inbox_note.replies_count == 1