[2.x] fix(messages): repair dialogs left pointing at a deleted first or last message (#4851)

* Fix dialogs left pointing at a deleted first or last message

Deleting the first or last message of a conversation could leave the dialog
with a null first_message_id or last_message_id, after which the conversation
would not render at all: MessageStream read straight through the missing
relationship and threw "Cannot read properties of null (reading 'id')". Since
the reply box goes down with the rest of the stream, the one thing that repairs
the pointer, posting another message, was out of reach.

Both columns are declared with nullOnDelete(), so the database sets the column
to null as part of the delete. The repair in DialogMessage's deleted handler
then compared the dialog's first_message_id against the id of the message that
had just been removed, but by then that column can already be null, so the
comparison missed and no repair ran. It only worked when the dialog happened to
be in memory before the delete, which is why this depends on an unrelated
setting: DialogMessagePolicy only loads the dialog when
allow_delete_own_messages is "until reply", and PHP short-circuits past that
check for the other values.

The handler now treats a pointer that is missing as one that needs recomputing,
and uses two conditions rather than an if/elseif chain so both ends can be
repaired in the same pass. Dialogs already left without pointers are repaired
the next time a message in them is deleted.

Two related fixes:

- DialogMessageResource set first_message_id to the newly created message
  whenever it was missing. For a new dialog that is the same thing, but for a
  dialog whose pointer had gone missing it recorded the newest message as the
  first one, which left the stream showing a "load previous messages" button
  that had nothing to load. It now starts from the oldest surviving message.

- The frontend read the first and last message relationships in three places
  without allowing for their absence (MessageStream.content, timeGap and the
  mark-as-read button in DialogListItem), so any dialog in this state took the
  whole page down. They now go through Dialog.messageRelationshipId(), which
  returns undefined when the relationship is not there. With no first message
  there is nothing older to load, with no last message nothing newer, and
  marking as read is skipped rather than sending a request built from null.

Added integration coverage for deleting the first, last, middle and only
message of a dialog, for repairing a dialog that is already in this state, and
for the dialog remaining readable afterwards. The first four fail against the
current code and pass with this change; the middle-message and only-message
cases pass either way and are there to pin the behaviour that should not
change.

* Cast the dialog's message pointers instead of comparing loosely

The repair in the deleted handler compared the dialog's pointer against the
deleted message's id with ==, which worked but hid the reason a strict
comparison could miss: Dialog casts neither id column, so the value comes back
however the driver hands it over while $message->id is an int.

Casting both columns to integer, as core's Discussion model already does for
first_post_id and last_post_id, makes the comparison mean what it says, so the
handler is back to ===.

Author: karl-bullock

extensions/messages/js/src/common/models/Dialog.ts

@@ -1,4 +1,4 @@
-import Model from 'flarum/common/Model';
+import Model, { type ModelIdentifier } from 'flarum/common/Model';
 import User from 'flarum/common/models/User';
 import DialogMessage from './DialogMessage';
 import app from 'flarum/common/app';
@@ -27,6 +27,17 @@ export default class Dialog extends Model {
     return Model.hasOne<DialogMessage>('lastMessage').call(this);
   }
 
+  /**
+   * The id of this dialog's first or last message, when it has one.
+   *
+   * A dialog can be left without either, for instance when the message one of
+   * them pointed at was deleted, so every caller has to cope with the
+   * relationship being absent rather than reading straight through it.
+   */
+  messageRelationshipId(name: 'firstMessage' | 'lastMessage'): string | undefined {
+    return (this.data.relationships?.[name]?.data as ModelIdentifier | undefined)?.id;
+  }
+
   unreadCount() {
     return Model.attribute<number>('unreadCount').call(this);
   }

extensions/messages/js/src/forum/components/DialogListItem.tsx

@@ -67,17 +67,19 @@ export default class DialogListItem<CustomAttrs extends IDialogListItemAttrs = I
           e.preventDefault();
           e.stopPropagation();
 
-          this.attrs.dialog
-            .save({ lastReadMessageId: (this.attrs.dialog.data.relationships?.lastMessage.data as ModelIdentifier).id })
-            .finally(() => {
-              if (this.attrs.dialog.unreadCount() === 0) {
-                app.session.user!.pushAttributes({
-                  messageCount: (app.session.user!.attribute<number>('messageCount') ?? 1) - 1,
-                });
-              }
+          const lastMessageId = this.attrs.dialog.messageRelationshipId('lastMessage');
 
-              m.redraw();
-            });
+          if (!lastMessageId) return;
+
+          this.attrs.dialog.save({ lastReadMessageId: lastMessageId }).finally(() => {
+            if (this.attrs.dialog.unreadCount() === 0) {
+              app.session.user!.pushAttributes({
+                messageCount: (app.session.user!.attribute<number>('messageCount') ?? 1) - 1,
+              });
+            }
+
+            m.redraw();
+          });
         }}
       />,
       100

