Modify the way we load resources via foreign keys

[gbrodman]

Previously, we would have separate database calls for mapping from foreign key to repo ID and then from repo ID to object. This PR modifies those calls to load the resource directly (the old system was an artifact of the Datastore key-value storage system).

In this PR, we merge the load-resource-by-foreign-key calls into a single database load, as well as adding a separate cache object for (foreign key) -> (resource). Now we cache, and have separate cleaner code paths, for fk -> resource, fk -> repo ID, and repo ID -> resource.

---

This change is 

[CydeWeys]

Reviewable status: 0 of 63 files reviewed, 9 unresolved discussions

---

core/src/main/java/google/registry/tools/CommandUtilities.java line 45 at r1 (raw file):

    public VKey<? extends EppResource> getKey(String uniqueId, DateTime now) {
      return ForeignKeyUtils.loadKey(clazz, uniqueId, now)
          .orElseThrow(() -> new IllegalArgumentException("Invalid resource ID"));

Why is this necessary now?

Also the resource ID should be in the exception message.

---

core/src/main/java/google/registry/bsa/BsaValidateAction.java line 232 at r1 (raw file):

  boolean isStalenessAllowed(Domain domain) {
    return domain.getCreationTime().plus(maxStaleness).isAfter(clock.nowUtc());

I think this change is wrong -- the previous code was re-loading the domain to see if it was stale (i.e. if changes had been made in the mean time). Now you're no longer doing that here?

---

core/src/main/java/google/registry/tools/DomainLockUtils.java line 330 at r1 (raw file):

  private Domain getDomain(String domainName, String registrarId, DateTime now) {
    Domain domain =
        ForeignKeyUtils.loadResource(Domain.class, domainName, now)

Should this be calling loadResourceByCacheIfEnabled(), or no?

---

core/src/main/java/google/registry/model/ForeignKeyUtils.java line 54 at r1 (raw file):

 *
 * <p>One can retrieve either the {@link VKey} (the repo ID) in the situations where that's
 * sufficient, or the resource itself, either with or without cache.

caching

---

core/src/main/java/google/registry/model/ForeignKeyUtils.java line 103 at r1 (raw file):

    return loadMostRecentResources(clazz, foreignKeys, false).entrySet().stream()
        .filter(e -> now.isBefore(e.getValue().deletionTime()))
        .collect(toImmutableMap(Entry::getKey, e -> VKey.create(clazz, e.getValue().repoId())));

Should this be calling cloneProjectedAtTime() on what it's loading? There seem to be inconsistencies on whether this is done or not.

---

core/src/main/java/google/registry/model/ForeignKeyUtils.java line 287 at r1 (raw file):

        .get(VKey.create(clazz, foreignKey))
        .filter(mrr -> now.isBefore(mrr.deletionTime()))
        .map(mrr -> VKey.create(clazz, mrr.repoId()));

Should this be calling cloneProjectedAtTime() on what it's loading?

---

core/src/main/java/google/registry/model/ForeignKeyUtils.java line 319 at r1 (raw file):

                  keys.stream().map(key -> (String) key.getKey()).collect(toImmutableList());
              ImmutableMap<String, ? extends EppResource> existingResources =
                  loadMostRecentResourceObjects(clazz, foreignKeys, true);

Should this be calling cloneProjectedAtTime() on what it's loading?

---

core/src/main/java/google/registry/model/ForeignKeyUtils.java line 402 at r1 (raw file):

            .get(VKey.create(clazz, foreignKey))
            .filter(e -> now.isBefore(e.getDeletionTime()))
            .map(e -> e.cloneProjectedAtTime(now));

Should this be calling cloneProjectedAtTime() on what it's loading?

---

core/src/main/java/google/registry/tools/GetHistoryEntriesCommand.java line 64 at r1 (raw file):

      VKey<? extends EppResource> parentKey = type.getKey(uniqueId, DateTime.now(UTC));
      checkArgumentNotNull(parentKey, "Invalid resource ID");

This diff should be reverted? Looks like you made a change here at some point but then backed it out.

[CydeWeys]

This PR will be easier to review if the basic automated refactors are separated out from the logic changes. See #2864 where I've moved the bulk of the interface refactoring (without changing any actual logic like what this PR should be focused on).

Reviewable status: 0 of 63 files reviewed, 9 unresolved discussions (waiting on @gbrodman)

[gbrodman]

waiting on the other PR to merge before pushing the rebase on top of that

Reviewable status: 0 of 63 files reviewed, 9 unresolved discussions (waiting on @CydeWeys)

---

core/src/main/java/google/registry/bsa/BsaValidateAction.java line 232 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

I think this change is wrong -- the previous code was re-loading the domain to see if it was stale (i.e. if changes had been made in the mean time). Now you're no longer doing that here?

We're still loading the domain, it just happens before this call.

The code grabs unblockable domain names from BsaUnblockableDomain in batches of 1000 (there are only 1959 total at the moment), loads each one, and checks the creation time against "now" with a fudge factor of an hour. Given that this part of the action is fairly quick (it just loads the domains and does some in-memory checking), and the fact that the creation time doesn't vary, this is functionally equivalent -- we just save some DB calls

---

core/src/main/java/google/registry/model/ForeignKeyUtils.java line 54 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

caching

Done.

---

core/src/main/java/google/registry/model/ForeignKeyUtils.java line 103 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Should this be calling cloneProjectedAtTime() on what it's loading? There seem to be inconsistencies on whether this is done or not.

These are keys (not objects), so no.

---

core/src/main/java/google/registry/model/ForeignKeyUtils.java line 287 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Should this be calling cloneProjectedAtTime() on what it's loading?

This is a key (not an object), so no.

---

core/src/main/java/google/registry/model/ForeignKeyUtils.java line 319 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Should this be calling cloneProjectedAtTime() on what it's loading?

no, because it delegates to another method

---

core/src/main/java/google/registry/model/ForeignKeyUtils.java line 402 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Should this be calling cloneProjectedAtTime() on what it's loading?

yes, and it already is

We only project to current time in the public methods that end up returning the resources to the caller, except for "loadResource" because that's explicitly immediately delegated to "loadResources", which projects

---

core/src/main/java/google/registry/tools/CommandUtilities.java line 45 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Why is this necessary now?

Also the resource ID should be in the exception message.

this is necessary because the API of FKUtils was modified to consistently return an Optional instead of throwing a runtime exception

this method is only called by GetHistoryEntriesCommand anyway

---

core/src/main/java/google/registry/tools/DomainLockUtils.java line 330 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Should this be calling loadResourceByCacheIfEnabled(), or no?

nah, since this is actually altering domains we should probably load the fresh version of the resource, not risk a weird cache happening

---

core/src/main/java/google/registry/tools/GetHistoryEntriesCommand.java line 64 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

This diff should be reverted? Looks like you made a change here at some point but then backed it out.

eh yeah, this check should be removed as it's unnecessary (the helper method verifies existence)

[CydeWeys]

Reviewable status: 0 of 63 files reviewed, 10 unresolved discussions

---

core/src/main/java/google/registry/model/EppResourceUtils.java line 132 at r2 (raw file):

  }

  static <T extends EppResource> Optional<T> loadByForeignKeyHelper(

Now that PR #2864 is merged, you can address your comment in a separate PR ahead of this one if you'd like.

should we (in this PR) move this method and the two methods above (loadByForeignKeysByCacheIfEnabled and loadByForeignKeysByCache) to ForeignKeyUtils so that everything can live there?

[gbrodman]

Reviewable status: 0 of 63 files reviewed, 10 unresolved discussions (waiting on @CydeWeys)

---

core/src/main/java/google/registry/model/EppResourceUtils.java line 132 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Now that PR #2864 is merged, you can address your comment in a separate PR ahead of this one if you'd like.

should we (in this PR) move this method and the two methods above (loadByForeignKeysByCacheIfEnabled and loadByForeignKeysByCache) to ForeignKeyUtils so that everything can live there?

already done

[CydeWeys]

Reviewable status: 0 of 63 files reviewed, 9 unresolved discussions (waiting on @gbrodman)

---

core/src/main/java/google/registry/model/EppResourceUtils.java line 132 at r2 (raw file):

Previously, gbrodman wrote…

already done

Is it? What's the other PR?

[CydeWeys]

Reviewable status: 0 of 63 files reviewed, 2 unresolved discussions (waiting on @gbrodman)

---

core/src/main/java/google/registry/tools/DomainLockUtils.java line 330 at r1 (raw file):

Previously, gbrodman wrote…

nah, since this is actually altering domains we should probably load the fresh version of the resource, not risk a weird cache happening

OK, so it was a bug before then.

[CydeWeys]

Reviewable status: 0 of 63 files reviewed, 2 unresolved discussions (waiting on @gbrodman)

[gbrodman]

Reviewable status: 0 of 63 files reviewed, 2 unresolved discussions (waiting on @CydeWeys)

---

core/src/main/java/google/registry/model/EppResourceUtils.java line 132 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Is it? What's the other PR?

Oh I meant that it was already done in this PR in the rebase process, so we might as well just keep it

---

core/src/main/java/google/registry/tools/DomainLockUtils.java line 330 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

OK, so it was a bug before then.

yeah, a low-stakes one