From 29c38f3622c433318fd0b92b61c12e44d7c5f436 Mon Sep 17 00:00:00 2001 From: mcilwain Date: Tue, 6 Feb 2018 11:44:50 -0800 Subject: [PATCH] Remove leniency on poll message ID format without years in them It's been long enough since the format change adding in years that all registrars should no longer have any IDs in the old format lying around that they're still attempting to ACK. All poll messages have already been coming back to registrars with the new format for months now. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=184714735 --- .../registry/flows/poll/PollAckFlow.java | 11 +++-- .../poll/PollMessageExternalKeyConverter.java | 4 +- .../registry/flows/poll/PollAckFlowTest.java | 48 +++++++++++-------- .../flows/poll/testdata/poll_ack_response.xml | 2 +- .../PollMessageExternalKeyConverterTest.java | 17 ++----- 5 files changed, 40 insertions(+), 42 deletions(-) diff --git a/java/google/registry/flows/poll/PollAckFlow.java b/java/google/registry/flows/poll/PollAckFlow.java index 991d1990f..008629bd8 100644 --- a/java/google/registry/flows/poll/PollAckFlow.java +++ b/java/google/registry/flows/poll/PollAckFlow.java @@ -19,6 +19,7 @@ import static google.registry.flows.FlowUtils.validateClientIsLoggedIn; import static google.registry.flows.poll.PollFlowUtils.getPollMessagesQuery; import static google.registry.model.eppoutput.Result.Code.SUCCESS_WITH_NO_MESSAGES; import static google.registry.model.ofy.ObjectifyService.ofy; +import static google.registry.model.poll.PollMessageExternalKeyConverter.makePollMessageExternalId; import static google.registry.model.poll.PollMessageExternalKeyConverter.parsePollMessageExternalId; import static google.registry.util.DateTimeUtils.isBeforeOrAt; @@ -80,14 +81,14 @@ public class PollAckFlow implements TransactionalFlow { final DateTime now = ofy().getTransactionTime(); // Load the message to be acked. If a message is queued to be delivered in the future, we treat - // it as if it doesn't exist yet. + // it as if it doesn't exist yet. Same for if the message ID year isn't the same as the actual + // poll message's event time (that means they're passing in an old already-acked ID). PollMessage pollMessage = ofy().load().key(pollMessageKey).now(); - if (pollMessage == null || !isBeforeOrAt(pollMessage.getEventTime(), now)) { + if (pollMessage == null + || !isBeforeOrAt(pollMessage.getEventTime(), now) + || !makePollMessageExternalId(pollMessage).equals(messageId)) { throw new MessageDoesNotExistException(messageId); } - // TODO(b/68953444): Once the year field on the external poll message ID becomes mandatory, add - // a check that the value of the year field is correct, by checking that - // makePollMessageExternalId(pollMessage) equals messageId. // Make sure this client is authorized to ack this message. It could be that the message is // supposed to go to a different registrar. diff --git a/java/google/registry/model/poll/PollMessageExternalKeyConverter.java b/java/google/registry/model/poll/PollMessageExternalKeyConverter.java index b2b4ec3bd..ecba67bf1 100644 --- a/java/google/registry/model/poll/PollMessageExternalKeyConverter.java +++ b/java/google/registry/model/poll/PollMessageExternalKeyConverter.java @@ -85,11 +85,9 @@ public class PollMessageExternalKeyConverter { * * @throws PollMessageExternalKeyParseException if the external key has an invalid format. */ - // TODO(b/68953444): Make the year field mandatory once sufficient time has elapsed and backwards - // compatibility is no longer necessary. public static Key parsePollMessageExternalId(String externalKey) { List idComponents = Splitter.on('-').splitToList(externalKey); - if (idComponents.size() != 5 && idComponents.size() != 6) { + if (idComponents.size() != 6) { throw new PollMessageExternalKeyParseException(); } try { diff --git a/javatests/google/registry/flows/poll/PollAckFlowTest.java b/javatests/google/registry/flows/poll/PollAckFlowTest.java index ab30213ea..bb697e769 100644 --- a/javatests/google/registry/flows/poll/PollAckFlowTest.java +++ b/javatests/google/registry/flows/poll/PollAckFlowTest.java @@ -104,24 +104,8 @@ public class PollAckFlowTest extends FlowTestCase { runFlowAssertResponse(loadFile("poll_ack_response_empty.xml")); } - // TODO(b/68953444): Remove test when missing year field backwards compatibility no longer needed. @Test - public void testSuccess_contactPollMessage_withMissingYearField() throws Exception { - setEppInput("poll_ack.xml", ImmutableMap.of("MSGID", "2-2-ROID-4-3")); - persistResource( - new PollMessage.OneTime.Builder() - .setId(MESSAGE_ID) - .setClientId(getClientIdForFlow()) - .setEventTime(clock.nowUtc().minusDays(1)) - .setMsg("Some poll message.") - .setParent(createHistoryEntryForEppResource(contact)) - .build()); - assertTransactionalFlow(true); - runFlowAssertResponse(loadFile("poll_ack_response_empty.xml")); - } - - @Test - public void testSuccess_contactPollMessage_withIncorrectYearField() throws Exception { + public void testFailure_contactPollMessage_withIncorrectYearField() throws Exception { setEppInput("poll_ack.xml", ImmutableMap.of("MSGID", "2-2-ROID-4-3-1999")); persistResource( new PollMessage.OneTime.Builder() @@ -132,7 +116,7 @@ public class PollAckFlowTest extends FlowTestCase { .setParent(createHistoryEntryForEppResource(contact)) .build()); assertTransactionalFlow(true); - runFlowAssertResponse(loadFile("poll_ack_response_empty.xml")); + assertThrows(MessageDoesNotExistException.class, this::runFlow); } @Test @@ -144,6 +128,7 @@ public class PollAckFlowTest extends FlowTestCase { @Test public void testSuccess_recentActiveAutorenew() throws Exception { + setEppInput("poll_ack.xml", ImmutableMap.of("MSGID", "1-3-EXAMPLE-4-3-2010")); persistAutorenewPollMessage(clock.nowUtc().minusMonths(6), END_OF_TIME); assertTransactionalFlow(true); runFlowAssertResponse(loadFile("poll_ack_response_empty.xml")); @@ -151,6 +136,7 @@ public class PollAckFlowTest extends FlowTestCase { @Test public void testSuccess_oldActiveAutorenew() throws Exception { + setEppInput("poll_ack.xml", ImmutableMap.of("MSGID", "1-3-EXAMPLE-4-3-2009")); persistAutorenewPollMessage(clock.nowUtc().minusYears(2), END_OF_TIME); // Create three other messages to be queued for retrieval to get our count right, since the poll // ack response wants there to be 4 messages in the queue when the ack comes back. @@ -158,11 +144,15 @@ public class PollAckFlowTest extends FlowTestCase { persistOneTimePollMessage(MESSAGE_ID + i); } assertTransactionalFlow(true); - runFlowAssertResponse(loadFile("poll_ack_response.xml")); + runFlowAssertResponse( + loadFile( + "poll_ack_response.xml", + ImmutableMap.of("MSGID", "1-3-EXAMPLE-4-3-2009", "COUNT", "4"))); } @Test public void testSuccess_oldInactiveAutorenew() throws Exception { + setEppInput("poll_ack.xml", ImmutableMap.of("MSGID", "1-3-EXAMPLE-4-3-2010")); persistAutorenewPollMessage(clock.nowUtc().minusMonths(6), clock.nowUtc()); assertTransactionalFlow(true); runFlowAssertResponse(loadFile("poll_ack_response_empty.xml")); @@ -175,7 +165,10 @@ public class PollAckFlowTest extends FlowTestCase { persistOneTimePollMessage(MESSAGE_ID + i); } assertTransactionalFlow(true); - runFlowAssertResponse(loadFile("poll_ack_response.xml")); + runFlowAssertResponse( + loadFile( + "poll_ack_response.xml", + ImmutableMap.of("MSGID", "1-3-EXAMPLE-4-3-2011", "COUNT", "4"))); } @Test @@ -201,6 +194,21 @@ public class PollAckFlowTest extends FlowTestCase { assertThrows(InvalidMessageIdException.class, this::runFlow); } + @Test + public void testFailure_contactPollMessage_withMissingYearField() throws Exception { + setEppInput("poll_ack.xml", ImmutableMap.of("MSGID", "2-2-ROID-4-3")); + persistResource( + new PollMessage.OneTime.Builder() + .setId(MESSAGE_ID) + .setClientId(getClientIdForFlow()) + .setEventTime(clock.nowUtc().minusDays(1)) + .setMsg("Some poll message.") + .setParent(createHistoryEntryForEppResource(contact)) + .build()); + assertTransactionalFlow(true); + assertThrows(InvalidMessageIdException.class, this::runFlow); + } + @Test public void testFailure_invalidId_stringInsteadOfNumeric() throws Exception { setEppInput("poll_ack.xml", ImmutableMap.of("MSGID", "ABC-12345")); diff --git a/javatests/google/registry/flows/poll/testdata/poll_ack_response.xml b/javatests/google/registry/flows/poll/testdata/poll_ack_response.xml index 69183a40a..9aa2a562f 100644 --- a/javatests/google/registry/flows/poll/testdata/poll_ack_response.xml +++ b/javatests/google/registry/flows/poll/testdata/poll_ack_response.xml @@ -3,7 +3,7 @@ Command completed successfully - + ABC-12346 server-trid diff --git a/javatests/google/registry/model/poll/PollMessageExternalKeyConverterTest.java b/javatests/google/registry/model/poll/PollMessageExternalKeyConverterTest.java index 2c07c755f..6d254ad2d 100644 --- a/javatests/google/registry/model/poll/PollMessageExternalKeyConverterTest.java +++ b/javatests/google/registry/model/poll/PollMessageExternalKeyConverterTest.java @@ -122,19 +122,10 @@ public class PollMessageExternalKeyConverterTest { } @Test - // TODO(b/68953444): Remove leniency when backwards compatibility with missing field is no longer - // required. - public void testSuccess_stillParsesWithMissingYearField() { - PollMessage.OneTime pollMessage = - persistResource( - new PollMessage.OneTime.Builder() - .setClientId("TheRegistrar") - .setEventTime(clock.nowUtc()) - .setMsg("Test poll message") - .setParent(historyEntry) - .build()); - assertThat(makePollMessageExternalId(pollMessage)).isEqualTo("1-2-FOOBAR-4-5-2007"); - assertThat(parsePollMessageExternalId("1-2-FOOBAR-4-5")).isEqualTo(Key.create(pollMessage)); + public void testFailure_missingYearField() { + assertThrows( + PollMessageExternalKeyParseException.class, + () -> parsePollMessageExternalId("1-2-FOOBAR-4-5")); } @Test