commit b8a6dc030cdeb3779788bf256e0d07d0b21581b0
author William Hesse <whesse@google.com> Fri Nov 26 09:28:04 2021 +0000
committer commit-bot@chromium.org <commit-bot@chromium.org> Fri Nov 26 09:28:04 2021 +0000
tree 31567dea1384b4a30da79441948e93616dd3bd81
parent 7c85666576e6b9a116ad9240721ff17ee2c9e610

[builder] Existing patchset documents may be written again

When new patchsets are found on a Gerrit CL, all patchsets are written
to Firestore documents, overwriting the originals with identical data.
Trying to avoid this would lead to race conditions, so we make it safe.

Change-Id: I412ca40ee7f83838368f6d0085a30412cb867daf
Reviewed-on: https://dart-review.googlesource.com/c/dart_ci/+/221323
Reviewed-by: Alexander Thomas <athom@google.com>
Commit-Queue: William Hesse <whesse@google.com>

builder/lib/src/firestore.dart

diff --git a/builder/lib/src/firestore.dart b/builder/lib/src/firestore.dart
index 6c2c7ab..e5de970 100644
--- a/builder/lib/src/firestore.dart
+++ b/builder/lib/src/firestore.dart
@@ -551,16 +551,15 @@
   Future<void> storePatchset(String review, int patchset, String kind,
       String description, int patchsetGroup, int number) async {
     final document = Document()
+      ..name = '$documents/reviews/$review/patchsets/$patchset'
       ..fields = taggedMap({
         'kind': kind,
         'description': description,
         'patchset_group': patchsetGroup,
         'number': number
       });
-    await firestore.projects.databases.documents.createDocument(
-        document, '$documents/reviews/$review', 'patchsets',
-        documentId: '$patchset');
-    log('Stored patchset: $documents/reviews/$review/$patchset\n'
+    await _executeWrite([Write()..update = document]);
+    log('Stored patchset: $documents/reviews/$review/patchsets/$patchset\n'
         '${untagMap(document.fields)}');
     documentsWritten++;
   }

builder/test/tryjob_test.dart

diff --git a/builder/test/tryjob_test.dart b/builder/test/tryjob_test.dart
index 7843a2a..04291f6 100644
--- a/builder/test/tryjob_test.dart
+++ b/builder/test/tryjob_test.dart
@@ -290,4 +290,35 @@
         from: 'try_results', where: fieldEquals('name', 'truncated_2_test'));
     expect(truncatedResult, isEmpty);
   });
+
+  test('patchsets', () async {
+    final document = await firestore.getDocument(
+        '${firestore.documents}/reviews/${data['review']}/patchsets/${data['patchset']}');
+    final fields = untagMap(document.fields);
+    expect(fields['number'].toString(), data['patchset']);
+    await firestore.storePatchset(
+        data['review'],
+        fields['number'],
+        fields['kind'],
+        fields['description'],
+        fields['patchset_group'],
+        fields['number']);
+    final document1 = await firestore.getDocument(document.name);
+    expect(untagMap(document1.fields), equals(fields));
+    fields['number'] += 1;
+    fields['description'] = 'test description';
+    await firestore.storePatchset(
+        data['review'],
+        fields['number'],
+        fields['kind'],
+        fields['description'],
+        fields['patchset_group'],
+        fields['number']);
+    final name =
+        '${firestore.documents}/reviews/${data['review']}/patchsets/${fields['number']}';
+    final document2 = await firestore.getDocument(name);
+    final fields2 = untagMap(document2.fields);
+    expect(fields2, equals(fields));
+    await firestore.deleteDocument(name);
+  });
 }