From 0241937dee102b423917a1ab3e919bba7afbc154 Mon Sep 17 00:00:00 2001 From: sarahcaseybot Date: Wed, 17 Jul 2024 18:06:09 -0400 Subject: [PATCH] Use feature flag for minimum dataset in domain flows to decide when to check for required contacts (#2486) * Check FeatureFlag in domain flows before checking contacts Check if phase 1 has begun of the transition to the minimum registry dataset, and if it has, do not require the presence of contacts in domain flows. * Add tests * Small test fixes * rename flag * Fix merge conflicts * Change todo * Add isActive methods * Add javadocs * small fix --- .../flows/domain/DomainFlowUtils.java | 13 +++- .../flows/domain/DomainUpdateFlow.java | 19 ++++-- .../registry/model/common/FeatureFlag.java | 12 ++++ .../flows/EppLifecycleDomainTest.java | 9 +++ .../registry/flows/EppLifecycleHostTest.java | 18 +++++ .../registry/flows/EppPointInTimeTest.java | 12 ++++ .../flows/domain/DomainCreateFlowTest.java | 68 ++++++++++++++++++- .../flows/domain/DomainUpdateFlowTest.java | 66 ++++++++++++++++++ .../model/common/FeatureFlagTest.java | 18 +++++ .../registry/tools/EppLifecycleToolsTest.java | 12 ++++ 10 files changed, 237 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java b/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java index f7c9c135d..1dba91f04 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java +++ b/core/src/main/java/google/registry/flows/domain/DomainFlowUtils.java @@ -24,6 +24,8 @@ import static com.google.common.collect.Sets.difference; import static com.google.common.collect.Sets.intersection; import static com.google.common.collect.Sets.union; import static google.registry.bsa.persistence.BsaLabelUtils.isLabelBlocked; +import static google.registry.model.common.FeatureFlag.FeatureName.MINIMUM_DATASET_CONTACTS_OPTIONAL; +import static google.registry.model.common.FeatureFlag.isActiveNow; import static google.registry.model.domain.Domain.MAX_REGISTRATION_YEARS; import static google.registry.model.domain.token.AllocationToken.TokenType.REGISTER_BSA; import static google.registry.model.tld.Tld.TldState.GENERAL_AVAILABILITY; @@ -481,10 +483,14 @@ public class DomainFlowUtils { } } - static void validateRequiredContactsPresent( + static void validateRequiredContactsPresentIfRequiredForDataset( Optional> registrant, Set contacts) throws RequiredParameterMissingException { - // TODO: Check minimum reg data set migration schedule here and don't throw when any are empty. + // TODO(b/353347632): Change this flag check to a registry config check. + if (isActiveNow(MINIMUM_DATASET_CONTACTS_OPTIONAL)) { + // Contacts are not required once we have begun the migration to the minimum dataset + return; + } if (registrant.isEmpty()) { throw new MissingRegistrantException(); } @@ -1041,7 +1047,8 @@ public class DomainFlowUtils { String tldStr = tld.getTldStr(); validateRegistrantAllowedOnTld(tldStr, command.getRegistrantContactId()); validateNoDuplicateContacts(command.getContacts()); - validateRequiredContactsPresent(command.getRegistrant(), command.getContacts()); + validateRequiredContactsPresentIfRequiredForDataset( + command.getRegistrant(), command.getContacts()); ImmutableSet hostNames = command.getNameserverHostNames(); validateNameserversCountForTld(tldStr, domainName, hostNames.size()); validateNameserversAllowedOnTld(tldStr, hostNames); diff --git a/core/src/main/java/google/registry/flows/domain/DomainUpdateFlow.java b/core/src/main/java/google/registry/flows/domain/DomainUpdateFlow.java index 2d66e54a6..7cbf6fdb9 100644 --- a/core/src/main/java/google/registry/flows/domain/DomainUpdateFlow.java +++ b/core/src/main/java/google/registry/flows/domain/DomainUpdateFlow.java @@ -38,9 +38,11 @@ import static google.registry.flows.domain.DomainFlowUtils.validateNameserversAl import static google.registry.flows.domain.DomainFlowUtils.validateNameserversCountForTld; import static google.registry.flows.domain.DomainFlowUtils.validateNoDuplicateContacts; import static google.registry.flows.domain.DomainFlowUtils.validateRegistrantAllowedOnTld; -import static google.registry.flows.domain.DomainFlowUtils.validateRequiredContactsPresent; +import static google.registry.flows.domain.DomainFlowUtils.validateRequiredContactsPresentIfRequiredForDataset; import static google.registry.flows.domain.DomainFlowUtils.verifyClientUpdateNotProhibited; import static google.registry.flows.domain.DomainFlowUtils.verifyNotInPendingDelete; +import static google.registry.model.common.FeatureFlag.FeatureName.MINIMUM_DATASET_CONTACTS_OPTIONAL; +import static google.registry.model.common.FeatureFlag.isActiveNow; import static google.registry.model.reporting.HistoryEntry.Type.DOMAIN_UPDATE; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; @@ -248,7 +250,7 @@ public final class DomainUpdateFlow implements MutatingFlow { checkSameValuesNotAddedAndRemoved(add.getContacts(), remove.getContacts()); checkSameValuesNotAddedAndRemoved(add.getStatusValues(), remove.getStatusValues()); Change change = command.getInnerChange(); - validateRegistrantIsntBeingRemoved(change); + validateRegistrantIsntBeingRemovedIfRequiredForDataset(change); Optional secDnsUpdate = eppInput.getSingleExtension(SecDnsUpdateExtension.class); @@ -300,9 +302,13 @@ public final class DomainUpdateFlow implements MutatingFlow { return domainBuilder.build(); } - private static void validateRegistrantIsntBeingRemoved(Change change) throws EppException { - // TODO(mcilwain): Make this check the minimum registration data set migration schedule - // and not require presence of a registrant in later stages. + private static void validateRegistrantIsntBeingRemovedIfRequiredForDataset(Change change) + throws EppException { + // TODO(b/353347632): Change this flag check to a registry config check. + if (isActiveNow(MINIMUM_DATASET_CONTACTS_OPTIONAL)) { + // registrants are not required once we have begun the migration to the minimum dataset + return; + } if (change.getRegistrantContactId().isPresent() && change.getRegistrantContactId().get().isEmpty()) { throw new MissingRegistrantException(); @@ -317,7 +323,8 @@ public final class DomainUpdateFlow implements MutatingFlow { * cause Domain update failure. */ private static void validateNewState(Domain newDomain) throws EppException { - validateRequiredContactsPresent(newDomain.getRegistrant(), newDomain.getContacts()); + validateRequiredContactsPresentIfRequiredForDataset( + newDomain.getRegistrant(), newDomain.getContacts()); validateDsData(newDomain.getDsData()); validateNameserversCountForTld( newDomain.getTld(), diff --git a/core/src/main/java/google/registry/model/common/FeatureFlag.java b/core/src/main/java/google/registry/model/common/FeatureFlag.java index cf1992b5a..a77bfabea 100644 --- a/core/src/main/java/google/registry/model/common/FeatureFlag.java +++ b/core/src/main/java/google/registry/model/common/FeatureFlag.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.collect.ImmutableSet.toImmutableSet; import static google.registry.config.RegistryConfig.getSingletonCacheRefreshDuration; +import static google.registry.model.common.FeatureFlag.FeatureStatus.ACTIVE; import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import com.fasterxml.jackson.annotation.JsonProperty; @@ -150,6 +151,17 @@ public class FeatureFlag extends ImmutableObject implements Buildable { return status.getValueAtTime(time); } + /** Returns if the FeatureFlag with the given FeatureName is active now. */ + public static boolean isActiveNow(FeatureName featureName) { + tm().assertInTransaction(); + return isActiveAt(featureName, tm().getTransactionTime()); + } + + /** Returns if the FeatureFlag with the given FeatureName is active at a given time. */ + public static boolean isActiveAt(FeatureName featureName, DateTime dateTime) { + return FeatureFlag.get(featureName).getStatus(dateTime).equals(ACTIVE); + } + @Override public FeatureFlag.Builder asBuilder() { return new FeatureFlag.Builder(clone(this)); diff --git a/core/src/test/java/google/registry/flows/EppLifecycleDomainTest.java b/core/src/test/java/google/registry/flows/EppLifecycleDomainTest.java index ac226f007..9e5c516c6 100644 --- a/core/src/test/java/google/registry/flows/EppLifecycleDomainTest.java +++ b/core/src/test/java/google/registry/flows/EppLifecycleDomainTest.java @@ -16,6 +16,8 @@ package google.registry.flows; import static com.google.common.truth.Truth.assertThat; import static google.registry.model.EppResourceUtils.loadByForeignKey; +import static google.registry.model.common.FeatureFlag.FeatureName.MINIMUM_DATASET_CONTACTS_OPTIONAL; +import static google.registry.model.common.FeatureFlag.FeatureStatus.INACTIVE; import static google.registry.model.eppoutput.Result.Code.SUCCESS; import static google.registry.model.eppoutput.Result.Code.SUCCESS_AND_CLOSE; import static google.registry.model.eppoutput.Result.Code.SUCCESS_WITH_ACTION_PENDING; @@ -40,6 +42,7 @@ import com.google.re2j.Matcher; import com.google.re2j.Pattern; import google.registry.model.billing.BillingBase.Reason; import google.registry.model.billing.BillingEvent; +import google.registry.model.common.FeatureFlag; import google.registry.model.domain.Domain; import google.registry.model.domain.DomainHistory; import google.registry.model.reporting.HistoryEntry.Type; @@ -73,6 +76,12 @@ class EppLifecycleDomainTest extends EppTestCase { @BeforeEach void beforeEach() { + persistResource( + new FeatureFlag() + .asBuilder() + .setFeatureName(MINIMUM_DATASET_CONTACTS_OPTIONAL) + .setStatusMap(ImmutableSortedMap.of(START_OF_TIME, INACTIVE)) + .build()); createTlds("example", "tld"); } diff --git a/core/src/test/java/google/registry/flows/EppLifecycleHostTest.java b/core/src/test/java/google/registry/flows/EppLifecycleHostTest.java index b48fa2272..734e90605 100644 --- a/core/src/test/java/google/registry/flows/EppLifecycleHostTest.java +++ b/core/src/test/java/google/registry/flows/EppLifecycleHostTest.java @@ -16,13 +16,19 @@ package google.registry.flows; import static com.google.common.truth.Truth.assertThat; import static google.registry.model.EppResourceUtils.loadByForeignKey; +import static google.registry.model.common.FeatureFlag.FeatureName.MINIMUM_DATASET_CONTACTS_OPTIONAL; +import static google.registry.model.common.FeatureFlag.FeatureStatus.INACTIVE; import static google.registry.model.eppoutput.Result.Code.SUCCESS; import static google.registry.testing.DatabaseHelper.createTld; import static google.registry.testing.DatabaseHelper.createTlds; +import static google.registry.testing.DatabaseHelper.persistResource; import static google.registry.testing.EppMetricSubject.assertThat; import static google.registry.testing.HostSubject.assertAboutHosts; +import static google.registry.util.DateTimeUtils.START_OF_TIME; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSortedMap; +import google.registry.model.common.FeatureFlag; import google.registry.model.domain.Domain; import google.registry.model.host.Host; import google.registry.persistence.transaction.JpaTestExtensions; @@ -88,6 +94,12 @@ class EppLifecycleHostTest extends EppTestCase { @Test void testRenamingHostToExistingHost_fails() throws Exception { + persistResource( + new FeatureFlag() + .asBuilder() + .setFeatureName(MINIMUM_DATASET_CONTACTS_OPTIONAL) + .setStatusMap(ImmutableSortedMap.of(START_OF_TIME, INACTIVE)) + .build()); createTld("example"); assertThatLoginSucceeds("NewRegistrar", "foo-BAR2"); // Create the fakesite domain. @@ -138,6 +150,12 @@ class EppLifecycleHostTest extends EppTestCase { @Test void testSuccess_multipartTldsWithSharedSuffixes() throws Exception { + persistResource( + new FeatureFlag() + .asBuilder() + .setFeatureName(MINIMUM_DATASET_CONTACTS_OPTIONAL) + .setStatusMap(ImmutableSortedMap.of(START_OF_TIME, INACTIVE)) + .build()); createTlds("bar.foo.tld", "foo.tld", "tld"); assertThatLoginSucceeds("NewRegistrar", "foo-BAR2"); diff --git a/core/src/test/java/google/registry/flows/EppPointInTimeTest.java b/core/src/test/java/google/registry/flows/EppPointInTimeTest.java index 2bb1108e1..b9cae7f09 100644 --- a/core/src/test/java/google/registry/flows/EppPointInTimeTest.java +++ b/core/src/test/java/google/registry/flows/EppPointInTimeTest.java @@ -17,18 +17,24 @@ package google.registry.flows; import static com.google.common.truth.Truth.assertThat; import static google.registry.model.EppResourceUtils.loadAtPointInTime; import static google.registry.model.ImmutableObjectSubject.assertAboutImmutableObjects; +import static google.registry.model.common.FeatureFlag.FeatureName.MINIMUM_DATASET_CONTACTS_OPTIONAL; +import static google.registry.model.common.FeatureFlag.FeatureStatus.INACTIVE; import static google.registry.testing.DatabaseHelper.createTld; import static google.registry.testing.DatabaseHelper.loadAllOf; import static google.registry.testing.DatabaseHelper.loadByEntity; import static google.registry.testing.DatabaseHelper.persistActiveContact; import static google.registry.testing.DatabaseHelper.persistActiveHost; +import static google.registry.testing.DatabaseHelper.persistResource; +import static google.registry.util.DateTimeUtils.START_OF_TIME; import static java.nio.charset.StandardCharsets.UTF_8; import static org.joda.time.DateTimeZone.UTC; import static org.joda.time.Duration.standardDays; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.Iterables; import google.registry.flows.EppTestComponent.FakesAndMocksModule; +import google.registry.model.common.FeatureFlag; import google.registry.model.domain.Domain; import google.registry.monitoring.whitebox.EppMetric; import google.registry.persistence.transaction.JpaTestExtensions; @@ -54,6 +60,12 @@ class EppPointInTimeTest { @BeforeEach void beforeEach() { + persistResource( + new FeatureFlag() + .asBuilder() + .setFeatureName(MINIMUM_DATASET_CONTACTS_OPTIONAL) + .setStatusMap(ImmutableSortedMap.of(START_OF_TIME, INACTIVE)) + .build()); createTld("tld"); } diff --git a/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java b/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java index f78ae631a..a7b2f87e5 100644 --- a/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java +++ b/core/src/test/java/google/registry/flows/domain/DomainCreateFlowTest.java @@ -25,6 +25,9 @@ import static google.registry.model.billing.BillingBase.Flag.SUNRISE; import static google.registry.model.billing.BillingBase.RenewalPriceBehavior.DEFAULT; import static google.registry.model.billing.BillingBase.RenewalPriceBehavior.NONPREMIUM; import static google.registry.model.billing.BillingBase.RenewalPriceBehavior.SPECIFIED; +import static google.registry.model.common.FeatureFlag.FeatureName.MINIMUM_DATASET_CONTACTS_OPTIONAL; +import static google.registry.model.common.FeatureFlag.FeatureStatus.ACTIVE; +import static google.registry.model.common.FeatureFlag.FeatureStatus.INACTIVE; import static google.registry.model.domain.fee.Fee.FEE_EXTENSION_URIS; import static google.registry.model.domain.token.AllocationToken.TokenType.BULK_PRICING; import static google.registry.model.domain.token.AllocationToken.TokenType.DEFAULT_PROMO; @@ -156,6 +159,7 @@ import google.registry.model.billing.BillingBase.Reason; import google.registry.model.billing.BillingBase.RenewalPriceBehavior; import google.registry.model.billing.BillingEvent; import google.registry.model.billing.BillingRecurrence; +import google.registry.model.common.FeatureFlag; import google.registry.model.domain.Domain; import google.registry.model.domain.DomainHistory; import google.registry.model.domain.GracePeriod; @@ -224,7 +228,7 @@ class DomainCreateFlowTest extends ResourceFlowTestCase> nameservers = new ImmutableSet.Builder<>(); for (int i = 1; i < 15; i++) { @@ -1478,6 +1502,27 @@ class DomainUpdateFlowTest extends ResourceFlowTestCasenaturalOrder() + .put(START_OF_TIME, INACTIVE) + .put(fakeClock.nowUtc().plusWeeks(8), ACTIVE) + .build()) + .build()); + assertThat(tm().transact(() -> FeatureFlag.isActiveNow(TEST_FEATURE))).isFalse(); + fakeClock.setTo(DateTime.parse("2011-10-17TZ")); + assertThat(tm().transact(() -> FeatureFlag.isActiveNow(TEST_FEATURE))).isTrue(); + } } diff --git a/core/src/test/java/google/registry/tools/EppLifecycleToolsTest.java b/core/src/test/java/google/registry/tools/EppLifecycleToolsTest.java index 021dd2b2f..2c693e30d 100644 --- a/core/src/test/java/google/registry/tools/EppLifecycleToolsTest.java +++ b/core/src/test/java/google/registry/tools/EppLifecycleToolsTest.java @@ -15,16 +15,22 @@ package google.registry.tools; import static google.registry.model.EppResourceUtils.loadByForeignKey; +import static google.registry.model.common.FeatureFlag.FeatureName.MINIMUM_DATASET_CONTACTS_OPTIONAL; +import static google.registry.model.common.FeatureFlag.FeatureStatus.INACTIVE; import static google.registry.testing.DatabaseHelper.assertBillingEventsForResource; import static google.registry.testing.DatabaseHelper.createTlds; import static google.registry.testing.DatabaseHelper.getOnlyHistoryEntryOfType; +import static google.registry.testing.DatabaseHelper.persistResource; import static google.registry.util.DateTimeUtils.END_OF_TIME; +import static google.registry.util.DateTimeUtils.START_OF_TIME; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSortedMap; import google.registry.flows.EppTestCase; import google.registry.model.billing.BillingBase.Reason; import google.registry.model.billing.BillingEvent; +import google.registry.model.common.FeatureFlag; import google.registry.model.domain.Domain; import google.registry.model.domain.DomainHistory; import google.registry.model.reporting.HistoryEntry.Type; @@ -53,6 +59,12 @@ class EppLifecycleToolsTest extends EppTestCase { @BeforeEach void beforeEach() { + persistResource( + new FeatureFlag() + .asBuilder() + .setFeatureName(MINIMUM_DATASET_CONTACTS_OPTIONAL) + .setStatusMap(ImmutableSortedMap.of(START_OF_TIME, INACTIVE)) + .build()); createTlds("example", "tld"); }