If gitlab throws error 500 try to resolve discussion (#557)

config/services.php

@@ -89,6 +89,7 @@
         ->bind('$gitlabCommentSyncEnabled', '%env(bool:GITLAB_COMMENT_SYNC)%')
         ->bind('$gitlabApiUrl', '%env(GITLAB_API_URL)%')
         ->bind('$applicationName', '%env(APP_NAME)%')
+        ->bind('$appAbsoluteUrl', '%env(APP_ABSOLUTE_URL)%')
         ->bind('$codeReviewExcludeAuthors', '%env(CODE_REVIEW_EXCLUDE_AUTHORS)%')
         ->bind(ProviderInterface::class . ' $collectionProvider', service(CollectionProvider::class));
 

src/MessageHandler/Gitlab/CommentAddedMessageHandler.php

@@ -55,6 +55,11 @@ public function __invoke(CommentAdded $event): void
             return;
         }
 
-        $this->commentService->create($api, $comment, $mergeRequestIId);
+        try {
+            $this->commentService->create($api, $comment, $mergeRequestIId);
+        } catch (Throwable $exception) {
+            $this->commentService->updateExtReferenceId($api, $comment, $mergeRequestIId);
+            throw $exception;
+        }
     }
 }

src/Service/Api/Gitlab/Discussions.php

@@ -5,6 +5,7 @@
 
 use DR\Review\Model\Api\Gitlab\Position;
 use DR\Utils\Arrays;
+use Generator;
 use Symfony\Contracts\HttpClient\HttpClientInterface;
 use Throwable;
 
@@ -14,6 +15,39 @@ public function __construct(private readonly HttpClientInterface $client)
     {
     }
 
+    /**
+     * @phpstan-return Generator<array{
+     *    id: string,
+     *    notes: array<array{
+     *      id: int,
+     *      body: string,
+     *      position: array{
+     *        base_sha: string,
+     *        start_sha: string,
+     *        head_sha: string,
+     *        old_path: string,
+     *        new_path: string,
+     *      }
+     *    }>
+     * }>
+     * @throws Throwable
+     * @link https://docs.gitlab.com/ee/api/discussions.html#merge-requests
+     * @link https://docs.gitlab.com/ee/api/rest/index.html#pagination-link-header
+     */
+    public function getDiscussions(int $projectId, int $mergeRequestIId, int $perPage = 20): Generator
+    {
+        $page = 1;
+        do {
+            $response = $this->client->request(
+                'GET',
+                sprintf('projects/%d/merge_requests/%d/discussions', $projectId, $mergeRequestIId),
+                ['query' => ['per_page' => $perPage, 'page' => $page]]
+            );
+            $page     = (int)($response->getHeaders()['x-next-page'][0] ?? -1);
+            yield from $response->toArray();
+        } while ($page > 0);
+    }
+
     /**
      * @throws Throwable
      * @link https://docs.gitlab.com/ee/api/discussions.html#create-a-new-thread-in-the-merge-request-diff

src/Service/Api/Gitlab/GitlabCommentFormatter.php

@@ -0,0 +1,28 @@
+<?php
+declare(strict_types=1);
+
+namespace DR\Review\Service\Api\Gitlab;
+
+use DR\Review\Controller\App\Review\ReviewController;
+use DR\Review\Entity\Review\Comment;
+use Psr\Log\LoggerAwareInterface;
+use Psr\Log\LoggerAwareTrait;
+use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
+
+class GitlabCommentFormatter implements LoggerAwareInterface
+{
+    use LoggerAwareTrait;
+
+    public function __construct(private readonly string $appAbsoluteUrl, private readonly UrlGeneratorInterface $urlGenerator)
+    {
+    }
+
+    public function format(Comment $comment): string
+    {
+        $url = $this->appAbsoluteUrl . $this->urlGenerator->generate(ReviewController::class, ['review' => $comment->getReview()]);
+
+        $message = str_replace("\n", "\n<br>", $comment->getMessage());
+
+        return sprintf("%s\n<br>\n<br>*123view: %s*", $message, $url);
+    }
+}

src/Service/Api/Gitlab/GitlabCommentService.php

@@ -15,8 +15,11 @@ class GitlabCommentService implements LoggerAwareInterface
 {
     use LoggerAwareTrait;
 
-    public function __construct(private readonly PositionFactory $positionFactory, private readonly CommentRepository $commentRepository)
-    {
+    public function __construct(
+        private readonly PositionFactory $positionFactory,
+        private readonly CommentRepository $commentRepository,
+        private readonly GitlabCommentFormatter $commentFormatter,
+    ) {
     }
 
     /**
@@ -47,11 +50,42 @@ public function create(GitlabApi $api, Comment $comment, int $mergeRequestIId):
             'Adding comment in gitlab: {projectId} {mergeRequestIId} {comment}',
             ['projectId' => $projectId, 'mergeRequestIId' => $mergeRequestIId, 'comment' => $comment->getMessage()]
         );
-        $referenceId = $api->discussions()->createDiscussion($projectId, $mergeRequestIId, $position, $comment->getMessage());
+
+        $message = $this->commentFormatter->format($comment);
+        $referenceId = $api->discussions()->createDiscussion($projectId, $mergeRequestIId, $position, $message);
         $comment->setExtReferenceId($referenceId);
         $this->commentRepository->save($comment, true);
     }
 
+    /**
+     * @throws Throwable
+     */
+    public function updateExtReferenceId(GitlabApi $api, Comment $comment, int $mergeRequestIId): void
+    {
+        $projectId = (int)$comment->getReview()->getRepository()->getRepositoryProperty('gitlab-project-id');
+        $reference = $comment->getLineReference();
+
+        foreach ($api->discussions()->getDiscussions($projectId, $mergeRequestIId) as $thread) {
+            foreach ($thread['notes'] as $note) {
+                // try to match body
+                if ($note['body'] !== $comment->getMessage()) {
+                    continue;
+                }
+
+                // should at least match either new path or old path
+                if ($note['position']['old_path'] !== $reference->oldPath && $note['position']['new_path'] !== $reference->newPath) {
+                    continue;
+                }
+
+                // set reference id and save
+                $referenceId = sprintf('%s:%s:%s', $mergeRequestIId, $thread['id'], $note['id']);
+                $this->commentRepository->save($comment->setExtReferenceId($referenceId), true);
+
+                return;
+            }
+        }
+    }
+
     /**
      * @throws Throwable
      */
