diff --git a/core/src/main/java/google/registry/model/tld/Tld.java b/core/src/main/java/google/registry/model/tld/Tld.java index a6b5de143..94ceb4b5a 100644 --- a/core/src/main/java/google/registry/model/tld/Tld.java +++ b/core/src/main/java/google/registry/model/tld/Tld.java @@ -814,6 +814,55 @@ public class Tld extends ImmutableObject implements Buildable, UnsafeSerializabl return new Builder(clone(this)); } + /** Checks the validity of the TLD object, for use during building or deserializing. */ + public void validateState() { + checkArgument(tldStr != null, "No registry TLD specified"); + // Check for canonical form by converting to an InternetDomainName and then back. + checkArgument( + InternetDomainName.isValid(tldStr) + && tldStr.equals(InternetDomainName.from(tldStr).toString()), + "Cannot create registry for TLD that is not a valid, canonical domain name"); + // Check the validity of all TimedTransitionProperties to ensure that they have values for + // START_INSTANT. The setters above have already checked this for new values, but also check + // here to catch cases where we loaded an invalid TimedTransitionProperty from the database + // and cloned it into a new builder, to block re-building a Tld in an invalid state. + tldStateTransitions.checkValidity(); + createBillingCostTransitions.checkValidity(); + renewBillingCostTransitions.checkValidity(); + eapFeeSchedule.checkValidity(); + // All costs must be in the expected currency. + checkArgumentNotNull(getCurrency(), "Currency must be set"); + Predicate currencyCheck = + (Money money) -> money.getCurrencyUnit().equals(currency) && money.isPositiveOrZero(); + checkArgument( + currencyCheck.test(getRestoreBillingCost()), + "Restore cost is negative or in the wrong currency"); + checkArgument( + currencyCheck.test(getServerStatusChangeBillingCost()), + "Server status change cost is negative or in the wrong currency"); + checkArgument( + currencyCheck.test(getRegistryLockOrUnlockBillingCost()), + "Registry lock/unlock cost is negative or in the wrong currency"); + checkArgument( + getRenewBillingCostTransitions().values().stream().allMatch(currencyCheck), + "Some renew cost(s) are negative or in the wrong currency"); + checkArgument( + getCreateBillingCostTransitions().values().stream().allMatch(currencyCheck), + "Some create cost(s) are negative or in the wrong currency"); + checkArgument( + eapFeeSchedule.toValueMap().values().stream().allMatch(currencyCheck), + "Some EAP fee cost(s) are negative or in the wrong currency'"); + checkArgumentNotNull( + pricingEngineClassName, "All registries must have a configured pricing engine"); + checkArgument( + dnsWriters != null && !dnsWriters.isEmpty(), + "At least one DNS writer must be specified." + + " VoidDnsWriter can be used if DNS writing isn't desired"); + checkArgument( + numDnsPublishLocks > 0, + "Number of DNS publish locks must be positive. Use 1 for TLD-wide locks."); + } + /** A builder for constructing {@link Tld} objects, since they are immutable. */ public static class Builder extends Buildable.Builder { public Builder() {} @@ -966,10 +1015,6 @@ public class Tld extends ImmutableObject implements Buildable, UnsafeSerializabl public Builder setCreateBillingCostTransitions( ImmutableSortedMap createCostsMap) { - checkArgumentNotNull(createCostsMap, "Create billing costs map cannot be null"); - checkArgument( - createCostsMap.values().stream().allMatch(Money::isPositiveOrZero), - "Create billing cost cannot be negative"); getInstance().createBillingCostTransitions = TimedTransitionProperty.fromValueMap(createCostsMap); return this; @@ -1012,17 +1057,12 @@ public class Tld extends ImmutableObject implements Buildable, UnsafeSerializabl } public Builder setRestoreBillingCost(Money amount) { - checkArgument(amount.isPositiveOrZero(), "restoreBillingCost cannot be negative"); getInstance().restoreBillingCost = amount; return this; } public Builder setRenewBillingCostTransitions( ImmutableSortedMap renewCostsMap) { - checkArgumentNotNull(renewCostsMap, "Renew billing costs map cannot be null"); - checkArgument( - renewCostsMap.values().stream().allMatch(Money::isPositiveOrZero), - "Renew billing cost cannot be negative"); getInstance().renewBillingCostTransitions = TimedTransitionProperty.fromValueMap(renewCostsMap); return this; @@ -1030,10 +1070,6 @@ public class Tld extends ImmutableObject implements Buildable, UnsafeSerializabl /** Sets the EAP fee schedule for the TLD. */ public Builder setEapFeeSchedule(ImmutableSortedMap eapFeeSchedule) { - checkArgumentNotNull(eapFeeSchedule, "EAP fee schedule cannot be null"); - checkArgument( - eapFeeSchedule.values().stream().allMatch(Money::isPositiveOrZero), - "EAP fee cannot be negative"); getInstance().eapFeeSchedule = TimedTransitionProperty.fromValueMap(eapFeeSchedule); return this; } @@ -1051,14 +1087,11 @@ public class Tld extends ImmutableObject implements Buildable, UnsafeSerializabl } public Builder setServerStatusChangeBillingCost(Money amount) { - checkArgument( - amount.isPositiveOrZero(), "Server status change billing cost cannot be negative"); getInstance().serverStatusChangeBillingCost = amount; return this; } public Builder setRegistryLockOrUnlockBillingCost(Money amount) { - checkArgument(amount.isPositiveOrZero(), "Registry lock/unlock cost cannot be negative"); getInstance().registryLockOrUnlockBillingCost = amount; return this; } @@ -1120,58 +1153,10 @@ public class Tld extends ImmutableObject implements Buildable, UnsafeSerializabl @Override public Tld build() { - final Tld instance = getInstance(); - // Pick up the name of the associated TLD from the instance object. - String tldName = instance.tldStr; - checkArgument(tldName != null, "No registry TLD specified"); - // Check for canonical form by converting to an InternetDomainName and then back. - checkArgument( - InternetDomainName.isValid(tldName) - && tldName.equals(InternetDomainName.from(tldName).toString()), - "Cannot create registry for TLD that is not a valid, canonical domain name"); - // Check the validity of all TimedTransitionProperties to ensure that they have values for - // START_INSTANT. The setters above have already checked this for new values, but also check - // here to catch cases where we loaded an invalid TimedTransitionProperty from the database - // and cloned it into a new builder, to block re-building a Tld in an invalid state. - instance.tldStateTransitions.checkValidity(); - instance.createBillingCostTransitions.checkValidity(); - instance.renewBillingCostTransitions.checkValidity(); - instance.eapFeeSchedule.checkValidity(); - // All costs must be in the expected currency. - checkArgumentNotNull(instance.getCurrency(), "Currency must be set"); - checkArgument( - instance.getRestoreBillingCost().getCurrencyUnit().equals(instance.currency), - "Restore cost must be in the TLD's currency"); - checkArgument( - instance.getServerStatusChangeBillingCost().getCurrencyUnit().equals(instance.currency), - "Server status change cost must be in the TLD's currency"); - checkArgument( - instance.getRegistryLockOrUnlockBillingCost().getCurrencyUnit().equals(instance.currency), - "Registry lock/unlock cost must be in the TLD's currency"); - Predicate currencyCheck = - (Money money) -> money.getCurrencyUnit().equals(instance.currency); - checkArgument( - instance.getRenewBillingCostTransitions().values().stream().allMatch(currencyCheck), - "Renew cost must be in the TLD's currency"); - checkArgument( - instance.getCreateBillingCostTransitions().values().stream().allMatch(currencyCheck), - "Create cost must be in the TLD's currency"); - checkArgument( - instance.eapFeeSchedule.toValueMap().values().stream().allMatch(currencyCheck), - "All EAP fees must be in the TLD's currency"); - checkArgumentNotNull( - instance.pricingEngineClassName, "All registries must have a configured pricing engine"); - checkArgument( - instance.dnsWriters != null && !instance.dnsWriters.isEmpty(), - "At least one DNS writer must be specified." - + " VoidDnsWriter can be used if DNS writing isn't desired"); // If not set explicitly, numDnsPublishLocks defaults to 1. - instance.setDefaultNumDnsPublishLocks(); - checkArgument( - instance.numDnsPublishLocks > 0, - "Number of DNS publish locks must be positive. Use 1 for TLD-wide locks."); - instance.tldStr = tldName; - instance.tldUnicode = Idn.toUnicode(tldName); + getInstance().setDefaultNumDnsPublishLocks(); + getInstance().validateState(); + getInstance().tldUnicode = Idn.toUnicode(getInstance().tldStr); return super.build(); } } diff --git a/core/src/main/java/google/registry/tools/ConfigureTldCommand.java b/core/src/main/java/google/registry/tools/ConfigureTldCommand.java index 59d141788..959a8e4ac 100644 --- a/core/src/main/java/google/registry/tools/ConfigureTldCommand.java +++ b/core/src/main/java/google/registry/tools/ConfigureTldCommand.java @@ -164,6 +164,8 @@ public class ConfigureTldCommand extends MutatingCommand { if (Boolean.TRUE.equals(breakGlass)) { newTld = newTld.asBuilder().setBreakglassMode(true).build(); } + // Enforce any restrictions, e.g. "no negative fees" + newTld.validateState(); stageEntityChange(oldTld, newTld); } diff --git a/core/src/test/java/google/registry/model/tld/TldTest.java b/core/src/test/java/google/registry/model/tld/TldTest.java index ffb94d84b..b721f62ae 100644 --- a/core/src/test/java/google/registry/model/tld/TldTest.java +++ b/core/src/test/java/google/registry/model/tld/TldTest.java @@ -257,7 +257,8 @@ public final class TldTest extends EntityTestCase { .asBuilder() .setCreateBillingCostTransitions(createCostTransitions) .build()); - assertThat(thrown.getMessage()).isEqualTo("Create billing cost cannot be negative"); + assertThat(thrown.getMessage()) + .isEqualTo("Some create cost(s) are negative or in the wrong currency"); } @Test @@ -614,8 +615,11 @@ public final class TldTest extends EntityTestCase { Tld.get("tld") .asBuilder() .setRenewBillingCostTransitions( - ImmutableSortedMap.of(START_INSTANT, Money.of(USD, -42)))); - assertThat(thrown).hasMessageThat().contains("billing cost cannot be negative"); + ImmutableSortedMap.of(START_INSTANT, Money.of(USD, -42))) + .build()); + assertThat(thrown) + .hasMessageThat() + .isEqualTo("Some renew cost(s) are negative or in the wrong currency"); } @Test @@ -623,8 +627,10 @@ public final class TldTest extends EntityTestCase { IllegalArgumentException thrown = assertThrows( IllegalArgumentException.class, - () -> Tld.get("tld").asBuilder().setRestoreBillingCost(Money.of(USD, -42))); - assertThat(thrown).hasMessageThat().contains("restoreBillingCost cannot be negative"); + () -> Tld.get("tld").asBuilder().setRestoreBillingCost(Money.of(USD, -42)).build()); + assertThat(thrown) + .hasMessageThat() + .isEqualTo("Restore cost is negative or in the wrong currency"); } @Test @@ -652,8 +658,14 @@ public final class TldTest extends EntityTestCase { IllegalArgumentException thrown = assertThrows( IllegalArgumentException.class, - () -> Tld.get("tld").asBuilder().setServerStatusChangeBillingCost(Money.of(USD, -42))); - assertThat(thrown).hasMessageThat().contains("billing cost cannot be negative"); + () -> + Tld.get("tld") + .asBuilder() + .setServerStatusChangeBillingCost(Money.of(USD, -42)) + .build()); + assertThat(thrown) + .hasMessageThat() + .isEqualTo("Server status change cost is negative or in the wrong currency"); } @Test @@ -667,7 +679,9 @@ public final class TldTest extends EntityTestCase { .setRenewBillingCostTransitions( ImmutableSortedMap.of(START_INSTANT, Money.of(EUR, 42))) .build()); - assertThat(thrown).hasMessageThat().contains("cost must be in the TLD's currency"); + assertThat(thrown) + .hasMessageThat() + .isEqualTo("Some renew cost(s) are negative or in the wrong currency"); } @Test @@ -681,7 +695,7 @@ public final class TldTest extends EntityTestCase { .setCreateBillingCostTransitions( ImmutableSortedMap.of(START_INSTANT, Money.of(EUR, 42))) .build()); - assertThat(thrown).hasMessageThat().contains("cost must be in the TLD's currency"); + assertThat(thrown).hasMessageThat().contains("negative or in the wrong currency"); } @Test @@ -690,7 +704,7 @@ public final class TldTest extends EntityTestCase { assertThrows( IllegalArgumentException.class, () -> Tld.get("tld").asBuilder().setRestoreBillingCost(Money.of(EUR, 42)).build()); - assertThat(thrown).hasMessageThat().contains("cost must be in the TLD's currency"); + assertThat(thrown).hasMessageThat().contains("negative or in the wrong currency"); } @Test @@ -703,7 +717,7 @@ public final class TldTest extends EntityTestCase { .asBuilder() .setServerStatusChangeBillingCost(Money.of(EUR, 42)) .build()); - assertThat(thrown).hasMessageThat().contains("cost must be in the TLD's currency"); + assertThat(thrown).hasMessageThat().contains("negative or in the wrong currency"); } @Test @@ -742,7 +756,7 @@ public final class TldTest extends EntityTestCase { .asBuilder() .setEapFeeSchedule(ImmutableSortedMap.of(START_INSTANT, Money.zero(EUR))) .build()); - assertThat(thrown).hasMessageThat().contains("All EAP fees must be in the TLD's currency"); + assertThat(thrown).hasMessageThat().contains("negative or in the wrong currency"); } @Test diff --git a/core/src/test/java/google/registry/tools/ConfigureTldCommandTest.java b/core/src/test/java/google/registry/tools/ConfigureTldCommandTest.java index f4758b6b9..ee2c35c11 100644 --- a/core/src/test/java/google/registry/tools/ConfigureTldCommandTest.java +++ b/core/src/test/java/google/registry/tools/ConfigureTldCommandTest.java @@ -454,6 +454,16 @@ public class ConfigureTldCommandTest extends CommandTestCase runCommandForced("--input=" + tldFile)); + assertThat(thrown.getMessage()) + .isEqualTo("Some create cost(s) are negative or in the wrong currency"); + } + @Test void testSuccess_emptyStringClearsDefaultPromoTokens() throws Exception { Tld tld = createTld("tld"); diff --git a/core/src/test/resources/google/registry/tools/negativecost.yaml b/core/src/test/resources/google/registry/tools/negativecost.yaml new file mode 100644 index 000000000..c2a7654a5 --- /dev/null +++ b/core/src/test/resources/google/registry/tools/negativecost.yaml @@ -0,0 +1,58 @@ +addGracePeriodLength: "PT120H" +allowedFullyQualifiedHostNames: [] +anchorTenantAddGracePeriodLength: "PT720H" +autoRenewGracePeriodLength: "PT1080H" +automaticTransferLength: "PT120H" +claimsPeriodEnd: "294247-01-10T04:00:54.775Z" +createBillingCostTransitions: + "1970-01-01T00:00:00.000Z": + currency: "USD" + amount: 8.00 + "2020-01-01T00:00:00.000Z": + currency: "USD" + amount: -25.00 +creationTime: "2022-09-01T00:00:00.000Z" +currency: "USD" +defaultPromoTokens: [] +dnsAPlusAaaaTtl: "PT15M" +dnsDsTtl: null +dnsNsTtl: null +dnsPaused: false +dnsWriters: +- "VoidDnsWriter" +driveFolderId: "driveFolder" +eapFeeSchedule: + "1970-01-01T00:00:00.000Z": + currency: "USD" + amount: 0.00 +escrowEnabled: false +idnTables: [] +invoicingEnabled: false +lordnUsername: null +numDnsPublishLocks: 1 +pendingDeleteLength: "PT120H" +premiumListName: "test" +pricingEngineClassName: "google.registry.model.pricing.StaticPremiumListPricingEngine" +redemptionGracePeriodLength: "PT720H" +registryLockOrUnlockBillingCost: + currency: "USD" + amount: 0.00 +renewBillingCostTransitions: + "1970-01-01T00:00:00.000Z": + currency: "USD" + amount: 11.00 +renewGracePeriodLength: "PT120H" +reservedListNames: [] +restoreBillingCost: + currency: "USD" + amount: 17.00 +roidSuffix: "TLD" +serverStatusChangeBillingCost: + currency: "USD" + amount: 19.00 +tldStateTransitions: + "1970-01-01T00:00:00.000Z": "GENERAL_AVAILABILITY" +tldStr: "negativecost" +tldType: "REAL" +tldUnicode: "negativecost" +transferGracePeriodLength: "PT120H" diff --git a/core/src/test/resources/google/registry/tools/nullablefieldsallnull.yaml b/core/src/test/resources/google/registry/tools/nullablefieldsallnull.yaml index 84109eb7f..a585d3b24 100644 --- a/core/src/test/resources/google/registry/tools/nullablefieldsallnull.yaml +++ b/core/src/test/resources/google/registry/tools/nullablefieldsallnull.yaml @@ -16,6 +16,7 @@ tldStateTransitions: creationTime: "2022-09-01T00:00:00.000Z" reservedListNames: null premiumListName: null +pricingEngineClassName: "google.registry.model.pricing.StaticPremiumListPricingEngine" escrowEnabled: false dnsPaused: false addGracePeriodLength: "PT720H"