From 1adcd87de140424d83c869fb7cc794d82578c78f Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Thu, 15 Apr 2021 17:03:25 -0400 Subject: [PATCH] Fix bug that was incorrectly assuming Cursor would always exist (#1088) * Fix bug that was incorrectly assuming Cursor would always exist In fact, the Cursor entity does not always exist (i.e. if an upload has never previously been done on this TLD, i.e. it's a new TLD), and the code needs to be resilient to its non-existence. This bug was introduced in #1044. --- .../google/registry/model/common/Cursor.java | 5 +++-- .../google/registry/rde/RdeReportAction.java | 6 ++++-- .../google/registry/rde/RdeStagingReducer.java | 8 ++++++-- .../google/registry/rde/RdeUploadAction.java | 4 +++- .../icann/IcannReportingUploadAction.java | 5 ++--- .../registry/rde/RdeReportActionTest.java | 14 ++++++++++++++ .../registry/rde/RdeUploadActionTest.java | 17 +++++++++++++++++ 7 files changed, 49 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/google/registry/model/common/Cursor.java b/core/src/main/java/google/registry/model/common/Cursor.java index c173033b6..13c9b20d5 100644 --- a/core/src/main/java/google/registry/model/common/Cursor.java +++ b/core/src/main/java/google/registry/model/common/Cursor.java @@ -36,6 +36,7 @@ import google.registry.persistence.VKey; import google.registry.schema.replay.DatastoreAndSqlEntity; import java.io.Serializable; import java.util.List; +import java.util.Optional; import javax.persistence.Column; import javax.persistence.EnumType; import javax.persistence.Enumerated; @@ -280,8 +281,8 @@ public class Cursor extends ImmutableObject implements DatastoreAndSqlEntity { /** * Returns the current time for a given cursor, or {@code START_OF_TIME} if the cursor is null. */ - public static DateTime getCursorTimeOrStartOfTime(Cursor cursor) { - return cursor != null ? cursor.getCursorTime() : START_OF_TIME; + public static DateTime getCursorTimeOrStartOfTime(Optional cursor) { + return cursor.map(Cursor::getCursorTime).orElse(START_OF_TIME); } public DateTime getCursorTime() { diff --git a/core/src/main/java/google/registry/rde/RdeReportAction.java b/core/src/main/java/google/registry/rde/RdeReportAction.java index 0f017bd26..2bf89ff6b 100644 --- a/core/src/main/java/google/registry/rde/RdeReportAction.java +++ b/core/src/main/java/google/registry/rde/RdeReportAction.java @@ -41,6 +41,7 @@ import google.registry.request.Response; import google.registry.request.auth.Auth; import java.io.IOException; import java.io.InputStream; +import java.util.Optional; import javax.inject.Inject; import org.bouncycastle.openpgp.PGPPrivateKey; import org.joda.time.DateTime; @@ -76,8 +77,9 @@ public final class RdeReportAction implements Runnable, EscrowTask { @Override public void runWithLock(DateTime watermark) throws Exception { - Cursor cursor = - transactIfJpaTm(() -> tm().loadByKey(Cursor.createVKey(CursorType.RDE_UPLOAD, tld))); + Optional cursor = + transactIfJpaTm( + () -> tm().loadByKeyIfPresent(Cursor.createVKey(CursorType.RDE_UPLOAD, tld))); DateTime cursorTime = getCursorTimeOrStartOfTime(cursor); if (isBeforeOrAt(cursorTime, watermark)) { throw new NoContentException( diff --git a/core/src/main/java/google/registry/rde/RdeStagingReducer.java b/core/src/main/java/google/registry/rde/RdeStagingReducer.java index 9cd22b356..a6c0db649 100644 --- a/core/src/main/java/google/registry/rde/RdeStagingReducer.java +++ b/core/src/main/java/google/registry/rde/RdeStagingReducer.java @@ -20,8 +20,8 @@ import static com.google.appengine.tools.cloudstorage.GcsServiceFactory.createGc import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Verify.verify; import static google.registry.model.common.Cursor.getCursorTimeOrStartOfTime; -import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; +import static google.registry.persistence.transaction.TransactionManagerUtil.transactIfJpaTm; import static java.nio.charset.StandardCharsets.UTF_8; import com.google.appengine.tools.cloudstorage.GcsFilename; @@ -210,7 +210,11 @@ public final class RdeStagingReducer extends Reducer { Registry registry = Registry.get(tld); - Cursor cursor = ofy().load().key(Cursor.createKey(key.cursor(), registry)).now(); + Optional cursor = + transactIfJpaTm( + () -> + tm().loadByKeyIfPresent( + Cursor.createVKey(key.cursor(), registry.getTldStr()))); DateTime position = getCursorTimeOrStartOfTime(cursor); checkState(key.interval() != null, "Interval must be present"); DateTime newPosition = key.watermark().plus(key.interval()); diff --git a/core/src/main/java/google/registry/rde/RdeUploadAction.java b/core/src/main/java/google/registry/rde/RdeUploadAction.java index 923c1e5da..004017081 100644 --- a/core/src/main/java/google/registry/rde/RdeUploadAction.java +++ b/core/src/main/java/google/registry/rde/RdeUploadAction.java @@ -64,6 +64,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.net.URI; +import java.util.Optional; import javax.inject.Inject; import javax.inject.Named; import org.bouncycastle.openpgp.PGPKeyPair; @@ -133,7 +134,8 @@ public final class RdeUploadAction implements Runnable, EscrowTask { @Override public void runWithLock(final DateTime watermark) throws Exception { logger.atInfo().log("Verifying readiness to upload the RDE deposit."); - Cursor cursor = transactIfJpaTm(() -> tm().loadByKey(Cursor.createVKey(RDE_STAGING, tld))); + Optional cursor = + transactIfJpaTm(() -> tm().loadByKeyIfPresent(Cursor.createVKey(RDE_STAGING, tld))); DateTime stagingCursorTime = getCursorTimeOrStartOfTime(cursor); if (isBeforeOrAt(stagingCursorTime, watermark)) { throw new NoContentException( diff --git a/core/src/main/java/google/registry/reporting/icann/IcannReportingUploadAction.java b/core/src/main/java/google/registry/reporting/icann/IcannReportingUploadAction.java index b32f7710a..6860e19b1 100644 --- a/core/src/main/java/google/registry/reporting/icann/IcannReportingUploadAction.java +++ b/core/src/main/java/google/registry/reporting/icann/IcannReportingUploadAction.java @@ -16,7 +16,6 @@ package google.registry.reporting.icann; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.net.MediaType.PLAIN_TEXT_UTF_8; -import static google.registry.model.common.Cursor.getCursorTimeOrStartOfTime; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.request.Action.Method.POST; import static javax.servlet.http.HttpServletResponse.SC_OK; @@ -107,10 +106,10 @@ public final class IcannReportingUploadAction implements Runnable { // If cursor time is before now, upload the corresponding report cursors.entrySet().stream() - .filter(entry -> getCursorTimeOrStartOfTime(entry.getKey()).isBefore(clock.nowUtc())) + .filter(entry -> entry.getKey().getCursorTime().isBefore(clock.nowUtc())) .forEach( entry -> { - DateTime cursorTime = getCursorTimeOrStartOfTime(entry.getKey()); + DateTime cursorTime = entry.getKey().getCursorTime(); uploadReport( cursorTime, entry.getKey().getType(), diff --git a/core/src/test/java/google/registry/rde/RdeReportActionTest.java b/core/src/test/java/google/registry/rde/RdeReportActionTest.java index 9fd9cf410..37106ae83 100644 --- a/core/src/test/java/google/registry/rde/RdeReportActionTest.java +++ b/core/src/test/java/google/registry/rde/RdeReportActionTest.java @@ -19,6 +19,7 @@ import static com.google.common.net.MediaType.PLAIN_TEXT_UTF_8; import static com.google.common.truth.Truth.assertThat; import static google.registry.model.common.Cursor.CursorType.RDE_REPORT; import static google.registry.model.common.Cursor.CursorType.RDE_UPLOAD; +import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.testing.DatabaseHelper.createTld; import static google.registry.testing.DatabaseHelper.loadByKey; import static google.registry.testing.DatabaseHelper.persistResource; @@ -162,6 +163,19 @@ public class RdeReportActionTest { assertThat(report.getWatermark()).isEqualTo(DateTime.parse("2010-10-17T00:00:00Z")); } + @TestOfyAndSql + void testRunWithLock_nonexistentCursor_throws204() { + tm().transact(() -> tm().delete(Cursor.createVKey(RDE_UPLOAD, "test"))); + NoContentException thrown = + assertThrows( + NoContentException.class, () -> createAction().runWithLock(loadRdeReportCursor())); + assertThat(thrown) + .hasMessageThat() + .isEqualTo( + "Waiting on RdeUploadAction for TLD test to send 2006-06-06T00:00:00.000Z report; last" + + " upload completion was at 1970-01-01T00:00:00.000Z"); + } + @TestOfyAndSql void testRunWithLock_uploadNotFinished_throws204() { persistResource( diff --git a/core/src/test/java/google/registry/rde/RdeUploadActionTest.java b/core/src/test/java/google/registry/rde/RdeUploadActionTest.java index 85e0f9b32..1281ed920 100644 --- a/core/src/test/java/google/registry/rde/RdeUploadActionTest.java +++ b/core/src/test/java/google/registry/rde/RdeUploadActionTest.java @@ -318,6 +318,23 @@ public class RdeUploadActionTest { assertThat(stderr).contains("rde-unittest@registry.test"); } + @TestOfyAndSql + void testRunWithLock_nonexistentCursor_throws204() throws Exception { + int port = sftpd.serve("user", "password", folder); + URI uploadUrl = URI.create(String.format("sftp://user:password@localhost:%d/", port)); + DateTime uploadCursor = DateTime.parse("2010-10-17TZ"); + RdeUploadAction action = createAction(uploadUrl); + NoContentException thrown = + assertThrows(NoContentException.class, () -> action.runWithLock(uploadCursor)); + assertThat(thrown) + .hasMessageThat() + .isEqualTo( + "Waiting on RdeStagingAction for TLD tld to send 2010-10-17T00:00:00.000Z upload; last" + + " RDE staging completion was at 1970-01-01T00:00:00.000Z"); + assertNoTasksEnqueued("rde-upload"); + assertThat(folder.list()).isEmpty(); + } + @TestOfyAndSql void testRunWithLock_stagingNotFinished_throws204() { URI url = URI.create("sftp://user:password@localhost:32323/");