From c9d7e759465232f237bc24562d349c2e58fea24b Mon Sep 17 00:00:00 2001 From: cgoldfeder Date: Tue, 16 May 2017 00:42:49 -0700 Subject: [PATCH] Cache Registrars in memory This replaces the memcache caching, which we think is overall a bad idea. We load all registrars at once instead of caching each as needed, so that the loadAllCached() methods can be cached as well, and therefore will always produce results consistent with loadByClientIdCached()'s view of the registrar's values. All of our prod registrars together total 300k of data right now, so this is hardly worth optimizing further, and in any case this will likely reduce latency even further since most requests will be served out of memory. While I was in the Registrar file I standardized the error messages for incorrect password and clientId length to be the same format, and cleaned up a few random things I noticed in the code. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=156151828 --- .../export/PublishDetailReportAction.java | 2 +- .../export/SyncGroupMembersAction.java | 8 +- .../export/sheet/SyncRegistrarsSheet.java | 4 +- .../flows/domain/DomainFlowUtils.java | 4 +- .../registry/flows/session/LoginFlow.java | 2 +- .../registry/model/registrar/Registrar.java | 163 +++++------------- .../registry/rdap/RdapEntityAction.java | 17 +- .../registry/rdap/RdapEntitySearchAction.java | 48 +++--- java/google/registry/rdap/RdapUtils.java | 38 ++++ .../google/registry/rde/RdeStagingMapper.java | 2 +- .../registry/rde/imports/RdeImportUtils.java | 2 +- java/google/registry/tmch/LordnTask.java | 2 +- .../registry/tools/VerifyOteCommand.java | 6 +- .../ui/server/registrar/ConsoleUiAction.java | 2 +- .../ui/server/registrar/SessionUtils.java | 6 +- .../whois/RegistrarLookupCommand.java | 10 +- .../model/registrar/RegistrarTest.java | 12 +- 17 files changed, 149 insertions(+), 179 deletions(-) create mode 100644 java/google/registry/rdap/RdapUtils.java diff --git a/java/google/registry/export/PublishDetailReportAction.java b/java/google/registry/export/PublishDetailReportAction.java index 8f863f8b2..769feb90c 100644 --- a/java/google/registry/export/PublishDetailReportAction.java +++ b/java/google/registry/export/PublishDetailReportAction.java @@ -91,7 +91,7 @@ public final class PublishDetailReportAction implements Runnable, JsonAction { try { logger.infofmt("Publishing detail report for parameters: %s", json); String registrarId = getParam(json, REGISTRAR_ID_PARAM); - Registrar registrar = checkArgumentNotNull(Registrar.loadByClientId(registrarId), + Registrar registrar = checkArgumentNotNull(Registrar.loadByClientIdCached(registrarId), "Registrar %s not found", registrarId); String driveFolderId = checkArgumentNotNull(registrar.getDriveFolderId(), "No drive folder associated with registrar " + registrarId); diff --git a/java/google/registry/export/SyncGroupMembersAction.java b/java/google/registry/export/SyncGroupMembersAction.java index fb93a03ff..e4392760d 100644 --- a/java/google/registry/export/SyncGroupMembersAction.java +++ b/java/google/registry/export/SyncGroupMembersAction.java @@ -113,13 +113,13 @@ public final class SyncGroupMembersAction implements Runnable { */ @Override public void run() { - List dirtyRegistrars = Registrar - .loadAllActive() + List dirtyRegistrars = FluentIterable.from(Registrar.loadAllCached()) .filter(new Predicate() { @Override public boolean apply(Registrar registrar) { - // Only grab registrars that require syncing and are of the correct type. - return registrar.getContactsRequireSyncing() + // Only grab active registrars that require syncing and are of the correct type. + return registrar.isActive() + && registrar.getContactsRequireSyncing() && registrar.getType() == Registrar.Type.REAL; }}) .toList(); diff --git a/java/google/registry/export/sheet/SyncRegistrarsSheet.java b/java/google/registry/export/sheet/SyncRegistrarsSheet.java index 0ea823c90..f6e8eb4f7 100644 --- a/java/google/registry/export/sheet/SyncRegistrarsSheet.java +++ b/java/google/registry/export/sheet/SyncRegistrarsSheet.java @@ -65,7 +65,7 @@ class SyncRegistrarsSheet { 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()) { + for (Registrar registrar : Registrar.loadAllCached()) { if (DateTimeUtils.isAtOrAfter(registrar.getLastUpdateTime(), lastUpdateTime)) { return true; } @@ -85,7 +85,7 @@ class SyncRegistrarsSheet { public int compare(Registrar left, Registrar right) { return left.getClientId().compareTo(right.getClientId()); } - }.immutableSortedCopy(Registrar.loadAll())) + }.immutableSortedCopy(Registrar.loadAllCached())) .filter( new Predicate() { @Override diff --git a/java/google/registry/flows/domain/DomainFlowUtils.java b/java/google/registry/flows/domain/DomainFlowUtils.java index 2527cc40c..b7c681733 100644 --- a/java/google/registry/flows/domain/DomainFlowUtils.java +++ b/java/google/registry/flows/domain/DomainFlowUtils.java @@ -230,7 +230,7 @@ public class DomainFlowUtils { /** Check if the registrar running the flow has access to the TLD in question. */ public static void checkAllowedAccessToTld(String clientId, String tld) throws EppException { - if (!Registrar.loadByClientId(clientId).getAllowedTlds().contains(tld)) { + if (!Registrar.loadByClientIdCached(clientId).getAllowedTlds().contains(tld)) { throw new DomainFlowUtils.NotAuthorizedForTldException(tld); } } @@ -434,7 +434,7 @@ public class DomainFlowUtils { if (isDomainPremium(domainName, priceTime)) { // NB: The load of the Registar object is transactionless, which means that it should hit // memcache most of the time. - if (Registrar.loadByClientId(clientId).getBlockPremiumNames()) { + if (Registrar.loadByClientIdCached(clientId).getBlockPremiumNames()) { throw new PremiumNameBlockedException(); } } diff --git a/java/google/registry/flows/session/LoginFlow.java b/java/google/registry/flows/session/LoginFlow.java index b4b9bfffa..eededa555 100644 --- a/java/google/registry/flows/session/LoginFlow.java +++ b/java/google/registry/flows/session/LoginFlow.java @@ -116,7 +116,7 @@ public class LoginFlow implements Flow { } serviceExtensionUrisBuilder.add(uri); } - Registrar registrar = Registrar.loadByClientId(login.getClientId()); + Registrar registrar = Registrar.loadByClientIdCached(login.getClientId()); if (registrar == null) { throw new BadRegistrarClientIdException(login.getClientId()); } diff --git a/java/google/registry/model/registrar/Registrar.java b/java/google/registry/model/registrar/Registrar.java index 8f2c4b087..a4fe2f02b 100644 --- a/java/google/registry/model/registrar/Registrar.java +++ b/java/google/registry/model/registrar/Registrar.java @@ -16,7 +16,6 @@ package google.registry.model.registrar; import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkArgument; -import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Predicates.equalTo; import static com.google.common.base.Predicates.in; import static com.google.common.base.Predicates.notNull; @@ -26,6 +25,7 @@ import static com.google.common.collect.Sets.immutableEnumSet; import static com.google.common.io.BaseEncoding.base64; import static google.registry.config.RegistryConfig.getDefaultRegistrarReferralUrl; import static google.registry.config.RegistryConfig.getDefaultRegistrarWhoisServer; +import static google.registry.model.CacheUtils.memoizeWithShortExpiration; import static google.registry.model.common.EntityGroupRoot.getCrossTldKey; import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.model.ofy.Ofy.RECOMMENDED_MEMCACHE_EXPIRATION; @@ -45,6 +45,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; +import com.google.common.collect.Range; import com.google.common.collect.Sets; import com.google.re2j.Pattern; import com.googlecode.objectify.Key; @@ -200,6 +201,27 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable } }; + /** + * A caching {@link Supplier} of a clientId to {@link Registrar} map. + * + *

The supplier's get() method enters a transactionless context briefly to avoid enrolling the + * query inside an unrelated client-affecting transaction. + */ + private static final Supplier> CACHE_BY_CLIENT_ID = + memoizeWithShortExpiration(new Supplier>() { + @Override + public ImmutableMap get() { + return ofy().doTransactionless(new Work>() { + @Override + public ImmutableMap run() { + ImmutableMap.Builder builder = new ImmutableMap.Builder<>(); + for (Registrar registrar : loadAll()) { + builder.put(registrar.getClientId(), registrar); + } + return builder.build(); + }}); + }}); + @Parent Key parent = getCrossTldKey(); @@ -306,9 +328,11 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable * @see Registrar IDs */ @Index + @Nullable Long ianaIdentifier; /** Identifier of registrar used in external billing system (e.g. Oracle). */ + @Nullable Long billingIdentifier; /** @@ -413,10 +437,12 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable return creationTime.getTimestamp(); } + @Nullable public Long getIanaIdentifier() { return ianaIdentifier; } + @Nullable public Long getBillingIdentifier() { return billingIdentifier; } @@ -647,7 +673,8 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable public Builder setClientId(String clientId) { // Client id must be [3,16] chars long. See "clIDType" in the base EPP schema of RFC 5730. // (Need to validate this here as there's no matching EPP XSD for validation.) - checkArgument(clientId.length() >= 3 && clientId.length() <= 16, + checkArgument( + Range.closed(3, 16).contains(clientId.length()), "Client identifier must be 3-16 characters long."); getInstance().clientIdentifier = clientId; return this; @@ -831,8 +858,9 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable public Builder setPassword(String password) { // Passwords must be [6,16] chars long. See "pwType" in the base EPP schema of RFC 5730. - checkArgument(password != null && password.length() >= 6 && password.length() <= 16, - "Password must be [6,16] characters long."); + checkArgument( + Range.closed(6, 16).contains(nullToEmpty(password).length()), + "Password must be 6-16 characters long."); getInstance().salt = base64().encode(saltSupplier.get()); getInstance().passwordHash = getInstance().hashPassword(password); return this; @@ -862,124 +890,25 @@ public class Registrar extends ImmutableObject implements Buildable, Jsonifiable } } - /** Load a registrar entity by its client id outside of a transaction. */ - @Nullable - public static Registrar loadByClientId(final String clientId) { - checkNotNull(clientId, "Client ID cannot be null"); - return ofy().doTransactionless(new Work() { - @Override - public Registrar run() { - return ofy().loadWithMemcache() - .type(Registrar.class) - .parent(getCrossTldKey()) - .id(clientId) - .now(); - }}); - } - - /** - * Load registrar entities by client id range outside of a transaction. - * - * @param clientIdStart returned registrars will have a client id greater than or equal to this - * @param clientIdAfterEnd returned registrars will have a client id less than this - * @param resultSetMaxSize the maximum number of registrar entities to be returned - */ - public static Iterable loadByClientIdRange( - final String clientIdStart, final String clientIdAfterEnd, final int resultSetMaxSize) { - return ofy().doTransactionless(new Work>() { - @Override - public Iterable run() { - return ofy().load() - .type(Registrar.class) - .filterKey(">=", Key.create(getCrossTldKey(), Registrar.class, clientIdStart)) - .filterKey("<", Key.create(getCrossTldKey(), Registrar.class, clientIdAfterEnd)) - .limit(resultSetMaxSize); - }}); - } - - /** Load a registrar entity by its name outside of a transaction. */ - @Nullable - public static Registrar loadByName(final String name) { - return ofy().doTransactionless(new Work() { - @Override - public Registrar run() { - return ofy().load() - .type(Registrar.class) - .filter("registrarName", name) - .first() - .now(); - }}); - } - - /** - * Load registrar entities by registrar name range, inclusive of the start but not the end, - * outside of a transaction. - * - * @param nameStart returned registrars will have a name greater than or equal to this - * @param nameAfterEnd returned registrars will have a name less than this - * @param resultSetMaxSize the maximum number of registrar entities to be returned - */ - public static Iterable loadByNameRange( - final String nameStart, final String nameAfterEnd, final int resultSetMaxSize) { - return ofy().doTransactionless(new Work>() { - @Override - public Iterable run() { - return ofy().load() - .type(Registrar.class) - .filter("registrarName >=", nameStart) - .filter("registrarName <", nameAfterEnd) - .limit(resultSetMaxSize); - }}); - } - - /** - * Load registrar entities by IANA identifier range outside of a transaction. - * - * @param ianaIdentifierStart returned registrars will have an IANA id greater than or equal to - * this - * @param ianaIdentifierAfterEnd returned registrars will have an IANA id less than this - * @param resultSetMaxSize the maximum number of registrar entities to be returned - */ - public static Iterable loadByIanaIdentifierRange( - final Long ianaIdentifierStart, - final Long ianaIdentifierAfterEnd, - final int resultSetMaxSize) { - return ofy().doTransactionless(new Work>() { - @Override - public Iterable run() { - return ofy().load() - .type(Registrar.class) - .filter("ianaIdentifier >=", ianaIdentifierStart) - .filter("ianaIdentifier <", ianaIdentifierAfterEnd) - .limit(resultSetMaxSize); - }}); - } - - /** Loads all registrar entities. */ + /** Loads all registrar entities directly from Datastore. */ public static Iterable loadAll() { - return ofy().load().type(Registrar.class).ancestor(getCrossTldKey()); + return ImmutableList.copyOf(ofy().load().type(Registrar.class).ancestor(getCrossTldKey())); } - /** Loads all active registrar entities. */ - public static FluentIterable loadAllActive() { - return FluentIterable.from(loadAll()).filter(IS_ACTIVE); + /** Loads all registrar entities using an in-memory cache. */ + public static Iterable loadAllCached() { + return CACHE_BY_CLIENT_ID.get().values(); } - private static final Predicate IS_ACTIVE = new Predicate() { - @Override - public boolean apply(Registrar registrar) { - return registrar.isActive(); - }}; - - /** Loads all active registrar entities. */ - public static FluentIterable loadAllActiveAndPubliclyVisible() { - return FluentIterable.from(loadAll()).filter(IS_ACTIVE_AND_PUBLICLY_VISIBLE); + /** Load a registrar entity by its client id directly from Datastore. */ + @Nullable + public static Registrar loadByClientId(String clientId) { + return ofy().load().type(Registrar.class).parent(getCrossTldKey()).id(clientId).now(); } - private static final Predicate IS_ACTIVE_AND_PUBLICLY_VISIBLE = - new Predicate() { - @Override - public boolean apply(Registrar registrar) { - return registrar.isActiveAndPubliclyVisible(); - }}; + /** Load a registrar entity by its client id using an in-memory cache. */ + @Nullable + public static Registrar loadByClientIdCached(String clientId) { + return CACHE_BY_CLIENT_ID.get().get(clientId); + } } diff --git a/java/google/registry/rdap/RdapEntityAction.java b/java/google/registry/rdap/RdapEntityAction.java index 0967fc56b..0fb1a38f4 100644 --- a/java/google/registry/rdap/RdapEntityAction.java +++ b/java/google/registry/rdap/RdapEntityAction.java @@ -15,12 +15,13 @@ package google.registry.rdap; import static google.registry.model.ofy.ObjectifyService.ofy; +import static google.registry.rdap.RdapUtils.getRegistrarByIanaIdentifier; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.HEAD; import com.google.common.base.Optional; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Iterables; +import com.google.common.primitives.Longs; import com.google.re2j.Pattern; import com.googlecode.objectify.Key; import google.registry.model.contact.ContactResource; @@ -96,18 +97,14 @@ public class RdapEntityAction extends RdapActionBase { OutputDataType.FULL); } } - try { - Long ianaIdentifier = Long.parseLong(pathSearchString); + Long ianaIdentifier = Longs.tryParse(pathSearchString); + if (ianaIdentifier != null) { wasValidKey = true; - Registrar registrar = Iterables.getOnlyElement( - Registrar.loadByIanaIdentifierRange(ianaIdentifier, ianaIdentifier + 1, 1), null); - if ((registrar != null) && registrar.isActiveAndPubliclyVisible()) { + Optional registrar = getRegistrarByIanaIdentifier(ianaIdentifier); + if ((registrar.isPresent()) && registrar.get().isActiveAndPubliclyVisible()) { return rdapJsonFormatter.makeRdapJsonForRegistrar( - registrar, true, rdapLinkBase, rdapWhoisServer, now, OutputDataType.FULL); + registrar.get(), true, rdapLinkBase, rdapWhoisServer, now, OutputDataType.FULL); } - } catch (NumberFormatException e) { - // Although the search string was not a valid IANA identifier, it might still have been a - // valid ROID. } // At this point, we have failed to find either a contact or a registrar. throw wasValidKey diff --git a/java/google/registry/rdap/RdapEntitySearchAction.java b/java/google/registry/rdap/RdapEntitySearchAction.java index 237575dff..4c4d07936 100644 --- a/java/google/registry/rdap/RdapEntitySearchAction.java +++ b/java/google/registry/rdap/RdapEntitySearchAction.java @@ -16,14 +16,18 @@ package google.registry.rdap; import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.rdap.RdapIcannStandardInformation.TRUNCATION_NOTICES; +import static google.registry.rdap.RdapUtils.getRegistrarByIanaIdentifier; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.HEAD; import static google.registry.util.DateTimeUtils.END_OF_TIME; import com.google.common.base.Optional; +import com.google.common.base.Predicate; +import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.primitives.Booleans; +import com.google.common.primitives.Longs; import com.googlecode.objectify.Key; import google.registry.config.RegistryConfig.Config; import google.registry.model.contact.ContactResource; @@ -138,19 +142,16 @@ public class RdapEntitySearchAction extends RdapActionBase { throw new UnprocessableEntityException("Suffixes not allowed in entity name searches"); } // Get the registrar matches, depending on whether there's a wildcard. - ImmutableList registrarMatches; - if (!partialStringQuery.getHasWildcard()) { - Registrar registrar = Registrar.loadByName(partialStringQuery.getInitialString()); - registrarMatches = (registrar == null) - ? ImmutableList.of() - : ImmutableList.of(registrar); - } else { - // Fetch an additional registrar, so we can detect result set truncation. - registrarMatches = ImmutableList.copyOf(Registrar.loadByNameRange( - partialStringQuery.getInitialString(), - partialStringQuery.getNextInitialString(), - rdapResultSetMaxSize + 1)); - } + ImmutableList registrarMatches = + FluentIterable.from(Registrar.loadAllCached()) + .filter( + new Predicate() { + @Override + public boolean apply(Registrar registrar) { + return partialStringQuery.matches(registrar.getRegistrarName()); + }}) + .limit(rdapResultSetMaxSize + 1) + .toList(); // Get the contact matches and return the results, fetching an additional contact to detect // truncation. return makeSearchResults( @@ -169,7 +170,8 @@ public class RdapEntitySearchAction extends RdapActionBase { .type(ContactResource.class) .id(partialStringQuery.getInitialString()) .now(); - ImmutableList registrars = getMatchingRegistrars(partialStringQuery); + ImmutableList registrars = + getMatchingRegistrars(partialStringQuery.getInitialString()); return makeSearchResults( ((contactResource == null) || !contactResource.getDeletionTime().isEqual(END_OF_TIME)) ? ImmutableList.of() : ImmutableList.of(contactResource), @@ -201,17 +203,15 @@ public class RdapEntitySearchAction extends RdapActionBase { } /** Looks up registrars by handle (i.e. IANA identifier). */ - private ImmutableList - getMatchingRegistrars(final RdapSearchPattern partialStringQuery) { - Long ianaIdentifier; - try { - ianaIdentifier = Long.parseLong(partialStringQuery.getInitialString()); - } catch (NumberFormatException e) { - return ImmutableList.of(); + private ImmutableList getMatchingRegistrars(final String ianaIdentifierString) { + Long ianaIdentifier = Longs.tryParse(ianaIdentifierString); + if (ianaIdentifier != null) { + Optional registrar = getRegistrarByIanaIdentifier(ianaIdentifier); + if (registrar.isPresent()) { + return ImmutableList.of(registrar.get()); + } } - // Fetch an additional registrar to detect result set truncation. - return ImmutableList.copyOf(Registrar.loadByIanaIdentifierRange( - ianaIdentifier, ianaIdentifier + 1, rdapResultSetMaxSize + 1)); + return ImmutableList.of(); } /** Builds a JSON array of entity info maps based on the specified contacts and registrars. */ diff --git a/java/google/registry/rdap/RdapUtils.java b/java/google/registry/rdap/RdapUtils.java new file mode 100644 index 000000000..dd04512ee --- /dev/null +++ b/java/google/registry/rdap/RdapUtils.java @@ -0,0 +1,38 @@ +// Copyright 2017 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.rdap; + +import static com.google.common.collect.Iterables.tryFind; + +import com.google.common.base.Optional; +import com.google.common.base.Predicate; +import google.registry.model.registrar.Registrar; + +/** Utility functions for RDAP. */ +public final class RdapUtils { + + private RdapUtils() {} + + /** Looks up a registrar by its IANA identifier. */ + static Optional getRegistrarByIanaIdentifier(final long ianaIdentifier) { + return tryFind( + Registrar.loadAllCached(), + new Predicate() { + @Override + public boolean apply(Registrar registrar) { + return registrar.getIanaIdentifier() == ianaIdentifier; + }}); + } +} diff --git a/java/google/registry/rde/RdeStagingMapper.java b/java/google/registry/rde/RdeStagingMapper.java index a26b75b2e..6eee05a06 100644 --- a/java/google/registry/rde/RdeStagingMapper.java +++ b/java/google/registry/rde/RdeStagingMapper.java @@ -62,7 +62,7 @@ public final class RdeStagingMapper extends Mapper getAllRegistrarNames() { - return Registrar.loadAllActive() + return FluentIterable.from(Registrar.loadAll()) .transform(new Function() { @Override public String apply(Registrar registrar) { + if (!registrar.isActive()) { + return null; + } String name = registrar.getClientId(); // Look for names of the form "regname-1", "regname-2", etc. and strip the -# suffix. String replacedName = name.replaceFirst("^(.*)-[1234]$", "$1"); diff --git a/java/google/registry/ui/server/registrar/ConsoleUiAction.java b/java/google/registry/ui/server/registrar/ConsoleUiAction.java index 420d64c2a..0711d9878 100644 --- a/java/google/registry/ui/server/registrar/ConsoleUiAction.java +++ b/java/google/registry/ui/server/registrar/ConsoleUiAction.java @@ -115,7 +115,7 @@ public final class ConsoleUiAction implements Runnable { .render()); return; } - Registrar registrar = Registrar.loadByClientId(sessionUtils.getRegistrarClientId(req)); + Registrar registrar = Registrar.loadByClientIdCached(sessionUtils.getRegistrarClientId(req)); data.put( "xsrfToken", xsrfTokenManager.generateToken(userService.getCurrentUser().getEmail())); diff --git a/java/google/registry/ui/server/registrar/SessionUtils.java b/java/google/registry/ui/server/registrar/SessionUtils.java index 9c3762117..3052fb3b0 100644 --- a/java/google/registry/ui/server/registrar/SessionUtils.java +++ b/java/google/registry/ui/server/registrar/SessionUtils.java @@ -122,7 +122,9 @@ public class SessionUtils { if (contact == null) { return Optional.absent(); } - Optional result = Optional.fromNullable(ofy().load().key(contact.getParent()).now()); + String registrarClientId = contact.getParent().getName(); + Optional result = + Optional.fromNullable(Registrar.loadByClientIdCached(registrarClientId)); if (!result.isPresent()) { logger.severefmt( "A contact record exists for non-existent registrar: %s.", Key.create(contact)); @@ -132,7 +134,7 @@ public class SessionUtils { /** @see #hasAccessToRegistrar(Registrar, String) */ private static boolean hasAccessToRegistrar(String clientId, final String gaeUserId) { - Registrar registrar = Registrar.loadByClientId(clientId); + Registrar registrar = Registrar.loadByClientIdCached(clientId); if (registrar == null) { logger.warningfmt("Registrar '%s' disappeared from Datastore!", clientId); return false; diff --git a/java/google/registry/whois/RegistrarLookupCommand.java b/java/google/registry/whois/RegistrarLookupCommand.java index 4393697bc..8f26dd0e8 100644 --- a/java/google/registry/whois/RegistrarLookupCommand.java +++ b/java/google/registry/whois/RegistrarLookupCommand.java @@ -25,6 +25,7 @@ import com.google.common.base.CharMatcher; import com.google.common.base.Joiner; import com.google.common.base.Splitter; import com.google.common.base.Supplier; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import google.registry.model.registrar.Registrar; import google.registry.util.FormattingLogger; @@ -48,10 +49,9 @@ final class RegistrarLookupCommand implements WhoisCommand { @Override public Map get() { Map map = new HashMap<>(); - // Use the normalized registrar name as a key. - Iterable registrars = Registrar.loadAllActiveAndPubliclyVisible(); - for (Registrar registrar : registrars) { - if (registrar.getRegistrarName() == null) { + // Use the normalized registrar name as a key, and ignore inactive and hidden registrars. + for (Registrar registrar : Registrar.loadAllCached()) { + if (!registrar.isActiveAndPubliclyVisible() || registrar.getRegistrarName() == null) { continue; } String normalized = normalizeRegistrarName(registrar.getRegistrarName()); @@ -65,7 +65,7 @@ final class RegistrarLookupCommand implements WhoisCommand { // if there isn't already a mapping for this string, so that if there's a registrar with a // two word name (Go Daddy) and no business-type suffix and another registrar with just // that first word as its name (Go), the latter will win. - for (Registrar registrar : registrars) { + for (Registrar registrar : ImmutableList.copyOf(map.values())) { if (registrar.getRegistrarName() == null) { continue; } diff --git a/javatests/google/registry/model/registrar/RegistrarTest.java b/javatests/google/registry/model/registrar/RegistrarTest.java index 801d8ba7d..389a970fc 100644 --- a/javatests/google/registry/model/registrar/RegistrarTest.java +++ b/javatests/google/registry/model/registrar/RegistrarTest.java @@ -142,19 +142,19 @@ public class RegistrarTest extends EntityTestCase { @Test public void testFailure_passwordNull() throws Exception { - thrown.expect(IllegalArgumentException.class, "Password must be [6,16] characters long."); + thrown.expect(IllegalArgumentException.class, "Password must be 6-16 characters long."); new Registrar.Builder().setPassword(null); } @Test public void testFailure_passwordTooShort() throws Exception { - thrown.expect(IllegalArgumentException.class, "Password must be [6,16] characters long."); + thrown.expect(IllegalArgumentException.class, "Password must be 6-16 characters long."); new Registrar.Builder().setPassword("abcde"); } @Test public void testFailure_passwordTooLong() throws Exception { - thrown.expect(IllegalArgumentException.class, "Password must be [6,16] characters long."); + thrown.expect(IllegalArgumentException.class, "Password must be 6-16 characters long."); new Registrar.Builder().setPassword("abcdefghijklmnopq"); } @@ -397,11 +397,11 @@ public class RegistrarTest extends EntityTestCase { } @Test - public void testLoadByClientId_isTransactionless() { + public void testLoadByClientIdCached_isTransactionless() { ofy().transact(new VoidWork() { @Override public void vrun() { - assertThat(Registrar.loadByClientId("registrar")).isNotNull(); + assertThat(Registrar.loadByClientIdCached("registrar")).isNotNull(); // Load something as a control to make sure we are seeing loaded keys in the session cache. ofy().load().entity(abuseAdminContact).now(); assertThat(ofy().getSessionKeys()).contains(Key.create(abuseAdminContact)); @@ -409,7 +409,7 @@ public class RegistrarTest extends EntityTestCase { }}); ofy().clearSessionCache(); // Conversely, loads outside of a transaction should end up in the session cache. - assertThat(Registrar.loadByClientId("registrar")).isNotNull(); + assertThat(Registrar.loadByClientIdCached("registrar")).isNotNull(); assertThat(ofy().getSessionKeys()).contains(Key.create(registrar)); } }