diff --git a/core/src/main/java/google/registry/model/ForeignKeyUtils.java b/core/src/main/java/google/registry/model/ForeignKeyUtils.java index 9b7169912eb..ece9a743e7c 100644 --- a/core/src/main/java/google/registry/model/ForeignKeyUtils.java +++ b/core/src/main/java/google/registry/model/ForeignKeyUtils.java @@ -16,6 +16,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableMap.toImmutableMap; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static google.registry.config.RegistryConfig.getEppResourceCachingDuration; import static google.registry.config.RegistryConfig.getEppResourceMaxCachedEntries; import static google.registry.persistence.transaction.TransactionManagerFactory.replicaTm; @@ -400,4 +401,24 @@ public static Optional loadResourceByCache( .filter(e -> now.isBefore(e.getDeletionTime())) .map(e -> e.cloneProjectedAtTime(now)); } + + /** + * Loads the last created version of multiple {@link EppResource}s from the replica database by + * foreign keys, using a cache. + * + *

This method ignores the config setting for caching, and is reserved for use cases that can + * tolerate slightly stale data. + */ + @SuppressWarnings("unchecked") + public static ImmutableMap loadResourcesByCache( + Class clazz, Collection foreignKeys, Instant now) { + ImmutableSet> vkeys = + foreignKeys.stream().map(fk -> VKey.create(clazz, fk)).collect(toImmutableSet()); + return foreignKeyToResourceCache.getAll(vkeys).entrySet().stream() + .filter(e -> e.getValue().isPresent() && now.isBefore(e.getValue().get().getDeletionTime())) + .collect( + toImmutableMap( + e -> (String) e.getKey().getKey(), + e -> (E) e.getValue().get().cloneProjectedAtTime(now))); + } } diff --git a/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java b/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java index b56df0faaed..260867675d7 100644 --- a/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java +++ b/core/src/main/java/google/registry/rdap/RdapDomainSearchAction.java @@ -454,9 +454,8 @@ private DomainSearchResponse searchByNameserverRefs( // We must break the query up into chunks, because the in operator is limited to 30 subqueries. // Since it is possible for the same domain to show up more than once in our result list (if // we do a wildcard nameserver search that returns multiple nameservers used by the same - // domain), we must create a set of resulting {@link Domain} objects. Use a sorted set, - // and fetch all domains, to make sure that we can return the first domains in alphabetical - // order. + // domain), we must create a set of resulting {@link Domain}s. Use a sorted set, fetch all + // domains, to make sure that we can return the first domains in alphabetical order. ImmutableSortedSet.Builder domainSetBuilder = ImmutableSortedSet.orderedBy(Comparator.comparing(Domain::getDomainName)); int numHostKeysSearched = 0; @@ -465,7 +464,7 @@ private DomainSearchResponse searchByNameserverRefs( replicaTm() .transact( () -> { - for (VKey hostKey : hostKeys) { + for (VKey hostKey : chunk) { CriteriaQueryBuilder queryBuilder = CriteriaQueryBuilder.create(replicaTm(), Domain.class) .whereFieldContains("nsHosts", hostKey) diff --git a/core/src/main/java/google/registry/rdap/RdapNameserverSearchAction.java b/core/src/main/java/google/registry/rdap/RdapNameserverSearchAction.java index 5a15107767a..1444d15963e 100644 --- a/core/src/main/java/google/registry/rdap/RdapNameserverSearchAction.java +++ b/core/src/main/java/google/registry/rdap/RdapNameserverSearchAction.java @@ -185,7 +185,7 @@ private NameserverSearchResponse searchByNameUsingSuperordinateDomain( throw new UnprocessableEntityException( "A suffix after a wildcard in a nameserver lookup must be an in-bailiwick domain"); } - List hostList = new ArrayList<>(); + List matchingFqhns = new ArrayList<>(); for (String fqhn : ImmutableSortedSet.copyOf(domain.get().getSubordinateHosts())) { if (cursorString.isPresent() && (fqhn.compareTo(cursorString.get()) <= 0)) { continue; @@ -193,10 +193,22 @@ private NameserverSearchResponse searchByNameUsingSuperordinateDomain( // We can't just check that the host name starts with the initial query string, because // then the query ns.exam*.example.com would match against nameserver ns.example.com. if (partialStringQuery.matches(fqhn)) { - Optional host = - ForeignKeyUtils.loadResourceByCache(Host.class, fqhn, getRequestTime()); - if (shouldBeVisible(host)) { - hostList.add(host.get()); + matchingFqhns.add(fqhn); + } + } + List hostList = new ArrayList<>(); + int chunkSize = getStandardQuerySizeLimit(); + // Batch load from cache in chunks to avoid sequential N+1 database queries on cache misses. + for (List fqhnChunk : Iterables.partition(matchingFqhns, chunkSize)) { + if (hostList.size() > rdapResultSetMaxSize) { + break; + } + ImmutableMap cachedHosts = + ForeignKeyUtils.loadResourcesByCache(Host.class, fqhnChunk, getRequestTime()); + for (String fqhn : fqhnChunk) { + Host host = cachedHosts.get(fqhn); + if (host != null && shouldBeVisible(host)) { + hostList.add(host); if (hostList.size() > rdapResultSetMaxSize) { break; } @@ -204,10 +216,7 @@ private NameserverSearchResponse searchByNameUsingSuperordinateDomain( } } return makeSearchResults( - hostList, - IncompletenessWarningType.COMPLETE, - domain.get().getSubordinateHosts().size(), - CursorType.NAME); + hostList, IncompletenessWarningType.COMPLETE, hostList.size(), CursorType.NAME); } /** diff --git a/core/src/test/java/google/registry/model/ForeignKeyUtilsTest.java b/core/src/test/java/google/registry/model/ForeignKeyUtilsTest.java index b6125046803..bf0a62b81cc 100644 --- a/core/src/test/java/google/registry/model/ForeignKeyUtilsTest.java +++ b/core/src/test/java/google/registry/model/ForeignKeyUtilsTest.java @@ -144,4 +144,17 @@ void testSuccess_loadHostKeysCached_cacheIsStale() { fakeClock.now())) .containsExactlyEntriesIn(ImmutableMap.of("ns1.example.com", host1.createVKey())); } + + @Test + void testSuccess_loadResourcesByCache_skipsDeletedAndNonexistent() { + Host host1 = persistActiveHost("ns1.example.com"); + Host host2 = persistActiveHost("ns2.example.com"); + persistResource(host2.asBuilder().setDeletionTime(minusDays(fakeClock.now(), 1)).build()); + assertThat( + ForeignKeyUtils.loadResourcesByCache( + Host.class, + ImmutableList.of("ns1.example.com", "ns2.example.com", "ns3.example.com"), + fakeClock.now())) + .containsExactlyEntriesIn(ImmutableMap.of("ns1.example.com", host1)); + } } diff --git a/core/src/test/java/google/registry/rdap/RdapNameserverActionTest.java b/core/src/test/java/google/registry/rdap/RdapNameserverActionTest.java index 25570e65887..1f5231df114 100644 --- a/core/src/test/java/google/registry/rdap/RdapNameserverActionTest.java +++ b/core/src/test/java/google/registry/rdap/RdapNameserverActionTest.java @@ -25,6 +25,7 @@ import static google.registry.util.DateTimeUtils.minusYears; import static org.mockito.Mockito.verify; +import google.registry.model.host.Host; import google.registry.model.registrar.Registrar; import google.registry.rdap.RdapMetrics.EndpointType; import google.registry.rdap.RdapMetrics.SearchType; @@ -111,13 +112,16 @@ void testValidNameserver_works() { @Test void testNameserver_tldTithHyphenOn3And4_works() { createTld("zz--main-2166"); - persistResource(makePunycodedHost("ns1.cat.zz--main-2166", "1.2.3.4", null, "TheRegistrar")); + Host host = + persistResource( + makePunycodedHost("ns1.cat.zz--main-2166", "1.2.3.4", null, "TheRegistrar")); assertAboutJson() .that(generateActualJson("ns1.cat.zz--main-2166")) .isEqualTo( addPermanentBoilerplateNotices( jsonFileBuilder() - .addNameserver("ns1.cat.zz--main-2166", "ns1.cat.zz--main-2166", "F-ROID") + .addNameserver( + "ns1.cat.zz--main-2166", "ns1.cat.zz--main-2166", host.getRepoId()) .putAll("ADDRESSTYPE", "v4", "ADDRESS", "1.2.3.4", "STATUS", "active") .load("rdap_host.json"))); assertThat(response.getStatus()).isEqualTo(200); diff --git a/core/src/test/java/google/registry/rdap/RdapNameserverSearchActionTest.java b/core/src/test/java/google/registry/rdap/RdapNameserverSearchActionTest.java index 12d642e4004..20688dc82f0 100644 --- a/core/src/test/java/google/registry/rdap/RdapNameserverSearchActionTest.java +++ b/core/src/test/java/google/registry/rdap/RdapNameserverSearchActionTest.java @@ -29,6 +29,7 @@ import static google.registry.util.DateTimeUtils.minusMonths; import static google.registry.util.DateTimeUtils.minusYears; import static java.nio.charset.StandardCharsets.UTF_8; +import static org.mockito.Mockito.clearInvocations; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; @@ -46,6 +47,7 @@ import google.registry.testing.FakeResponse; import google.registry.testing.FullFieldsTestEntityHelper; import java.net.URLDecoder; +import java.time.Instant; import java.util.Optional; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -428,7 +430,7 @@ void testNameMatch_nsstar_cat_lol_notFound_differentRegistrarRequested() { action.registrarParam = Optional.of("unicoderegistrar"); generateActualJsonWithName("ns*.cat.lol"); assertThat(response.getStatus()).isEqualTo(404); - verifyErrorMetrics(Optional.of(2L), 404); + verifyErrorMetrics(Optional.of(0L), 404); } @Test @@ -445,6 +447,30 @@ void testNameMatch_star_cat_lol_found() { verifyMetrics(2); } + @Test + void testNameMatch_star_cat_lol_usesForeignKeyCache() { + generateActualJsonWithName("*.cat.lol"); + assertThat(response.getStatus()).isEqualTo(200); + verifyMetrics(2); + clearInvocations(rdapMetrics); + + Instant newTransferTime = clock.now(); + persistResource(hostNs1CatLol.asBuilder().setLastTransferTime(newTransferTime).build()); + clock.advanceOneMilli(); + + action.response = new FakeResponse(); + generateActualJsonWithName("*.cat.lol"); + assertThat(response.getStatus()).isEqualTo(200); + verifyMetrics(2); + + JsonObject searchResults = + parseJsonObject(response.getPayload()) + .getAsJsonArray("nameserverSearchResults") + .get(0) + .getAsJsonObject(); + assertThat(searchResults.toString()).doesNotContain(newTransferTime.toString()); + } + @Test void testNameMatch_star_cat_lol_found_sameRegistrarRequested() { action.registrarParam = Optional.of("TheRegistrar"); @@ -458,7 +484,7 @@ void testNameMatch_star_cat_lol_notFound_differentRegistrarRequested() { action.registrarParam = Optional.of("unicoderegistrar"); generateActualJsonWithName("*.cat.lol"); assertThat(response.getStatus()).isEqualTo(404); - verifyErrorMetrics(Optional.of(2L), 404); + verifyErrorMetrics(Optional.of(0L), 404); } @Test @@ -521,7 +547,7 @@ void testNameMatch_reallyTruncatedResultSet() { "rdap_truncated_hosts.json", "QUERY", "name=nsx*.cat.lol&cursor=bnN4NC5jYXQubG9s")); assertThat(response.getStatus()).isEqualTo(200); // When searching names, we look for additional matches, in case some are not visible. - verifyMetrics(9, IncompletenessWarningType.TRUNCATED); + verifyMetrics(5, IncompletenessWarningType.TRUNCATED); } @Test @@ -536,7 +562,7 @@ void testNameMatchDeletedHost_foundTheOtherHost() { .putAll("ADDRESSTYPE", "v6", "ADDRESS", "bad:f00d:cafe::15:beef") .load("rdap_host_linked.json"))); assertThat(response.getStatus()).isEqualTo(200); - verifyMetrics(2); + verifyMetrics(1); } @Test