From 91615aef541b302c65ce4e513d58c2a49b63f19e Mon Sep 17 00:00:00 2001
From: Ben McIlwain <mcilwain@google.com>
Date: Fri, 19 Apr 2024 13:59:58 -0400
Subject: [PATCH] Handle bad header names in registrar sheet syncing action
 (#2404)

The existing behavior was to ignore bad header names, in a way that was
counter-intuitive as a user of the Google Sheet. If a header name was bad (which
could just be someone accidentally changing it not realizing it needs to
correspond exactly to the name of the field on the Java object), then all of the
data in that column was just silently left as-is and never updated. This led to
gradually worsening sync and offset shift errors over time.

Now, it will write out an error message into every single cell in the bad
column, so it's clear that the column name is wrong and does not correspond to any
actual data in the DB.

BUG=http://b/332336068
---
 .../registry/export/sheet/SheetSynchronizer.java     | 12 ++++++++----
 .../registry/export/sheet/SheetSynchronizerTest.java |  8 ++++----
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/core/src/main/java/google/registry/export/sheet/SheetSynchronizer.java b/core/src/main/java/google/registry/export/sheet/SheetSynchronizer.java
index 8b6ec15b1..c1a46eca7 100644
--- a/core/src/main/java/google/registry/export/sheet/SheetSynchronizer.java
+++ b/core/src/main/java/google/registry/export/sheet/SheetSynchronizer.java
@@ -38,6 +38,8 @@ class SheetSynchronizer {
 
   private static final String SHEET_NAME = "Registrars";
 
+  private static final String INVALID_HEADER_TEXT = "Invalid Sheet column header name";
+
   @Inject Sheets sheetsService;
   @Inject SheetSynchronizer() {}
 
@@ -92,10 +94,12 @@ class SheetSynchronizer {
         cellRow.add("");
       }
       for (int j = 0; j < headers.size(); j++) {
-        // Look for the value corresponding to the row and header indices in data
-        String dataField = data.get(i).get(headers.get(j));
-        // If the cell's header matches a data header, and the values aren't equal, mutate it
-        if (dataField != null && !cellRow.get(j).toString().equals(dataField)) {
+        // Look for the value corresponding to the row and header indices in data, otherwise write
+        // out an error so it's clear to a reader of the Google Sheet that the column header is
+        // invalid.
+        String dataField = data.get(i).getOrDefault(headers.get(j), INVALID_HEADER_TEXT);
+        // If the existing cell value is different than the new data, mutate it.
+        if (!cellRow.get(j).toString().equals(dataField)) {
           mutated = true;
           originalVals.get(i).set(j, dataField);
         }
diff --git a/core/src/test/java/google/registry/export/sheet/SheetSynchronizerTest.java b/core/src/test/java/google/registry/export/sheet/SheetSynchronizerTest.java
index 7b20c6e0b..16a05a4e8 100644
--- a/core/src/test/java/google/registry/export/sheet/SheetSynchronizerTest.java
+++ b/core/src/test/java/google/registry/export/sheet/SheetSynchronizerTest.java
@@ -114,7 +114,7 @@ class SheetSynchronizerTest {
   }
 
   @Test
-  void testSynchronize_unknownFields_doesntUpdate() throws Exception {
+  void testSynchronize_unknownFields_writesColumnHeaderErrorIntoCell() throws Exception {
     existingSheet.add(createRow("a", "c", "b"));
     existingSheet.add(createRow("diffVal1", "sameVal", "diffVal2"));
     data = ImmutableList.of(ImmutableMap.of("a", "val1", "b", "val2", "d", "val3"));
@@ -125,7 +125,7 @@ class SheetSynchronizerTest {
 
     BatchUpdateValuesRequest expectedRequest = new BatchUpdateValuesRequest();
     List<List<Object>> expectedVals = newArrayList();
-    expectedVals.add(createRow("val1", "sameVal", "val2"));
+    expectedVals.add(createRow("val1", "Invalid Sheet column header name", "val2"));
     expectedRequest.setData(
         newArrayList(new ValueRange().setRange("Registrars!A2").setValues(expectedVals)));
     expectedRequest.setValueInputOption("RAW");
@@ -133,7 +133,7 @@ class SheetSynchronizerTest {
   }
 
   @Test
-  void testSynchronize_notFullRow_getsPadded() throws Exception {
+  void testSynchronize_notFullRow_isHandledCorrectly() throws Exception {
     existingSheet.add(createRow("a", "c", "b"));
     existingSheet.add(createRow("diffVal1", "diffVal2"));
     data = ImmutableList.of(ImmutableMap.of("a", "val1", "b", "paddedVal", "d", "val3"));
@@ -144,7 +144,7 @@ class SheetSynchronizerTest {
 
     BatchUpdateValuesRequest expectedRequest = new BatchUpdateValuesRequest();
     List<List<Object>> expectedVals = newArrayList();
-    expectedVals.add(createRow("val1", "diffVal2", "paddedVal"));
+    expectedVals.add(createRow("val1", "Invalid Sheet column header name", "paddedVal"));
     expectedRequest.setData(
         newArrayList(new ValueRange().setRange("Registrars!A2").setValues(expectedVals)));
     expectedRequest.setValueInputOption("RAW");