@@ -70,7 +104,9 @@ public function update(GitlabApi $api, Comment $comment): void
             'Updating comment in gitlab: {projectId} {mergeRequestIId} {discussionId}',
             ['projectId' => $projectId, 'mergeRequestIId' => $mergeRequestIId, 'discussionId' => $discussionId]
         );
-        $api->discussions()->updateNote($projectId, (int)$mergeRequestIId, $discussionId, $noteId, $comment->getMessage());
+
+        $message = $this->commentFormatter->format($comment);
+        $api->discussions()->updateNote($projectId, (int)$mergeRequestIId, $discussionId, $noteId, $message);
     }
 
     /**
@@ -107,6 +143,12 @@ public function delete(GitlabApi $api, Repository $repository, string $extRefere
             'Deleting comment in gitlab: {projectId} {mergeRequestIId} {discussionId}',
             ['projectId' => $projectId, 'mergeRequestIId' => $mergeRequestIId, 'discussionId' => $discussionId]
         );
-        $api->discussions()->deleteNote($projectId, (int)$mergeRequestIId, $discussionId, $noteId);
+        try {
+            $api->discussions()->deleteNote($projectId, (int)$mergeRequestIId, $discussionId, $noteId);
+            // @codeCoverageIgnoreStart
+        } catch (Throwable $exception) {
+            $this->logger?->notice('Failed to remove note from Gitlab', ['exception' => $exception]);
+        }
+        // @codeCoverageIgnoreEnd
     }
 }

tests/AbstractTestCase.php

@@ -4,6 +4,7 @@
 namespace DR\Review\Tests;
 
 use DigitalRevolution\AccessorPairConstraint\AccessorPairAsserter;
+use Generator;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 use Psr\Log\LoggerInterface;
@@ -18,14 +19,23 @@ abstract class AbstractTestCase extends TestCase
     use AccessorPairAsserter;
     use TestTrait;
 
-    /** @var MockObject&LoggerInterface */
-    protected LoggerInterface $logger;
-
+    protected MockObject&LoggerInterface $logger;
     protected Envelope $envelope;
 
     protected function setUp(): void
     {
         $this->envelope = new Envelope(new stdClass(), []);
         $this->logger   = $this->createMock(LoggerInterface::class);
     }
+
+    /**
+     * @template T
+     * @param T[] $items
+     *
+     * @return Generator<T>
+     */
+    protected static function createGeneratorFrom(array $items): Generator
+    {
+        yield from $items;
+    }
 }

tests/Unit/MessageHandler/Gitlab/CommentAddedMessageHandlerTest.php

@@ -17,6 +17,7 @@
 use DR\Review\Tests\AbstractTestCase;
 use PHPUnit\Framework\Attributes\CoversClass;
 use PHPUnit\Framework\MockObject\MockObject;
+use RuntimeException;
 use Throwable;
 
 #[CoversClass(CommentAddedMessageHandler::class)]
@@ -127,7 +128,35 @@ public function testInvokeSuccess(): void
         $this->apiProvider->expects(self::once())->method('create')->with($repository, $user)->willReturn($api);
         $this->mergeRequestService->expects(self::once())->method('retrieveMergeRequestIID')->with($api, $review)->willReturn(12345);
         $this->commentService->expects(self::once())->method('create')->with($api, $comment, 12345);
