From 61599b0d456dec72316de84de336563ae5f8bdfa Mon Sep 17 00:00:00 2001 From: gbrodman Date: Tue, 23 Feb 2021 11:28:10 -0500 Subject: [PATCH] Partially convert RDAP ofy calls to tm calls (#964) * Partially convert RDAP ofy calls to tm calls This converts the simple retrieval actions but does not touch the more complicated search actions -- those use some ofy() query searching logic and will likely end up being significantly more complicated than this change. Here, though, we are just changing the calls that can be converted easily to tm() lookups. To change in future PRs: - RdapDomainSearchAction - RdapEntitySearchAction - RdapNameserverSearchAction - RdapSearchActionBase --- .../registry/model/registrar/Registrar.java | 7 ++ .../registry/rdap/RdapEntityAction.java | 14 ++-- .../registry/rdap/RdapJsonFormatter.java | 32 ++++----- .../registry/rdap/RdapSearchActionBase.java | 16 ----- .../UpdateRegistrarRdapBaseUrlsAction.java | 66 +++++++++---------- .../registry/rdap/RdapEntityActionTest.java | 60 +++++++++-------- .../registry/rdap/RdapJsonFormatterTest.java | 52 ++++++++------- ...UpdateRegistrarRdapBaseUrlsActionTest.java | 24 +++---- 8 files changed, 132 insertions(+), 139 deletions(-) diff --git a/core/src/main/java/google/registry/model/registrar/Registrar.java b/core/src/main/java/google/registry/model/registrar/Registrar.java index d27f6b870..4e235a273 100644 --- a/core/src/main/java/google/registry/model/registrar/Registrar.java +++ b/core/src/main/java/google/registry/model/registrar/Registrar.java @@ -996,6 +996,13 @@ public class Registrar extends ImmutableObject return CACHE_BY_CLIENT_ID.get().values(); } + /** Loads all registrar keys using an in-memory cache. */ + public static ImmutableSet> loadAllKeysCached() { + return CACHE_BY_CLIENT_ID.get().keySet().stream() + .map(Registrar::createVKey) + .collect(toImmutableSet()); + } + /** Loads and returns a registrar entity by its client id directly from Datastore. */ public static Optional loadByClientId(String clientId) { checkArgument(!Strings.isNullOrEmpty(clientId), "clientId must be specified"); diff --git a/core/src/main/java/google/registry/rdap/RdapEntityAction.java b/core/src/main/java/google/registry/rdap/RdapEntityAction.java index e57f786a9..6a4bfddfb 100644 --- a/core/src/main/java/google/registry/rdap/RdapEntityAction.java +++ b/core/src/main/java/google/registry/rdap/RdapEntityAction.java @@ -14,7 +14,8 @@ package google.registry.rdap; -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 google.registry.rdap.RdapUtils.getRegistrarByIanaIdentifier; import static google.registry.rdap.RdapUtils.getRegistrarByName; import static google.registry.request.Action.Method.GET; @@ -23,9 +24,9 @@ import static google.registry.request.Action.Method.HEAD; import com.google.common.collect.ImmutableSet; import com.google.common.primitives.Longs; import com.google.re2j.Pattern; -import com.googlecode.objectify.Key; import google.registry.model.contact.ContactResource; import google.registry.model.registrar.Registrar; +import google.registry.persistence.VKey; import google.registry.rdap.RdapJsonFormatter.OutputDataType; import google.registry.rdap.RdapMetrics.EndpointType; import google.registry.rdap.RdapObjectClasses.RdapEntity; @@ -69,13 +70,14 @@ public class RdapEntityAction extends RdapActionBase { // RDAP Technical Implementation Guide 2.3.1 - MUST support contact entity lookup using the // handle if (ROID_PATTERN.matcher(pathSearchString).matches()) { - Key contactKey = Key.create(ContactResource.class, pathSearchString); - ContactResource contactResource = ofy().load().key(contactKey).now(); + VKey contactVKey = VKey.create(ContactResource.class, pathSearchString); + Optional contactResource = + transactIfJpaTm(() -> tm().loadByKeyIfPresent(contactVKey)); // As per Andy Newton on the regext mailing list, contacts by themselves have no role, since // they are global, and might have different roles for different domains. - if (contactResource != null && isAuthorized(contactResource)) { + if (contactResource.isPresent() && isAuthorized(contactResource.get())) { return rdapJsonFormatter.createRdapContactEntity( - contactResource, ImmutableSet.of(), OutputDataType.FULL); + contactResource.get(), ImmutableSet.of(), OutputDataType.FULL); } } diff --git a/core/src/main/java/google/registry/rdap/RdapJsonFormatter.java b/core/src/main/java/google/registry/rdap/RdapJsonFormatter.java index 018a90800..7a98cbc6a 100644 --- a/core/src/main/java/google/registry/rdap/RdapJsonFormatter.java +++ b/core/src/main/java/google/registry/rdap/RdapJsonFormatter.java @@ -20,8 +20,8 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap; import static google.registry.model.EppResourceUtils.isLinked; -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 google.registry.rdap.RdapIcannStandardInformation.CONTACT_REDACTED_VALUE; import static google.registry.util.CollectionUtils.union; @@ -35,7 +35,6 @@ import com.google.common.collect.Streams; import com.google.common.flogger.FluentLogger; import com.google.common.net.InetAddresses; import com.google.gson.JsonArray; -import com.googlecode.objectify.Key; import google.registry.config.RegistryConfig.Config; import google.registry.model.EppResource; import google.registry.model.contact.ContactAddress; @@ -77,7 +76,6 @@ import java.net.URI; import java.nio.file.Paths; import java.util.HashMap; import java.util.Locale; -import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.stream.Stream; @@ -343,15 +341,11 @@ public class RdapJsonFormatter { // Kick off the database loads of the nameservers that we will need, so it can load // asynchronously while we load and process the contacts. ImmutableSet loadedHosts = - ImmutableSet.copyOf(tm().loadByKeys(domainBase.getNameservers()).values()); + transactIfJpaTm( + () -> ImmutableSet.copyOf(tm().loadByKeys(domainBase.getNameservers()).values())); // Load the registrant and other contacts and add them to the data. - Map, ContactResource> loadedContacts = - ofy() - .load() - .keys( - domainBase.getReferencedContacts().stream() - .map(VKey::getOfyKey) - .collect(toImmutableSet())); + ImmutableMap, ContactResource> loadedContacts = + transactIfJpaTm(() -> tm().loadByKeysIfPresent(domainBase.getReferencedContacts())); // RDAP Response Profile 2.7.3, A domain MUST have the REGISTRANT, ADMIN, TECH roles and MAY // have others. We also add the BILLING. // @@ -359,16 +353,16 @@ public class RdapJsonFormatter { // fields we don't want to show (as opposed to not having contacts at all) because of GDPR etc. // // the GDPR redaction is handled in createRdapContactEntity - ImmutableSetMultimap, Type> contactsToRoles = + ImmutableSetMultimap, Type> contactsToRoles = Streams.concat( domainBase.getContacts().stream(), Stream.of(DesignatedContact.create(Type.REGISTRANT, domainBase.getRegistrant()))) .sorted(DESIGNATED_CONTACT_ORDERING) .collect( toImmutableSetMultimap( - contact -> contact.getContactKey().getOfyKey(), DesignatedContact::getType)); + DesignatedContact::getContactKey, DesignatedContact::getType)); - for (Key contactKey : contactsToRoles.keySet()) { + for (VKey contactKey : contactsToRoles.keySet()) { Set roles = contactsToRoles.get(contactKey).stream() .map(RdapJsonFormatter::convertContactTypeToRdapRole) @@ -430,10 +424,12 @@ public class RdapJsonFormatter { statuses.add(StatusValue.LINKED); } if (hostResource.isSubordinate() - && tm().loadByKey(hostResource.getSuperordinateDomain()) - .cloneProjectedAtTime(getRequestTime()) - .getStatusValues() - .contains(StatusValue.PENDING_TRANSFER)) { + && transactIfJpaTm( + () -> + tm().loadByKey(hostResource.getSuperordinateDomain()) + .cloneProjectedAtTime(getRequestTime()) + .getStatusValues() + .contains(StatusValue.PENDING_TRANSFER))) { statuses.add(StatusValue.PENDING_TRANSFER); } builder diff --git a/core/src/main/java/google/registry/rdap/RdapSearchActionBase.java b/core/src/main/java/google/registry/rdap/RdapSearchActionBase.java index 4392ba085..b1d844b60 100644 --- a/core/src/main/java/google/registry/rdap/RdapSearchActionBase.java +++ b/core/src/main/java/google/registry/rdap/RdapSearchActionBase.java @@ -392,22 +392,6 @@ public abstract class RdapSearchActionBase extends RdapActionBase { return setOtherQueryAttributes(query, deletedItemHandling, resultSetMaxSize); } - /** Handles searches by key using a simple string. */ - static Query queryItemsByKey( - Class clazz, - String queryString, - DeletedItemHandling deletedItemHandling, - int resultSetMaxSize) { - if (queryString.length() < RdapSearchPattern.MIN_INITIAL_STRING_LENGTH) { - throw new UnprocessableEntityException( - String.format( - "Initial search string must be at least %d characters", - RdapSearchPattern.MIN_INITIAL_STRING_LENGTH)); - } - Query query = ofy().load().type(clazz).filterKey("=", Key.create(clazz, queryString)); - return setOtherQueryAttributes(query, deletedItemHandling, resultSetMaxSize); - } - static Query setOtherQueryAttributes( Query query, DeletedItemHandling deletedItemHandling, int resultSetMaxSize) { if (deletedItemHandling != DeletedItemHandling.INCLUDE) { diff --git a/core/src/main/java/google/registry/rdap/UpdateRegistrarRdapBaseUrlsAction.java b/core/src/main/java/google/registry/rdap/UpdateRegistrarRdapBaseUrlsAction.java index 6974713f5..ec3c12a0a 100644 --- a/core/src/main/java/google/registry/rdap/UpdateRegistrarRdapBaseUrlsAction.java +++ b/core/src/main/java/google/registry/rdap/UpdateRegistrarRdapBaseUrlsAction.java @@ -16,7 +16,6 @@ package google.registry.rdap; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; -import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static java.nio.charset.StandardCharsets.UTF_8; @@ -34,7 +33,6 @@ import com.google.gson.Gson; import com.google.gson.JsonArray; import com.google.gson.JsonElement; import com.google.gson.JsonObject; -import com.googlecode.objectify.Key; import google.registry.keyring.api.KeyModule; import google.registry.model.registrar.Registrar; import google.registry.model.registry.Registries; @@ -197,37 +195,37 @@ public final class UpdateRegistrarRdapBaseUrlsAction implements Runnable { @Override public void run() { ImmutableSetMultimap ianaToBaseUrls = getRdapBaseUrlsPerIanaId(); - - for (Key registrarKey : ofy().load().type(Registrar.class).keys()) { - tm() - .transact( - () -> { - Registrar registrar = ofy().load().key(registrarKey).now(); - // Has the registrar been deleted since we loaded the key? (unlikly, especially - // given we don't delete registrars...) - if (registrar == null) { - return; - } - // Only update REAL registrars - if (registrar.getType() != Registrar.Type.REAL) { - return; - } - String ianaId = String.valueOf(registrar.getIanaIdentifier()); - ImmutableSet baseUrls = ianaToBaseUrls.get(ianaId); - // If this registrar already has these values, skip it - if (registrar.getRdapBaseUrls().equals(baseUrls)) { - logger.atInfo().log( - "No change in RdapBaseUrls for registrar %s (ianaId %s)", - registrar.getClientId(), ianaId); - return; - } - logger.atInfo().log( - "Updating RdapBaseUrls for registrar %s (ianaId %s) from %s to %s", - registrar.getClientId(), ianaId, registrar.getRdapBaseUrls(), baseUrls); - ofy() - .save() - .entity(registrar.asBuilder().setRdapBaseUrls(baseUrls).build()); - }); - } + Registrar.loadAllKeysCached() + .forEach( + (key) -> + tm().transact( + () -> { + Registrar registrar = tm().loadByKey(key); + // Has the registrar been deleted since we loaded the key? (unlikely, + // especially given we don't delete registrars...) + if (registrar == null) { + return; + } + // Only update REAL registrars + if (registrar.getType() != Registrar.Type.REAL) { + return; + } + String ianaId = String.valueOf(registrar.getIanaIdentifier()); + ImmutableSet baseUrls = ianaToBaseUrls.get(ianaId); + // If this registrar already has these values, skip it + if (registrar.getRdapBaseUrls().equals(baseUrls)) { + logger.atInfo().log( + "No change in RdapBaseUrls for registrar %s (ianaId %s)", + registrar.getClientId(), ianaId); + return; + } + logger.atInfo().log( + "Updating RdapBaseUrls for registrar %s (ianaId %s) from %s to %s", + registrar.getClientId(), + ianaId, + registrar.getRdapBaseUrls(), + baseUrls); + tm().put(registrar.asBuilder().setRdapBaseUrls(baseUrls).build()); + })); } } diff --git a/core/src/test/java/google/registry/rdap/RdapEntityActionTest.java b/core/src/test/java/google/registry/rdap/RdapEntityActionTest.java index 7836319b2..6790f67da 100644 --- a/core/src/test/java/google/registry/rdap/RdapEntityActionTest.java +++ b/core/src/test/java/google/registry/rdap/RdapEntityActionTest.java @@ -38,12 +38,14 @@ import google.registry.rdap.RdapMetrics.SearchType; import google.registry.rdap.RdapMetrics.WildcardType; import google.registry.rdap.RdapSearchResults.IncompletenessWarningType; import google.registry.request.Action; +import google.registry.testing.DualDatabaseTest; +import google.registry.testing.TestOfyAndSql; import java.util.Optional; import javax.annotation.Nullable; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; /** Unit tests for {@link RdapEntityAction}. */ +@DualDatabaseTest class RdapEntityActionTest extends RdapActionBaseTestCase { RdapEntityActionTest() { @@ -187,35 +189,35 @@ class RdapEntityActionTest extends RdapActionBaseTestCase { assertThat(response.getStatus()).isEqualTo(404); } - @Test + @TestOfyAndSql void testUnknownEntity_RoidPattern_notFound() { runNotFoundTest("_MISSING-ENTITY_"); } - @Test + @TestOfyAndSql void testUnknownEntity_IanaPattern_notFound() { runNotFoundTest("123"); } - @Test + @TestOfyAndSql void testUnknownEntity_notRoidNotIana_notFound() { // Since we allow search by registrar name, every string is a possible name runNotFoundTest("some,random,string"); } - @Test + @TestOfyAndSql void testValidRegistrantContact_works() { login("evilregistrar"); runSuccessfulHandleTest(registrant.getRepoId(), "rdap_associated_contact.json"); } - @Test + @TestOfyAndSql void testValidRegistrantContact_found_asAdministrator() { loginAsAdmin(); runSuccessfulHandleTest(registrant.getRepoId(), "rdap_associated_contact.json"); } - @Test + @TestOfyAndSql void testValidRegistrantContact_found_notLoggedIn() { runSuccessfulHandleTest( registrant.getRepoId(), @@ -225,7 +227,7 @@ class RdapEntityActionTest extends RdapActionBaseTestCase { "rdap_associated_contact_no_personal_data.json"); } - @Test + @TestOfyAndSql void testValidRegistrantContact_found_loggedInAsOtherRegistrar() { login("otherregistrar"); runSuccessfulHandleTest( @@ -236,49 +238,49 @@ class RdapEntityActionTest extends RdapActionBaseTestCase { "rdap_associated_contact_no_personal_data.json"); } - @Test + @TestOfyAndSql void testValidAdminContact_works() { login("evilregistrar"); runSuccessfulHandleTest(adminContact.getRepoId(), "rdap_associated_contact.json"); } - @Test + @TestOfyAndSql void testValidTechContact_works() { login("evilregistrar"); runSuccessfulHandleTest(techContact.getRepoId(), "rdap_associated_contact.json"); } - @Test + @TestOfyAndSql void testValidDisconnectedContact_works() { login("evilregistrar"); runSuccessfulHandleTest(disconnectedContact.getRepoId(), "rdap_contact.json"); } - @Test + @TestOfyAndSql void testDeletedContact_notFound() { runNotFoundTest(deletedContact.getRepoId()); } - @Test + @TestOfyAndSql void testDeletedContact_notFound_includeDeletedSetFalse() { action.includeDeletedParam = Optional.of(false); runNotFoundTest(deletedContact.getRepoId()); } - @Test + @TestOfyAndSql void testDeletedContact_notFound_notLoggedIn() { action.includeDeletedParam = Optional.of(true); runNotFoundTest(deletedContact.getRepoId()); } - @Test + @TestOfyAndSql void testDeletedContact_notFound_loggedInAsDifferentRegistrar() { login("idnregistrar"); action.includeDeletedParam = Optional.of(true); runNotFoundTest(deletedContact.getRepoId()); } - @Test + @TestOfyAndSql void testDeletedContact_found_loggedInAsCorrectRegistrar() { login("evilregistrar"); action.includeDeletedParam = Optional.of(true); @@ -290,7 +292,7 @@ class RdapEntityActionTest extends RdapActionBaseTestCase { "rdap_contact_deleted.json"); } - @Test + @TestOfyAndSql void testDeletedContact_found_loggedInAsAdmin() { loginAsAdmin(); action.includeDeletedParam = Optional.of(true); @@ -302,12 +304,12 @@ class RdapEntityActionTest extends RdapActionBaseTestCase { "rdap_contact_deleted.json"); } - @Test + @TestOfyAndSql void testRegistrar_found() { runSuccessfulHandleTest("101", "Yes Virginia