From d2f955a488b9bf82597ce6944f8ffbe39e240f22 Mon Sep 17 00:00:00 2001 From: gbrodman Date: Thu, 30 May 2019 11:36:23 -0700 Subject: [PATCH] Split ResourceAlreadyExistsException based on if this client owns the resource ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=250728711 --- docs/flows.md | 3 ++ .../registry/flows/ResourceFlowUtils.java | 17 +++++++--- .../flows/contact/ContactCreateFlow.java | 7 +++-- .../flows/domain/DomainCreateFlow.java | 22 ++++++++----- ...ceAlreadyExistsForThisClientException.java | 27 ++++++++++++++++ ...=> ResourceCreateContentionException.java} | 7 +++-- .../registry/flows/host/HostCreateFlow.java | 7 +++-- .../flows/contact/ContactCreateFlowTest.java | 31 ++++++++++++++++--- .../flows/domain/DomainCreateFlowTest.java | 25 +++++++++++++-- .../flows/host/HostCreateFlowTest.java | 25 +++++++++++++-- 10 files changed, 143 insertions(+), 28 deletions(-) create mode 100644 java/google/registry/flows/exceptions/ResourceAlreadyExistsForThisClientException.java rename java/google/registry/flows/exceptions/{ResourceAlreadyExistsException.java => ResourceCreateContentionException.java} (68%) diff --git a/docs/flows.md b/docs/flows.md index 547cc597f..cdb3ab958 100644 --- a/docs/flows.md +++ b/docs/flows.md @@ -27,6 +27,7 @@ An EPP flow that creates a new contact. * Internationalized postal infos can only contain ASCII characters. * 2302 * Resource with this id already exists. + * Resource with this id already exists. * 2306 * Declining contact disclosure is disallowed by server policy. @@ -341,6 +342,7 @@ An EPP flow that creates a new domain resource. * Registrar must be active in order to perform this operation. * 2302 * Resource with this id already exists. + * Resource with this id already exists. * 2303 * Resource linked to this domain does not exist. * 2304 @@ -823,6 +825,7 @@ allows creating a host name, and if necessary enqueues tasks to update DNS. * Host names must be puny-coded. * 2302 * Resource with this id already exists. + * Resource with this id already exists. * 2303 * Superordinate domain for this hostname does not exist. * 2304 diff --git a/java/google/registry/flows/ResourceFlowUtils.java b/java/google/registry/flows/ResourceFlowUtils.java index e11a82b25..9118f41da 100644 --- a/java/google/registry/flows/ResourceFlowUtils.java +++ b/java/google/registry/flows/ResourceFlowUtils.java @@ -31,7 +31,8 @@ import google.registry.flows.EppException.ParameterValueRangeErrorException; import google.registry.flows.exceptions.MissingTransferRequestAuthInfoException; import google.registry.flows.exceptions.NotPendingTransferException; import google.registry.flows.exceptions.NotTransferInitiatorException; -import google.registry.flows.exceptions.ResourceAlreadyExistsException; +import google.registry.flows.exceptions.ResourceAlreadyExistsForThisClientException; +import google.registry.flows.exceptions.ResourceCreateContentionException; import google.registry.flows.exceptions.ResourceStatusProhibitsOperationException; import google.registry.flows.exceptions.ResourceToDeleteIsReferencedException; import google.registry.flows.exceptions.TooManyResourceChecksException; @@ -45,6 +46,7 @@ import google.registry.model.eppcommon.StatusValue; import google.registry.model.index.ForeignKeyIndex; import google.registry.model.transfer.TransferStatus; import java.util.List; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.function.Function; @@ -131,9 +133,16 @@ public final class ResourceFlowUtils { } public static void verifyResourceDoesNotExist( - Class clazz, String targetId, DateTime now) throws EppException { - if (loadAndGetKey(clazz, targetId, now) != null) { - throw new ResourceAlreadyExistsException(targetId); + Class clazz, String targetId, DateTime now, String clientId) throws EppException { + Key key = loadAndGetKey(clazz, targetId, now); + if (key != null) { + R resource = ofy().load().key(key).now(); + // These are similar exceptions, but we can track them internally as log-based metrics. + if (Objects.equals(clientId, resource.getPersistedCurrentSponsorClientId())) { + throw new ResourceAlreadyExistsForThisClientException(targetId); + } else { + throw new ResourceCreateContentionException(targetId); + } } } diff --git a/java/google/registry/flows/contact/ContactCreateFlow.java b/java/google/registry/flows/contact/ContactCreateFlow.java index 0266db61e..08b4754de 100644 --- a/java/google/registry/flows/contact/ContactCreateFlow.java +++ b/java/google/registry/flows/contact/ContactCreateFlow.java @@ -29,6 +29,8 @@ import google.registry.flows.FlowModule.ClientId; import google.registry.flows.FlowModule.TargetId; import google.registry.flows.TransactionalFlow; import google.registry.flows.annotations.ReportingSpec; +import google.registry.flows.exceptions.ResourceAlreadyExistsForThisClientException; +import google.registry.flows.exceptions.ResourceCreateContentionException; import google.registry.model.contact.ContactCommand.Create; import google.registry.model.contact.ContactResource; import google.registry.model.domain.metadata.MetadataExtension; @@ -46,7 +48,8 @@ import org.joda.time.DateTime; /** * An EPP flow that creates a new contact. * - * @error {@link google.registry.flows.exceptions.ResourceAlreadyExistsException} + * @error {@link ResourceAlreadyExistsForThisClientException} + * @error {@link ResourceCreateContentionException} * @error {@link ContactFlowUtils.BadInternationalizedPostalInfoException} * @error {@link ContactFlowUtils.DeclineContactDisclosureFieldDisallowedPolicyException} */ @@ -69,7 +72,7 @@ public final class ContactCreateFlow implements TransactionalFlow { validateClientIsLoggedIn(clientId); Create command = (Create) resourceCommand; DateTime now = ofy().getTransactionTime(); - verifyResourceDoesNotExist(ContactResource.class, targetId, now); + verifyResourceDoesNotExist(ContactResource.class, targetId, now, clientId); ContactResource newContact = new ContactResource.Builder() .setContactId(targetId) diff --git a/java/google/registry/flows/domain/DomainCreateFlow.java b/java/google/registry/flows/domain/DomainCreateFlow.java index 089450f17..d39f1a27e 100644 --- a/java/google/registry/flows/domain/DomainCreateFlow.java +++ b/java/google/registry/flows/domain/DomainCreateFlow.java @@ -68,6 +68,8 @@ import google.registry.flows.custom.DomainCreateFlowCustomLogic.BeforeResponsePa import google.registry.flows.custom.DomainCreateFlowCustomLogic.BeforeResponseReturnData; import google.registry.flows.custom.EntityChanges; import google.registry.flows.domain.token.AllocationTokenFlowUtils; +import google.registry.flows.exceptions.ResourceAlreadyExistsForThisClientException; +import google.registry.flows.exceptions.ResourceCreateContentionException; import google.registry.model.ImmutableObject; import google.registry.model.billing.BillingEvent; import google.registry.model.billing.BillingEvent.Flag; @@ -114,13 +116,19 @@ import org.joda.time.Duration; /** * An EPP flow that creates a new domain resource. * - * @error {@link google.registry.flows.domain.token.AllocationTokenFlowUtils.AllocationTokenNotInPromotionException} - * @error {@link google.registry.flows.domain.token.AllocationTokenFlowUtils.AllocationTokenNotValidForRegistrarException} - * @error {@link google.registry.flows.domain.token.AllocationTokenFlowUtils.AllocationTokenNotValidForTldException} - * @error {@link google.registry.flows.domain.token.AllocationTokenFlowUtils.AlreadyRedeemedAllocationTokenException} - * @error {@link google.registry.flows.domain.token.AllocationTokenFlowUtils.InvalidAllocationTokenException} + * @error {@link + * google.registry.flows.domain.token.AllocationTokenFlowUtils.AllocationTokenNotInPromotionException} + * @error {@link + * google.registry.flows.domain.token.AllocationTokenFlowUtils.AllocationTokenNotValidForRegistrarException} + * @error {@link + * google.registry.flows.domain.token.AllocationTokenFlowUtils.AllocationTokenNotValidForTldException} + * @error {@link + * google.registry.flows.domain.token.AllocationTokenFlowUtils.AlreadyRedeemedAllocationTokenException} + * @error {@link + * google.registry.flows.domain.token.AllocationTokenFlowUtils.InvalidAllocationTokenException} * @error {@link google.registry.flows.exceptions.OnlyToolCanPassMetadataException} - * @error {@link google.registry.flows.exceptions.ResourceAlreadyExistsException} + * @error {@link ResourceAlreadyExistsForThisClientException} + * @error {@link ResourceCreateContentionException} * @error {@link google.registry.flows.EppException.UnimplementedExtensionException} * @error {@link google.registry.flows.ExtensionManager.UndeclaredServiceExtensionException} * @error {@link google.registry.flows.FlowUtils.UnknownCurrencyEppException} @@ -218,7 +226,7 @@ public class DomainCreateFlow implements TransactionalFlow { verifyUnitIsYears(period); int years = period.getValue(); validateRegistrationPeriod(years); - verifyResourceDoesNotExist(DomainBase.class, targetId, now); + verifyResourceDoesNotExist(DomainBase.class, targetId, now, clientId); // Validate that this is actually a legal domain name on a TLD that the registrar has access to. InternetDomainName domainName = validateDomainName(command.getFullyQualifiedDomainName()); String domainLabel = domainName.parts().get(0); diff --git a/java/google/registry/flows/exceptions/ResourceAlreadyExistsForThisClientException.java b/java/google/registry/flows/exceptions/ResourceAlreadyExistsForThisClientException.java new file mode 100644 index 000000000..d662a98c3 --- /dev/null +++ b/java/google/registry/flows/exceptions/ResourceAlreadyExistsForThisClientException.java @@ -0,0 +1,27 @@ +// Copyright 2019 The Nomulus Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package google.registry.flows.exceptions; + +import google.registry.flows.EppException.ObjectAlreadyExistsException; + +/** Resource with this id already exists. */ +// This is different from ResourceCreateContentionException in that this is used in situations where +// the requesting client already owns this resource. Javadoc and exception message are the same for +// backcompat purposes. +public class ResourceAlreadyExistsForThisClientException extends ObjectAlreadyExistsException { + public ResourceAlreadyExistsForThisClientException(String resourceId) { + super(String.format("Object with given ID (%s) already exists", resourceId)); + } +} diff --git a/java/google/registry/flows/exceptions/ResourceAlreadyExistsException.java b/java/google/registry/flows/exceptions/ResourceCreateContentionException.java similarity index 68% rename from java/google/registry/flows/exceptions/ResourceAlreadyExistsException.java rename to java/google/registry/flows/exceptions/ResourceCreateContentionException.java index c06305174..c04717bd0 100644 --- a/java/google/registry/flows/exceptions/ResourceAlreadyExistsException.java +++ b/java/google/registry/flows/exceptions/ResourceCreateContentionException.java @@ -17,8 +17,11 @@ package google.registry.flows.exceptions; import google.registry.flows.EppException.ObjectAlreadyExistsException; /** Resource with this id already exists. */ -public class ResourceAlreadyExistsException extends ObjectAlreadyExistsException { - public ResourceAlreadyExistsException(String resourceId) { +// This is different from ResourceAlreadyExistsForThisClientException in that this is used in +// resource creation contention situations, where another client owns this resource. Javadoc and +// exception message are the same for backcompat purposes. +public class ResourceCreateContentionException extends ObjectAlreadyExistsException { + public ResourceCreateContentionException(String resourceId) { super(String.format("Object with given ID (%s) already exists", resourceId)); } } diff --git a/java/google/registry/flows/host/HostCreateFlow.java b/java/google/registry/flows/host/HostCreateFlow.java index 47aee1798..ec6078c09 100644 --- a/java/google/registry/flows/host/HostCreateFlow.java +++ b/java/google/registry/flows/host/HostCreateFlow.java @@ -37,6 +37,8 @@ import google.registry.flows.FlowModule.ClientId; import google.registry.flows.FlowModule.TargetId; import google.registry.flows.TransactionalFlow; import google.registry.flows.annotations.ReportingSpec; +import google.registry.flows.exceptions.ResourceAlreadyExistsForThisClientException; +import google.registry.flows.exceptions.ResourceCreateContentionException; import google.registry.model.ImmutableObject; import google.registry.model.domain.DomainBase; import google.registry.model.domain.metadata.MetadataExtension; @@ -64,7 +66,8 @@ import org.joda.time.DateTime; * update DNS. * * @error {@link google.registry.flows.FlowUtils.IpAddressVersionMismatchException} - * @error {@link google.registry.flows.exceptions.ResourceAlreadyExistsException} + * @error {@link ResourceAlreadyExistsForThisClientException} + * @error {@link ResourceCreateContentionException} * @error {@link HostFlowUtils.HostNameTooLongException} * @error {@link HostFlowUtils.HostNameTooShallowException} * @error {@link HostFlowUtils.InvalidHostNameException} @@ -101,7 +104,7 @@ public final class HostCreateFlow implements TransactionalFlow { validateClientIsLoggedIn(clientId); Create command = (Create) resourceCommand; DateTime now = ofy().getTransactionTime(); - verifyResourceDoesNotExist(HostResource.class, targetId, now); + verifyResourceDoesNotExist(HostResource.class, targetId, now, clientId); // The superordinate domain of the host object if creating an in-bailiwick host, or null if // creating an external host. This is looked up before we actually create the Host object so // we can detect error conditions earlier. diff --git a/javatests/google/registry/flows/contact/ContactCreateFlowTest.java b/javatests/google/registry/flows/contact/ContactCreateFlowTest.java index b09b9036d..2e50fc4f6 100644 --- a/javatests/google/registry/flows/contact/ContactCreateFlowTest.java +++ b/javatests/google/registry/flows/contact/ContactCreateFlowTest.java @@ -17,8 +17,10 @@ package google.registry.flows.contact; import static com.google.common.truth.Truth.assertThat; import static google.registry.testing.ContactResourceSubject.assertAboutContacts; import static google.registry.testing.DatastoreHelper.assertNoBillingEvents; +import static google.registry.testing.DatastoreHelper.newContactResource; import static google.registry.testing.DatastoreHelper.persistActiveContact; import static google.registry.testing.DatastoreHelper.persistDeletedContact; +import static google.registry.testing.DatastoreHelper.persistResource; import static google.registry.testing.EppExceptionSubject.assertAboutEppExceptions; import static google.registry.testing.JUnitBackports.assertThrows; @@ -26,7 +28,8 @@ import google.registry.flows.EppException; import google.registry.flows.ResourceFlowTestCase; import google.registry.flows.contact.ContactFlowUtils.BadInternationalizedPostalInfoException; import google.registry.flows.contact.ContactFlowUtils.DeclineContactDisclosureFieldDisallowedPolicyException; -import google.registry.flows.exceptions.ResourceAlreadyExistsException; +import google.registry.flows.exceptions.ResourceAlreadyExistsForThisClientException; +import google.registry.flows.exceptions.ResourceCreateContentionException; import google.registry.model.contact.ContactResource; import org.joda.time.DateTime; import org.junit.Test; @@ -44,8 +47,10 @@ public class ContactCreateFlowTest assertTransactionalFlow(true); runFlowAssertResponse(loadFile("contact_create_response.xml")); // Check that the contact was created and persisted with a history entry. - assertAboutContacts().that(reloadResourceByForeignKey()) - .hasOnlyOneHistoryEntryWhich().hasNoXml(); + assertAboutContacts() + .that(reloadResourceByForeignKey()) + .hasOnlyOneHistoryEntryWhich() + .hasNoXml(); assertNoBillingEvents(); assertEppResourceIndexEntityFor(reloadResourceByForeignKey()); } @@ -70,8 +75,8 @@ public class ContactCreateFlowTest @Test public void testFailure_alreadyExists() throws Exception { persistActiveContact(getUniqueIdFromCommand()); - ResourceAlreadyExistsException thrown = - assertThrows(ResourceAlreadyExistsException.class, this::runFlow); + ResourceAlreadyExistsForThisClientException thrown = + assertThrows(ResourceAlreadyExistsForThisClientException.class, this::runFlow); assertThat(thrown) .hasMessageThat() .contains( @@ -79,6 +84,22 @@ public class ContactCreateFlowTest assertAboutEppExceptions().that(thrown).marshalsToXml(); } + @Test + public void testFailure_resourceContention() throws Exception { + String targetId = getUniqueIdFromCommand(); + persistResource( + newContactResource(targetId) + .asBuilder() + .setPersistedCurrentSponsorClientId("NewRegistrar") + .build()); + ResourceCreateContentionException thrown = + assertThrows(ResourceCreateContentionException.class, this::runFlow); + assertThat(thrown) + .hasMessageThat() + .contains(String.format("Object with given ID (%s) already exists", targetId)); + assertAboutEppExceptions().that(thrown).marshalsToXml(); + } + @Test public void testSuccess_nonAsciiInLocAddress() throws Exception { setEppInput("contact_create_hebrew_loc.xml"); diff --git a/javatests/google/registry/flows/domain/DomainCreateFlowTest.java b/javatests/google/registry/flows/domain/DomainCreateFlowTest.java index bdc07a69e..906b9a848 100644 --- a/javatests/google/registry/flows/domain/DomainCreateFlowTest.java +++ b/javatests/google/registry/flows/domain/DomainCreateFlowTest.java @@ -40,6 +40,7 @@ import static google.registry.testing.DatastoreHelper.deleteTld; import static google.registry.testing.DatastoreHelper.getHistoryEntries; import static google.registry.testing.DatastoreHelper.loadRegistrar; import static google.registry.testing.DatastoreHelper.newContactResource; +import static google.registry.testing.DatastoreHelper.newDomainBase; import static google.registry.testing.DatastoreHelper.newHostResource; import static google.registry.testing.DatastoreHelper.persistActiveContact; import static google.registry.testing.DatastoreHelper.persistActiveDomain; @@ -134,7 +135,8 @@ import google.registry.flows.domain.token.AllocationTokenFlowUtils.AllocationTok import google.registry.flows.domain.token.AllocationTokenFlowUtils.AlreadyRedeemedAllocationTokenException; import google.registry.flows.domain.token.AllocationTokenFlowUtils.InvalidAllocationTokenException; import google.registry.flows.exceptions.OnlyToolCanPassMetadataException; -import google.registry.flows.exceptions.ResourceAlreadyExistsException; +import google.registry.flows.exceptions.ResourceAlreadyExistsForThisClientException; +import google.registry.flows.exceptions.ResourceCreateContentionException; import google.registry.model.billing.BillingEvent; import google.registry.model.billing.BillingEvent.Flag; import google.registry.model.billing.BillingEvent.Reason; @@ -965,8 +967,8 @@ public class DomainCreateFlowTest extends ResourceFlowTestCase