zydronium/google-nomulus
1
0
Fork 0
mirror of https://github.com/google/nomulus.git synced 2025-05-13 16:07:15 +02:00
Code Issues Projects Releases Packages Wiki Activity

Fix off-by-one issue in SheetSynchronizer

Browse source 
This doesn't change the end result of a successful run, though this is what a typical flow looks like prior to this fix:

Consider a sheet with 10 data rows (+ 1 header row = 11). A 10-row data set will call worksheet.setRowCount(10), which truncates the last row of the existing sheet. This row will eventually be added again in the last for loop, but if the synchronizer fails mid-sync, this last row will remain dropped. This fix will prevent this last row from being dropped.

This doesn't fix the broader issue of SheetSynchronizer not behaving transactionally -- that's a different can of worms.

See the linked bug for an instance where the synchronizer failed mid-run and dropped a data row as a result.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=136398109
This commit is contained in:
ctingue 2016-10-17 14:44:12 -07:00 • committed by Ben McIlwain
parent 361a53a3c9
commit 1f5a8d5542
2 changed files with 6 additions and 6 deletions
Download patch file Download diff file Expand all files Collapse all files

2
java/google/registry/export/sheet/SheetSynchronizer.java
View file

@ -71,7 +71,7 @@ class SheetSynchronizer {
URL url = new URL(SPREADSHEET_URL_PREFIX + spreadsheetId); URL url = new URL(SPREADSHEET_URL_PREFIX + spreadsheetId);
SpreadsheetEntry spreadsheet = spreadsheetService.getEntry(url, SpreadsheetEntry.class); SpreadsheetEntry spreadsheet = spreadsheetService.getEntry(url, SpreadsheetEntry.class);
WorksheetEntry worksheet = spreadsheet.getWorksheets().get(0); WorksheetEntry worksheet = spreadsheet.getWorksheets().get(0);
worksheet.setRowCount(data.size()); worksheet.setRowCount(data.size() + 1); // account for header row
worksheet = worksheet.update(); worksheet = worksheet.update();
ListFeed listFeed = spreadsheetService.getFeed(worksheet.getListFeedUrl(), ListFeed.class); ListFeed listFeed = spreadsheetService.getFeed(worksheet.getListFeedUrl(), ListFeed.class);
List<ListEntry> entries = listFeed.getEntries(); List<ListEntry> entries = listFeed.getEntries();

10
javatests/google/registry/export/sheet/SheetSynchronizerTest.java
View file

@ -78,7 +78,7 @@ public class SheetSynchronizerTest {
public void testSynchronize_bothEmpty_doNothing() throws Exception { public void testSynchronize_bothEmpty_doNothing() throws Exception {
when(listFeed.getEntries()).thenReturn(ImmutableList.<ListEntry>of()); when(listFeed.getEntries()).thenReturn(ImmutableList.<ListEntry>of());
sheetSynchronizer.synchronize("foobar", ImmutableList.<ImmutableMap<String, String>>of()); sheetSynchronizer.synchronize("foobar", ImmutableList.<ImmutableMap<String, String>>of());
verify(worksheet).setRowCount(0); verify(worksheet).setRowCount(1);
verify(worksheet).update(); verify(worksheet).update();
} }
@ -88,7 +88,7 @@ public class SheetSynchronizerTest {
when(listFeed.getEntries()).thenReturn(ImmutableList.of(entry)); when(listFeed.getEntries()).thenReturn(ImmutableList.of(entry));
sheetSynchronizer.synchronize("foobar", ImmutableList.of( sheetSynchronizer.synchronize("foobar", ImmutableList.of(
ImmutableMap.of("key", "value"))); ImmutableMap.of("key", "value")));
verify(worksheet).setRowCount(1); verify(worksheet).setRowCount(2);
verify(worksheet).update(); verify(worksheet).update();
verify(entry, atLeastOnce()).getCustomElements(); verify(entry, atLeastOnce()).getCustomElements();
verifyNoMoreInteractions(entry); verifyNoMoreInteractions(entry);
@ -102,7 +102,7 @@ public class SheetSynchronizerTest {
ImmutableMap.of("key", "new value"))); ImmutableMap.of("key", "new value")));
verify(entry.getCustomElements()).setValueLocal("key", "new value"); verify(entry.getCustomElements()).setValueLocal("key", "new value");
verify(entry).update(); verify(entry).update();
verify(worksheet).setRowCount(1); verify(worksheet).setRowCount(2);
verify(worksheet).update(); verify(worksheet).update();
verify(entry, atLeastOnce()).getCustomElements(); verify(entry, atLeastOnce()).getCustomElements();
verifyNoMoreInteractions(entry); verifyNoMoreInteractions(entry);
@ -117,7 +117,7 @@ public class SheetSynchronizerTest {
ImmutableMap.of("key", "value"))); ImmutableMap.of("key", "value")));
verify(entry.getCustomElements()).setValueLocal("key", "value"); verify(entry.getCustomElements()).setValueLocal("key", "value");
verify(listFeed).insert(entry); verify(listFeed).insert(entry);
verify(worksheet).setRowCount(1); verify(worksheet).setRowCount(2);
verify(worksheet).update(); verify(worksheet).update();
verify(listFeed).createEntry(); verify(listFeed).createEntry();
verify(entry, atLeastOnce()).getCustomElements(); verify(entry, atLeastOnce()).getCustomElements();
@ -129,7 +129,7 @@ public class SheetSynchronizerTest {
ListEntry entry = makeListEntry(ImmutableMap.of("key", "value")); ListEntry entry = makeListEntry(ImmutableMap.of("key", "value"));
when(listFeed.getEntries()).thenReturn(ImmutableList.of(entry)); when(listFeed.getEntries()).thenReturn(ImmutableList.of(entry));
sheetSynchronizer.synchronize("foobar", ImmutableList.<ImmutableMap<String, String>>of()); sheetSynchronizer.synchronize("foobar", ImmutableList.<ImmutableMap<String, String>>of());
verify(worksheet).setRowCount(0); verify(worksheet).setRowCount(1);
verify(worksheet).update(); verify(worksheet).update();
verifyNoMoreInteractions(entry); verifyNoMoreInteractions(entry);
} }