diff --git a/docs/code-structure.md b/docs/code-structure.md index 1e01bb766..8d6239d0f 100644 --- a/docs/code-structure.md +++ b/docs/code-structure.md @@ -74,6 +74,8 @@ The following cursor types are defined: escrow provider's SFTP server for a given TLD. * **`RECURRING_BILLING`** - Expansion of `Recurring` (renew) billing events into `OneTime` events. +* **`SYNC_REGISTRAR_SHEET`** - Tracks the last time the registrar spreadsheet + was successfully synced. All `Cursor` entities in Datastore contain a `DateTime` that represents the next timestamp at which an operation should resume processing and a `CursorType` that diff --git a/java/google/registry/config/ConfigModule.java b/java/google/registry/config/ConfigModule.java index 79045208c..a073bd295 100644 --- a/java/google/registry/config/ConfigModule.java +++ b/java/google/registry/config/ConfigModule.java @@ -618,17 +618,6 @@ public final class ConfigModule { } } - /** - * Amount of time between synchronizations of the Registrar spreadsheet. - * - * @see google.registry.export.sheet.SyncRegistrarsSheetAction - */ - @Provides - @Config("sheetRegistrarInterval") - public static Duration provideSheetRegistrarInterval() { - return Duration.standardHours(1); - } - /** * Returns SSH client connection and read timeout. * diff --git a/java/google/registry/export/sheet/BUILD b/java/google/registry/export/sheet/BUILD index 671504f2c..341ae3d85 100644 --- a/java/google/registry/export/sheet/BUILD +++ b/java/google/registry/export/sheet/BUILD @@ -21,6 +21,7 @@ java_library( "//third_party/java/joda_time", "//third_party/java/jsr305_annotations", "//third_party/java/jsr330_inject", + "//third_party/java/objectify:objectify-v4_1", "//third_party/java/servlet/servlet_api", "//java/google/registry/config", "//java/google/registry/model", diff --git a/java/google/registry/export/sheet/SyncRegistrarsSheet.java b/java/google/registry/export/sheet/SyncRegistrarsSheet.java index 23aa7dc87..14e88babb 100644 --- a/java/google/registry/export/sheet/SyncRegistrarsSheet.java +++ b/java/google/registry/export/sheet/SyncRegistrarsSheet.java @@ -15,6 +15,8 @@ package google.registry.export.sheet; import static com.google.common.base.MoreObjects.firstNonNull; +import static google.registry.model.common.Cursor.CursorType.SYNC_REGISTRAR_SHEET; +import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.model.registrar.RegistrarContact.Type.ABUSE; import static google.registry.model.registrar.RegistrarContact.Type.ADMIN; import static google.registry.model.registrar.RegistrarContact.Type.BILLING; @@ -22,6 +24,7 @@ import static google.registry.model.registrar.RegistrarContact.Type.LEGAL; import static google.registry.model.registrar.RegistrarContact.Type.MARKETING; import static google.registry.model.registrar.RegistrarContact.Type.TECH; import static google.registry.model.registrar.RegistrarContact.Type.WHOIS; +import static google.registry.util.DateTimeUtils.START_OF_TIME; import com.google.common.base.Function; import com.google.common.base.Joiner; @@ -32,6 +35,8 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Ordering; import com.google.gdata.util.ServiceException; +import com.googlecode.objectify.VoidWork; +import google.registry.model.common.Cursor; import google.registry.model.registrar.Registrar; import google.registry.model.registrar.RegistrarAddress; import google.registry.model.registrar.RegistrarContact; @@ -41,10 +46,9 @@ import java.io.IOException; import javax.annotation.Nullable; import javax.inject.Inject; import org.joda.time.DateTime; -import org.joda.time.Duration; /** - * Class for synchronizing all {@link Registrar} datastore objects to a Google Spreadsheet. + * Class for synchronizing all {@link Registrar} Datastore objects to a Google Spreadsheet. * * @see SyncRegistrarsSheetAction */ @@ -54,11 +58,15 @@ class SyncRegistrarsSheet { @Inject SheetSynchronizer sheetSynchronizer; @Inject SyncRegistrarsSheet() {} - /** Returns true if a {@link Registrar} entity was modified in past {@code duration}. */ - boolean wasRegistrarsModifiedInLast(Duration duration) { - DateTime watermark = clock.nowUtc().minus(duration); + /** + * Returns true if any {@link Registrar} entity was modified since the last time this task + * successfully completed, as measured by a cursor. + */ + boolean wereRegistrarsModified() { + Cursor cursor = ofy().load().key(Cursor.createGlobalKey(SYNC_REGISTRAR_SHEET)).now(); + DateTime lastUpdateTime = (cursor == null) ? START_OF_TIME : cursor.getCursorTime(); for (Registrar registrar : Registrar.loadAll()) { - if (DateTimeUtils.isAtOrAfter(registrar.getLastUpdateTime(), watermark)) { + if (DateTimeUtils.isAtOrAfter(registrar.getLastUpdateTime(), lastUpdateTime)) { return true; } } @@ -67,6 +75,7 @@ class SyncRegistrarsSheet { /** Performs the synchronization operation. */ void run(String spreadsheetId) throws IOException, ServiceException { + final DateTime executionTime = clock.nowUtc(); sheetSynchronizer.synchronize( spreadsheetId, FluentIterable @@ -167,6 +176,11 @@ class SyncRegistrarsSheet { } }) .toList()); + ofy().transact(new VoidWork() { + @Override + public void vrun() { + ofy().save().entity(Cursor.createGlobal(SYNC_REGISTRAR_SHEET, executionTime)); + }}); } private static String convertContacts( diff --git a/java/google/registry/export/sheet/SyncRegistrarsSheetAction.java b/java/google/registry/export/sheet/SyncRegistrarsSheetAction.java index 01acd655c..842d72caa 100644 --- a/java/google/registry/export/sheet/SyncRegistrarsSheetAction.java +++ b/java/google/registry/export/sheet/SyncRegistrarsSheetAction.java @@ -111,7 +111,6 @@ public class SyncRegistrarsSheetAction implements Runnable { @Inject SyncRegistrarsSheet syncRegistrarsSheet; @Inject @Config("sheetLockTimeout") Duration timeout; @Inject @Config("sheetRegistrarId") Optional idConfig; - @Inject @Config("sheetRegistrarInterval") Duration interval; @Inject @Parameter("id") Optional idParam; @Inject SyncRegistrarsSheetAction() {} @@ -123,8 +122,7 @@ public class SyncRegistrarsSheetAction implements Runnable { return; } if (!idParam.isPresent()) { - // TODO(b/19082368): Use a cursor. - if (!syncRegistrarsSheet.wasRegistrarsModifiedInLast(interval)) { + if (!syncRegistrarsSheet.wereRegistrarsModified()) { Result.NOTMODIFIED.send(response, null); return; } diff --git a/java/google/registry/model/common/Cursor.java b/java/google/registry/model/common/Cursor.java index 26ead4785..ee678b25a 100644 --- a/java/google/registry/model/common/Cursor.java +++ b/java/google/registry/model/common/Cursor.java @@ -71,7 +71,14 @@ public class Cursor extends ImmutableObject { * for which Recurring billing events have been expanded (i.e. the inclusive first billing time * for the next expansion job). */ - RECURRING_BILLING(EntityGroupRoot.class); + RECURRING_BILLING(EntityGroupRoot.class), + + /** + * Cursor for {@link google.registry.export.sheet.SyncRegistrarsSheetAction}. The DateTime + * stored is the last time that registrar changes were successfully synced to the sheet. If + * there were no changes since the last time the action run, the cursor is not updated. + */ + SYNC_REGISTRAR_SHEET(EntityGroupRoot.class); /** See the definition of scope on {@link #getScopeClass}. */ private final Class scope; diff --git a/javatests/google/registry/export/sheet/BUILD b/javatests/google/registry/export/sheet/BUILD index eb9056d5c..6c7024c43 100644 --- a/javatests/google/registry/export/sheet/BUILD +++ b/javatests/google/registry/export/sheet/BUILD @@ -18,6 +18,7 @@ java_library( "//third_party/java/jsr305_annotations", "//third_party/java/junit", "//third_party/java/mockito", + "//third_party/java/objectify:objectify-v4_1", "//third_party/java/servlet/servlet_api", "//third_party/java/truth", "//java/google/registry/config", diff --git a/javatests/google/registry/export/sheet/SyncRegistrarsSheetActionTest.java b/javatests/google/registry/export/sheet/SyncRegistrarsSheetActionTest.java index 1a5258bc7..b09e67788 100644 --- a/javatests/google/registry/export/sheet/SyncRegistrarsSheetActionTest.java +++ b/javatests/google/registry/export/sheet/SyncRegistrarsSheetActionTest.java @@ -16,7 +16,6 @@ package google.registry.export.sheet; import static com.google.common.net.MediaType.PLAIN_TEXT_UTF_8; import static com.google.common.truth.Truth.assertThat; -import static org.mockito.Matchers.any; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -55,7 +54,6 @@ public class SyncRegistrarsSheetActionTest { action.syncRegistrarsSheet = syncRegistrarsSheet; action.idConfig = Optional.fromNullable(idConfig); action.idParam = Optional.fromNullable(idParam); - action.interval = Duration.standardHours(1); action.timeout = Duration.standardHours(1); action.run(); } @@ -69,22 +67,22 @@ public class SyncRegistrarsSheetActionTest { @Test public void testPost_withoutParams_runsSyncWithDefaultIdAndChecksIfModified() throws Exception { - when(syncRegistrarsSheet.wasRegistrarsModifiedInLast(any(Duration.class))).thenReturn(true); + when(syncRegistrarsSheet.wereRegistrarsModified()).thenReturn(true); runAction("jazz", null); assertThat(response.getStatus()).isEqualTo(200); assertThat(response.getContentType()).isEqualTo(PLAIN_TEXT_UTF_8); assertThat(response.getPayload()).startsWith("OK"); - verify(syncRegistrarsSheet).wasRegistrarsModifiedInLast(any(Duration.class)); + verify(syncRegistrarsSheet).wereRegistrarsModified(); verify(syncRegistrarsSheet).run(eq("jazz")); verifyNoMoreInteractions(syncRegistrarsSheet); } @Test public void testPost_noModificationsToRegistrarEntities_doesNothing() throws Exception { - when(syncRegistrarsSheet.wasRegistrarsModifiedInLast(any(Duration.class))).thenReturn(false); + when(syncRegistrarsSheet.wereRegistrarsModified()).thenReturn(false); runAction("NewRegistrar", null); assertThat(response.getPayload()).startsWith("NOTMODIFIED"); - verify(syncRegistrarsSheet).wasRegistrarsModifiedInLast(any(Duration.class)); + verify(syncRegistrarsSheet).wereRegistrarsModified(); verifyNoMoreInteractions(syncRegistrarsSheet); } diff --git a/javatests/google/registry/export/sheet/SyncRegistrarsSheetTest.java b/javatests/google/registry/export/sheet/SyncRegistrarsSheetTest.java index 384c8ea26..50b085205 100644 --- a/javatests/google/registry/export/sheet/SyncRegistrarsSheetTest.java +++ b/javatests/google/registry/export/sheet/SyncRegistrarsSheetTest.java @@ -16,12 +16,14 @@ package google.registry.export.sheet; import static com.google.common.collect.Iterables.getOnlyElement; import static com.google.common.truth.Truth.assertThat; +import static google.registry.model.common.Cursor.CursorType.SYNC_REGISTRAR_SHEET; +import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.deleteResource; import static google.registry.testing.DatastoreHelper.persistResource; import static google.registry.testing.DatastoreHelper.persistSimpleResources; import static org.joda.time.DateTimeZone.UTC; -import static org.joda.time.Duration.standardHours; +import static org.joda.time.Duration.standardMinutes; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.verify; @@ -29,6 +31,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import google.registry.config.RegistryEnvironment; +import google.registry.model.common.Cursor; import google.registry.model.ofy.Ofy; import google.registry.model.registrar.Registrar; import google.registry.model.registrar.RegistrarAddress; @@ -37,7 +40,6 @@ import google.registry.testing.AppEngineRule; import google.registry.testing.FakeClock; import google.registry.testing.InjectRule; import org.joda.time.DateTime; -import org.joda.time.Duration; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -87,14 +89,12 @@ public class SyncRegistrarsSheetTest { } @Test - public void testWasRegistrarsModifiedInLast_noRegistrars_returnsFalse() throws Exception { - SyncRegistrarsSheet sync = newSyncRegistrarsSheet(); - assertThat(sync.wasRegistrarsModifiedInLast(Duration.standardHours(1))).isFalse(); + public void test_wereRegistrarsModified_noRegistrars_returnsFalse() throws Exception { + assertThat(newSyncRegistrarsSheet().wereRegistrarsModified()).isFalse(); } @Test - public void testWasRegistrarsModifiedInLastInterval() throws Exception { - Duration interval = standardHours(1); + public void test_wereRegistrarsModified_atDifferentCursorTimes() throws Exception { persistResource(new Registrar.Builder() .setClientId("SomeRegistrar") .setRegistrarName("Some Registrar Inc.") @@ -102,14 +102,15 @@ public class SyncRegistrarsSheetTest { .setIanaIdentifier(8L) .setState(Registrar.State.ACTIVE) .build()); - clock.advanceBy(interval); - assertThat(newSyncRegistrarsSheet().wasRegistrarsModifiedInLast(interval)).isTrue(); - clock.advanceOneMilli(); - assertThat(newSyncRegistrarsSheet().wasRegistrarsModifiedInLast(interval)).isFalse(); + persistResource(Cursor.createGlobal(SYNC_REGISTRAR_SHEET, clock.nowUtc().minusHours(1))); + assertThat(newSyncRegistrarsSheet().wereRegistrarsModified()).isTrue(); + persistResource(Cursor.createGlobal(SYNC_REGISTRAR_SHEET, clock.nowUtc().plusHours(1))); + assertThat(newSyncRegistrarsSheet().wereRegistrarsModified()).isFalse(); } @Test public void testRun() throws Exception { + DateTime beforeExecution = clock.nowUtc(); persistResource(new Registrar.Builder() .setClientId("anotherregistrar") .setRegistrarName("Another Registrar LLC") @@ -190,6 +191,7 @@ public class SyncRegistrarsSheetTest { persistSimpleResources(contacts); persistResource(registrar); + clock.advanceBy(standardMinutes(1)); newSyncRegistrarsSheet().run("foobar"); verify(sheetSynchronizer).synchronize(eq("foobar"), rowsCaptor.capture()); @@ -254,8 +256,8 @@ public class SyncRegistrarsSheetTest { assertThat(row).containsEntry("address.countryCode", "US"); assertThat(row).containsEntry("phoneNumber", "+1.2223334444"); assertThat(row).containsEntry("faxNumber", ""); - assertThat(row.get("creationTime")).isEqualTo(clock.nowUtc().toString()); - assertThat(row.get("lastUpdateTime")).isEqualTo(clock.nowUtc().toString()); + assertThat(row.get("creationTime")).isEqualTo(beforeExecution.toString()); + assertThat(row.get("lastUpdateTime")).isEqualTo(beforeExecution.toString()); assertThat(row).containsEntry("allowedTlds", "example"); assertThat(row).containsEntry("blockPremiumNames", "false"); assertThat(row).containsEntry("ipAddressWhitelist", ""); @@ -289,8 +291,8 @@ public class SyncRegistrarsSheetTest { assertThat(row).containsEntry("address.countryCode", "US"); assertThat(row).containsEntry("phoneNumber", "+1.2125551212"); assertThat(row).containsEntry("faxNumber", "+1.2125551213"); - assertThat(row.get("creationTime")).isEqualTo(clock.nowUtc().toString()); - assertThat(row.get("lastUpdateTime")).isEqualTo(clock.nowUtc().toString()); + assertThat(row.get("creationTime")).isEqualTo(beforeExecution.toString()); + assertThat(row.get("lastUpdateTime")).isEqualTo(beforeExecution.toString()); assertThat(row).containsEntry("allowedTlds", ""); assertThat(row).containsEntry("whoisServer", "whois.example.com"); assertThat(row).containsEntry("blockPremiumNames", "false"); @@ -299,6 +301,10 @@ public class SyncRegistrarsSheetTest { assertThat(row).containsEntry("referralUrl", ENVIRONMENT.config().getRegistrarDefaultReferralUrl().toString()); assertThat(row).containsEntry("icannReferralEmail", "jim@example.net"); + + Cursor cursor = ofy().load().key(Cursor.createGlobalKey(SYNC_REGISTRAR_SHEET)).now(); + assertThat(cursor).isNotNull(); + assertThat(cursor.getCursorTime()).isGreaterThan(beforeExecution); } @Test