extensions/messages/js/src/forum/components/MessageStream.tsx

@@ -84,7 +84,13 @@ export default class MessageStream<CustomAttrs extends IDialogStreamAttrs = IDia
     const ReplyPlaceholder = this.replyPlaceholderComponent();
     const LoadingPost = this.loadingPostComponent();
 
-    if (messages[0].id() !== (this.attrs.dialog.data.relationships?.firstMessage.data as ModelIdentifier).id) {
+    // A dialog without a first or last message has nothing to load in that
+    // direction, and reading straight through the missing relationship would
+    // take the whole conversation down with it.
+    const firstMessageId = this.attrs.dialog.messageRelationshipId('firstMessage');
+    const lastMessageId = this.attrs.dialog.messageRelationshipId('lastMessage');
+
+    if (firstMessageId && messages[0]?.id() !== firstMessageId) {
       items.push(
         <div className="MessageStream-item" key="loadNext">
           <Button
@@ -108,7 +114,7 @@ export default class MessageStream<CustomAttrs extends IDialogStreamAttrs = IDia
 
     messages.forEach((message, index) => items.push(this.messageItem(message, index)));
 
-    if (messages[messages.length - 1].id() !== (this.attrs.dialog.data.relationships?.lastMessage.data as ModelIdentifier).id) {
+    if (lastMessageId && messages[messages.length - 1]?.id() !== lastMessageId) {
       if (LoadingPost) {
         items.push(
           <div className="MessageStream-item" key="loading-prev">
@@ -167,7 +173,7 @@ export default class MessageStream<CustomAttrs extends IDialogStreamAttrs = IDia
   }
 
   timeGap(message: DialogMessage): Mithril.Children {
-    if (message.id() === (this.attrs.dialog.data.relationships?.firstMessage.data as ModelIdentifier).id) {
+    if (message.id() === this.attrs.dialog.messageRelationshipId('firstMessage')) {
       this.lastTime = message.createdAt()!;
 
       return (

extensions/messages/src/Api/Resource/DialogMessageResource.php

@@ -271,7 +271,10 @@ public function created(object $model, OriginalContext $context): ?object
         }
 
         if (! $model->dialog->first_message_id) {
-            $model->dialog->setFirstMessage($model);
+            // Normally this is the first message of a new dialog. It can also be
+            // a dialog whose first message pointer went missing, though, and
+            // that one starts at its oldest surviving message, not at this one.
+            $model->dialog->setFirstMessage($model->dialog->messages()->oldest('id')->first() ?? $model);
         }
 
         $model->dialog->isDirty() && $model->dialog->save();

extensions/messages/src/Dialog.php

@@ -43,6 +43,8 @@ class Dialog extends AbstractModel
     protected $table = 'dialogs';
 
     protected $casts = [
+        'first_message_id' => 'integer',
+        'last_message_id' => 'integer',
         'last_message_at' => 'datetime'
     ];
 

extensions/messages/src/DialogMessage.php

@@ -72,21 +72,40 @@ public static function boot()
         });
 
         static::deleted(function (self $message) {
-            if ($message->dialog) {
-                if ($message->dialog->messages()->count() === 0) {
-                    $message->dialog->delete();
-                } elseif ($message->dialog->first_message_id === $message->id) {
-                    $message->dialog->setFirstMessage(
-                        $message->dialog->messages()->oldest('id')->first()
-                    );
-                    $message->dialog->save();
-                } elseif ($message->dialog->last_message_id === $message->id) {
-                    $message->dialog->setLastMessage(
-                        $message->dialog->messages()->latest('id')->first()
-                    );
-                    $message->dialog->save();
+            $dialog = $message->dialog;
+
+            if (! $dialog) {
+                return;
+            }
+
+            if ($dialog->messages()->count() === 0) {
+                $dialog->delete();
+
+                return;
+            }
+
+            // Both columns are declared with nullOnDelete(), so by the time this
+            // runs the database may already have set whichever one pointed at
+            // this message to null. Unless the dialog happened to be loaded
+            // before the delete, reading it back gives null rather than the id
+            // being looked for, so a pointer that is missing is treated as one
+            // that needs recomputing. That also repairs dialogs left pointing at
+            // nothing by earlier deletions.
+            if ($dialog->first_message_id === null || $dialog->first_message_id === $message->id) {
+                if ($first = $dialog->messages()->oldest('id')->first()) {
+                    $dialog->setFirstMessage($first);
                 }
             }
+
+            if ($dialog->last_message_id === null || $dialog->last_message_id === $message->id) {
+                if ($last = $dialog->messages()->latest('id')->first()) {
+                    $dialog->setLastMessage($last);
+                }
+            }
+
+            if ($dialog->isDirty()) {
+                $dialog->save();
+            }
         });
     }
 

extensions/messages/tests/integration/api/dialog_messages/DeleteTest.php

@@ -0,0 +1,143 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Messages\Tests\integration\api\dialog_messages;
+
+use Carbon\Carbon;
+use Flarum\Messages\Dialog;
+use Flarum\Messages\DialogMessage;
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use Flarum\Testing\integration\TestCase;
+use Flarum\User\User;
+
+class DeleteTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->extension('flarum-messages');
+
+        $this->prepareDatabase([
+            User::class => [
+                ['id' => 3, 'username' => 'alice'],
+                ['id' => 4, 'username' => 'bob'],
+            ],
+            Dialog::class => [
+                ['id' => 102, 'type' => 'direct', 'first_message_id' => 102, 'last_message_id' => 104],
+            ],
+            DialogMessage::class => [
+                ['id' => 102, 'dialog_id' => 102, 'user_id' => 3, 'content' => 'First', 'number' => 1],
+                ['id' => 103, 'dialog_id' => 102, 'user_id' => 3, 'content' => 'Second', 'number' => 2],
+                ['id' => 104, 'dialog_id' => 102, 'user_id' => 3, 'content' => 'Third', 'number' => 3],
+            ],
+            'dialog_user' => [
+                ['dialog_id' => 102, 'user_id' => 3, 'joined_at' => Carbon::now()],
+                ['dialog_id' => 102, 'user_id' => 4, 'joined_at' => Carbon::now()],
+            ],
+        ]);
+
+        // Anything other than "until reply" reaches the delete without the
+        // policy having loaded the dialog, which is the case the first/last
+        // pointers used to be lost in.
+        $this->setting('flarum-messages.allow_delete_own_messages', '-1');
+    }
+
+    protected function delete(int $messageId, int $actor = 3): int
+    {
+        return $this->send(
+            $this->request('DELETE', '/api/dialog-messages/'.$messageId, ['authenticatedAs' => $actor])
+        )->getStatusCode();
+    }
+
+    /**
+     * Read through the query builder rather than the model, so this works
+     * before the application has been booted by a request.
+     */
+    protected function dialogRow(): ?object
+    {
+        return $this->database()->table('dialogs')->where('id', 102)->first();
+    }
+
+    public function test_deleting_the_first_message_moves_the_pointer_to_the_next_one(): void
+    {
+        $this->assertEquals(204, $this->delete(102));
+
+        $dialog = $this->dialogRow();
+
+        // The foreign key nulls this column on delete, so it has to be
+        // recomputed rather than left pointing at nothing.
+        $this->assertEquals(103, $dialog->first_message_id);
+        $this->assertEquals(104, $dialog->last_message_id);
+    }
+
+    public function test_deleting_the_last_message_moves_the_pointer_to_the_previous_one(): void
+    {
+        $this->assertEquals(204, $this->delete(104));
+
+        $dialog = $this->dialogRow();
+
+        $this->assertEquals(102, $dialog->first_message_id);
+        $this->assertEquals(103, $dialog->last_message_id);
+    }
+
+    public function test_deleting_a_message_in_the_middle_leaves_both_pointers_alone(): void
+    {
+        $this->assertEquals(204, $this->delete(103));
+
+        $dialog = $this->dialogRow();
+
+        $this->assertEquals(102, $dialog->first_message_id);
+        $this->assertEquals(104, $dialog->last_message_id);
+    }
+
+    public function test_deleting_the_only_message_deletes_the_dialog(): void
+    {
+        $this->assertEquals(204, $this->delete(102));
+        $this->assertEquals(204, $this->delete(103));
+        $this->assertEquals(204, $this->delete(104));
+
+        $this->assertNull($this->dialogRow());
+    }
+
+    public function test_a_dialog_left_without_pointers_is_repaired_on_the_next_deletion(): void
+    {
+        // The state an affected forum is already in.
+        $this->database()->table('dialogs')->where('id', 102)->update([
+            'first_message_id' => null,
+            'last_message_id' => null,
+        ]);
+
+        $this->assertEquals(204, $this->delete(103));
+
+        $dialog = $this->dialogRow();
+
+        $this->assertEquals(102, $dialog->first_message_id);
+        $this->assertEquals(104, $dialog->last_message_id);
+    }
+
+    public function test_the_dialog_is_still_readable_after_its_first_message_is_deleted(): void
+    {
+        $this->delete(102);
+
+        $response = $this->send(
+            $this->request('GET', '/api/dialogs/102', ['authenticatedAs' => 3])
+        );
+
+        $this->assertEquals(200, $response->getStatusCode());
+
+        $data = json_decode($response->getBody()->getContents(), true);
+
+        // A null relationship here is what the message stream chokes on.
+        $this->assertNotNull($data['data']['relationships']['firstMessage']['data'] ?? null);
+        $this->assertEquals('103', $data['data']['relationships']['firstMessage']['data']['id']);
+    }
+}