From 4ef22d4ddb4d6b27746ce596b8e79a21f9a06fda Mon Sep 17 00:00:00 2001
From: Matan Lurey <matan@lurey.org>
Date: Tue, 13 May 2025 09:54:30 -0700
Subject: [PATCH] Do not add/update a commit if the SHA already exists.

---
 app_dart/lib/src/service/commit_service.dart  | 22 ++++++++++--
 .../test/service/commit_service_test.dart     | 36 ++++++++++++++-----
 2 files changed, 47 insertions(+), 11 deletions(-)

diff --git a/app_dart/lib/src/service/commit_service.dart b/app_dart/lib/src/service/commit_service.dart
index 0711822c8..c5f55c91d 100644
--- a/app_dart/lib/src/service/commit_service.dart
+++ b/app_dart/lib/src/service/commit_service.dart
@@ -4,6 +4,7 @@
 
 import 'dart:async';
 
+import 'package:cocoon_server/logging.dart';
 import 'package:github/github.dart' as gh;
 import 'package:github/hooks.dart' as gh;
 import 'package:googleapis/firestore/v1.dart' as fs;
@@ -44,23 +45,38 @@ interface class CommitService {
     );
 
     // Convert into the format the rest of the service uses.
-    await _insertFirestore(
+    await _insertFirestoreIfNewSha(
       _Commit.fromBranchEvent(
         repository: slug,
         branch: branch,
         commit: ghCommit,
         now: _now(),
       ),
+      debugOrigin: 'GitHub Branch Event',
     );
   }
 
   /// Add a commit based on a Push event to the Firestore.
   /// https://docs.github.com/en/webhooks/webhook-events-and-payloads#push
   Future<void> handlePushGithubRequest(Map<String, Object?> pushEvent) async {
-    await _insertFirestore(_Commit.fromPushEventJson(pushEvent));
+    await _insertFirestoreIfNewSha(
+      _Commit.fromPushEventJson(pushEvent),
+      debugOrigin: 'GitHub Push Event',
+    );
   }
 
-  Future<void> _insertFirestore(_Commit commit) async {
+  Future<void> _insertFirestoreIfNewSha(
+    _Commit commit, {
+    required String debugOrigin,
+  }) async {
+    if (await fs.Commit.tryFromFirestoreBySha(_firestore, sha: commit.sha) !=
+        null) {
+      log.info(
+        'Skipping commit ${commit.sha} (from $debugOrigin): already exists in '
+        'Firestore',
+      );
+      return;
+    }
     final fsCommit = fs.Commit(
       createTimestamp: commit.createdOn.millisecondsSinceEpoch,
       repositoryPath: commit.repository.fullName,
diff --git a/app_dart/test/service/commit_service_test.dart b/app_dart/test/service/commit_service_test.dart
index 49c50ef9d..b90e21c12 100644
--- a/app_dart/test/service/commit_service_test.dart
+++ b/app_dart/test/service/commit_service_test.dart
@@ -12,6 +12,7 @@ import 'package:test/test.dart';
 
 import '../src/fake_config.dart';
 import '../src/service/fake_firestore_service.dart';
+import '../src/utilities/entity_generators.dart';
 import '../src/utilities/mocks.mocks.dart';
 import '../src/utilities/webhook_generators.dart';
 
@@ -90,15 +91,23 @@ void main() {
       );
     });
 
-    test('does not add commit to db if it exists in Firestore', () async {
+    // Regression test for https://github.com/flutter/flutter/issues/168684.
+    test('does not update commit in db if SHA exists in Firestore', () async {
+      firestore.putDocument(
+        generateFirestoreCommit(1, branch: branch, sha: sha),
+      );
+
       when(
         githubService.getReference(
           RepositorySlug(owner, repository),
-          'heads/$branch',
+          'heads/$branch-different',
         ),
       ).thenAnswer((Invocation invocation) {
         return Future<GitReference>.value(
-          GitReference(ref: 'refs/$branch', object: GitObject('', sha, '')),
+          GitReference(
+            ref: 'refs/$branch-different',
+            object: GitObject('', sha, ''),
+          ),
         );
       });
 
@@ -119,12 +128,15 @@ void main() {
       });
 
       final createEvent = generateCreateBranchEvent(
-        branch,
+        '$branch-different',
         '$owner/$repository',
       );
       await commitService.handleCreateGithubRequest(createEvent);
 
-      expect(firestore, existsInStorage(fs.Commit.metadata, hasLength(1)));
+      expect(
+        firestore,
+        existsInStorage(fs.Commit.metadata, [isCommit.hasBranch(branch)]),
+      );
     });
   });
 
@@ -155,9 +167,14 @@ void main() {
       );
     });
 
-    test('does not add commit to db if it exists in Firestore', () async {
+    // Regression test for https://github.com/flutter/flutter/issues/168684.
+    test('does not update commit in db if SHA exists in Firestore', () async {
+      firestore.putDocument(
+        generateFirestoreCommit(1, branch: branch, sha: sha),
+      );
+
       final pushEvent = generatePushEvent(
-        branch,
+        '$branch-different',
         owner,
         repository,
         message: message,
@@ -167,7 +184,10 @@ void main() {
       );
       await commitService.handlePushGithubRequest(pushEvent);
 
-      expect(firestore, existsInStorage(fs.Commit.metadata, hasLength(1)));
+      expect(
+        firestore,
+        existsInStorage(fs.Commit.metadata, [isCommit.hasBranch(branch)]),
+      );
     });
   });
 }