+        $this->commentService->expects(self::never())->method('updateExtReferenceId');
 
         ($this->handler)(new CommentAdded(111, 222, 333, 'file', 'message'));
     }
+
+    /**
+     * @throws Throwable
+     */
+    public function testInvokeFailureRetry(): void
+    {
+        $user       = new User();
+        $repository = new Repository();
+
+        $review = new CodeReview();
+        $review->setRepository($repository);
+
+        $comment = new Comment();
+        $comment->setReview($review);
+        $comment->setUser($user);
+
+        $api = $this->createMock(GitlabApi::class);
+
+        $this->commentRepository->expects(self::once())->method('find')->with(222)->willReturn($comment);
+        $this->apiProvider->expects(self::once())->method('create')->with($repository, $user)->willReturn($api);
+        $this->mergeRequestService->expects(self::once())->method('retrieveMergeRequestIID')->with($api, $review)->willReturn(12345);
+        $this->commentService->expects(self::once())->method('create')->with($api, $comment, 12345)->willThrowException(new RuntimeException('foo'));
+        $this->commentService->expects(self::once())->method('updateExtReferenceId')->with($api, $comment, 12345);
+
+        $this->expectException(RuntimeException::class);
+        ($this->handler)(new CommentAdded(111, 222, 333, 'file', 'message'));
+    }
 }

tests/Unit/Service/Api/Gitlab/DiscussionsTest.php

@@ -11,6 +11,7 @@
 use Symfony\Contracts\HttpClient\HttpClientInterface;
 use Symfony\Contracts\HttpClient\ResponseInterface;
 use Throwable;
+use function DR\PHPUnitExtensions\Mock\consecutive;
 
 #[CoversClass(Discussions::class)]
 class DiscussionsTest extends AbstractTestCase
@@ -25,6 +26,42 @@ protected function setUp(): void
         $this->discussions = new Discussions($this->client);
     }
 
+    /**
+     * @throws Throwable
+     */
+    public function testGetDiscussions(): void
+    {
+        $discussionA = ['id' => 333, 'notes' => [['id' => 444]]];
+        $discussionB = ['id' => 555, 'notes' => [['id' => 666]]];
+
+        $response = $this->createMock(ResponseInterface::class);
+        $response->method('getHeaders')->willReturn(['x-next-page' => ['2']], ['x-next-page' => []]);
+        $response->method('toArray')->willReturn([$discussionA], [$discussionB]);
+
+        $this->client->expects(self::exactly(2))
+            ->method('request')
+            ->with(
+                ...consecutive(
+                    [
+                        'GET',
+                        'projects/111/merge_requests/222/discussions',
+                        ['query' => ['per_page' => 20, 'page' => 1]]
+                    ],
+                    [
+                        'GET',
+                        'projects/111/merge_requests/222/discussions',
+                        ['query' => ['per_page' => 20, 'page' => 2]]
+                    ]
+                )
+            )->willReturn($response);
+
+        $discussions = [];
+        foreach ($this->discussions->getDiscussions(111, 222) as $discussion) {
+            $discussions[] = $discussion;
+        }
+        static::assertSame([$discussionA, $discussionB], $discussions);
+    }
+
     /**
      * @throws Throwable
      */

tests/Unit/Service/Api/Gitlab/GitlabCommentFormatterTest.php

@@ -0,0 +1,42 @@
+<?php
+declare(strict_types=1);
+
+namespace DR\Review\Tests\Unit\Service\Api\Gitlab;
+
+use DR\Review\Controller\App\Review\ReviewController;
+use DR\Review\Entity\Review\CodeReview;
+use DR\Review\Entity\Review\Comment;
+use DR\Review\Service\Api\Gitlab\GitlabCommentFormatter;
+use DR\Review\Tests\AbstractTestCase;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\MockObject\MockObject;
+use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
+
+#[CoversClass(GitlabCommentFormatter::class)]
+class GitlabCommentFormatterTest extends AbstractTestCase
+{
+    private UrlGeneratorInterface&MockObject $urlGenerator;
+    private GitlabCommentFormatter           $formatter;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        $this->urlGenerator = $this->createMock(UrlGeneratorInterface::class);
+        $this->formatter    = new GitlabCommentFormatter('https://example.com', $this->urlGenerator);
+    }
+
+    public function testFormat(): void
+    {
+        $review  = new CodeReview();
+        $comment = new Comment();
+        $comment->setMessage("foo\nbar");
+        $comment->setReview($review);
+
+        $this->urlGenerator->expects(self::once())
+            ->method('generate')
+            ->with(ReviewController::class, ['review' => $review])
+            ->willReturn('/path/to/review');
+
+        static::assertSame("foo\n<br>bar\n<br>\n<br>*123view: https://example.com/path/to/review*", $this->formatter->format($comment));
+    }
+}