From ea24f6ca31b16498a794f12fc5410c07559704af Mon Sep 17 00:00:00 2001 From: mcilwain Date: Wed, 3 Aug 2016 08:08:27 -0700 Subject: [PATCH] Use string keys for the multimap of pricing engines for TLDs This is better than the previous way of using the canonical name of the class, because the previous way did not allow for refactoring, and also required the PremiumPricingEngine to live in the model package lest there be circular dependencies, which does not seem ideal. Note that, for reasons of backwards compatibility with existing persisted data, the name of the static premium pricing engine has been set to its canonical class name, but the class can now be refactored going forward so long as this string remains unchanged, and any new pricing engine implementations can use whatever string key they want (it doesn't have to be a canonical class name). ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=129215185 --- .../StaticPremiumListPricingEngine.java | 3 +++ .../registry/model/registry/Registry.java | 16 ++++++++-------- .../registry/pricing/PricingComponent.java | 2 +- .../registry/pricing/PricingEngineProxy.java | 19 ++----------------- .../registry/pricing/PricingModule.java | 15 ++++----------- .../tools/CreateOrUpdateTldCommand.java | 2 +- .../model/registry/label/PremiumListTest.java | 2 +- .../pricing/PricingEngineProxyTest.java | 13 +++++-------- .../registry/testing/DatastoreHelper.java | 2 +- 9 files changed, 26 insertions(+), 48 deletions(-) diff --git a/java/google/registry/model/pricing/StaticPremiumListPricingEngine.java b/java/google/registry/model/pricing/StaticPremiumListPricingEngine.java index 18a8bd9e3..e9786e40d 100644 --- a/java/google/registry/model/pricing/StaticPremiumListPricingEngine.java +++ b/java/google/registry/model/pricing/StaticPremiumListPricingEngine.java @@ -34,6 +34,9 @@ import org.joda.time.DateTime; /** A premium list pricing engine that stores static pricing information in Datastore entities. */ public final class StaticPremiumListPricingEngine implements PremiumPricingEngine { + /** The name of the pricing engine, as used in {@code Registry.pricingEngineClassName}. */ + public static final String NAME = "google.registry.model.pricing.StaticPremiumListPricingEngine"; + @Inject StaticPremiumListPricingEngine() {} @Override diff --git a/java/google/registry/model/registry/Registry.java b/java/google/registry/model/registry/Registry.java index faf93bc65..b0e8de742 100644 --- a/java/google/registry/model/registry/Registry.java +++ b/java/google/registry/model/registry/Registry.java @@ -58,7 +58,6 @@ import google.registry.model.common.EntityGroupRoot; import google.registry.model.common.TimedTransitionProperty; import google.registry.model.common.TimedTransitionProperty.TimedTransition; import google.registry.model.domain.fee.EapFee; -import google.registry.model.pricing.PremiumPricingEngine; import google.registry.model.pricing.StaticPremiumListPricingEngine; import google.registry.model.registry.label.PremiumList; import google.registry.model.registry.label.ReservedList; @@ -252,15 +251,18 @@ public class Registry extends ImmutableObject implements Buildable { @OnLoad void backfillPricingEngine() { if (pricingEngineClassName == null) { - pricingEngineClassName = StaticPremiumListPricingEngine.class.getCanonicalName(); + pricingEngineClassName = StaticPremiumListPricingEngine.NAME; } } /** - * The fully qualified canonical classname of the pricing engine that this TLD uses. + * The name of the pricing engine that this TLD uses. * *

This must be a valid key for the map of pricing engines injected by - * @Inject Map + * {@code @Inject Map}. + * + *

Note that it used to be the canonical class name, hence the name of this field, but this + * restriction has since been relaxed and it may now be any unique string. */ String pricingEngineClassName; @@ -609,10 +611,8 @@ public class Registry extends ImmutableObject implements Buildable { return this; } - public Builder setPremiumPricingEngineClass( - Class pricingEngineClass) { - getInstance().pricingEngineClassName = - checkArgumentNotNull(pricingEngineClass).getCanonicalName(); + public Builder setPremiumPricingEngine(String pricingEngineClass) { + getInstance().pricingEngineClassName = checkArgumentNotNull(pricingEngineClass); return this; } diff --git a/java/google/registry/pricing/PricingComponent.java b/java/google/registry/pricing/PricingComponent.java index a57f1e6a2..ef190b485 100644 --- a/java/google/registry/pricing/PricingComponent.java +++ b/java/google/registry/pricing/PricingComponent.java @@ -29,5 +29,5 @@ import javax.inject.Singleton; @Singleton @Component(modules = {PricingModule.class}) interface PricingComponent { - Map, PremiumPricingEngine> premiumPricingEngines(); + Map premiumPricingEngines(); } diff --git a/java/google/registry/pricing/PricingEngineProxy.java b/java/google/registry/pricing/PricingEngineProxy.java index 136521646..20f77ad11 100644 --- a/java/google/registry/pricing/PricingEngineProxy.java +++ b/java/google/registry/pricing/PricingEngineProxy.java @@ -18,9 +18,6 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; import static google.registry.util.DomainNameUtils.getTldFromDomainName; -import com.google.common.base.Function; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Maps; import google.registry.model.pricing.PremiumPricingEngine; import google.registry.model.pricing.PremiumPricingEngine.DomainPrices; import google.registry.model.registry.Registry; @@ -34,20 +31,8 @@ import org.joda.time.DateTime; */ public final class PricingEngineProxy { - private static final Map, PremiumPricingEngine> - premiumPricingEngineClasses = DaggerPricingComponent.create().premiumPricingEngines(); - - // Dagger map keys have to be provided with constant values that are known at compile time, so it - // can't be done using clazz.getCanonicalName(). So we construct the map by canonical name here, - // at runtime. - private static final ImmutableMap premiumPricingEngines = - Maps.uniqueIndex( - premiumPricingEngineClasses.values(), - new Function() { - @Override - public String apply(PremiumPricingEngine pricingEngine) { - return pricingEngine.getClass().getCanonicalName(); - }}); + private static final Map + premiumPricingEngines = DaggerPricingComponent.create().premiumPricingEngines(); /** Returns the billing cost for registering the specified domain name for this many years. */ public static Money getDomainCreateCost(String domainName, DateTime priceTime, int years) { diff --git a/java/google/registry/pricing/PricingModule.java b/java/google/registry/pricing/PricingModule.java index 3eb260f7c..36f4b9031 100644 --- a/java/google/registry/pricing/PricingModule.java +++ b/java/google/registry/pricing/PricingModule.java @@ -14,10 +14,10 @@ package google.registry.pricing; -import dagger.MapKey; import dagger.Module; import dagger.Provides; import dagger.multibindings.IntoMap; +import dagger.multibindings.StringKey; import google.registry.model.pricing.PremiumPricingEngine; import google.registry.model.pricing.StaticPremiumListPricingEngine; @@ -25,21 +25,14 @@ import google.registry.model.pricing.StaticPremiumListPricingEngine; * Dagger module for injecting pricing engines. * *

To add a new pricing engine, create a new class that implements {@link PremiumPricingEngine}, - * and add a module that provides an instance of {@link PremiumPricingEngine} with a - * {@link PremiumPricingEngineClassKey} annotation with the class of the implementation and also - * @Provides @IntoMap annotations. + * and add a module that provides an instance of {@link PremiumPricingEngine} with a unique + * {@link StringKey} annotation, and also @Provides @IntoMap annotations. */ @Module public class PricingModule { - /** The annotation used for PricingEngine implementation keys. */ - @MapKey - @interface PremiumPricingEngineClassKey { - Class value(); - } - @Provides @IntoMap - @PremiumPricingEngineClassKey(StaticPremiumListPricingEngine.class) + @StringKey(StaticPremiumListPricingEngine.NAME) static PremiumPricingEngine provideStaticPremiumList(StaticPremiumListPricingEngine engine) { return engine; } diff --git a/java/google/registry/tools/CreateOrUpdateTldCommand.java b/java/google/registry/tools/CreateOrUpdateTldCommand.java index c0788b66f..63018a38c 100644 --- a/java/google/registry/tools/CreateOrUpdateTldCommand.java +++ b/java/google/registry/tools/CreateOrUpdateTldCommand.java @@ -252,7 +252,7 @@ abstract class CreateOrUpdateTldCommand extends MutatingCommand { oldRegistry == null ? new Registry.Builder() .setTldStr(tld) - .setPremiumPricingEngineClass(StaticPremiumListPricingEngine.class) + .setPremiumPricingEngine(StaticPremiumListPricingEngine.NAME) : oldRegistry.asBuilder(); if (escrow != null) { diff --git a/javatests/google/registry/model/registry/label/PremiumListTest.java b/javatests/google/registry/model/registry/label/PremiumListTest.java index 4754f4b28..43ce9fe62 100644 --- a/javatests/google/registry/model/registry/label/PremiumListTest.java +++ b/javatests/google/registry/model/registry/label/PremiumListTest.java @@ -70,7 +70,7 @@ public class PremiumListTest { persistResource( new Registry.Builder() .setTldStr("ghost") - .setPremiumPricingEngineClass(StaticPremiumListPricingEngine.class) + .setPremiumPricingEngine(StaticPremiumListPricingEngine.NAME) .build()); assertThat(Registry.get("ghost").getPremiumList()).isNull(); assertThat(getPremiumPrice("blah", "ghost")).isAbsent(); diff --git a/javatests/google/registry/pricing/PricingEngineProxyTest.java b/javatests/google/registry/pricing/PricingEngineProxyTest.java index bd671098d..5f86e3659 100644 --- a/javatests/google/registry/pricing/PricingEngineProxyTest.java +++ b/javatests/google/registry/pricing/PricingEngineProxyTest.java @@ -25,7 +25,6 @@ import static google.registry.util.DateTimeUtils.START_OF_TIME; import static org.joda.money.CurrencyUnit.USD; import com.google.common.collect.ImmutableSortedMap; -import google.registry.model.pricing.PremiumPricingEngine; import google.registry.model.registry.Registry; import google.registry.model.registry.label.PremiumList; import google.registry.testing.AppEngineRule; @@ -145,14 +144,12 @@ public class PricingEngineProxyTest { public void testFailure_cantLoadPricingEngine() throws Exception { createTld("example"); persistResource( - Registry.get("example").asBuilder().setPremiumPricingEngineClass(FakePricingEngine.class).build()); + Registry.get("example") + .asBuilder() + .setPremiumPricingEngine("fake") + .build()); thrown.expect( - IllegalStateException.class, - String.format( - "Could not load pricing engine %s for TLD example", - FakePricingEngine.class.getCanonicalName())); + IllegalStateException.class, "Could not load pricing engine fake for TLD example"); getDomainCreateCost("bad.example", clock.nowUtc(), 1); } - - private abstract static class FakePricingEngine implements PremiumPricingEngine {} } diff --git a/javatests/google/registry/testing/DatastoreHelper.java b/javatests/google/registry/testing/DatastoreHelper.java index be50b90d9..fa00e5a25 100644 --- a/javatests/google/registry/testing/DatastoreHelper.java +++ b/javatests/google/registry/testing/DatastoreHelper.java @@ -233,7 +233,7 @@ public class DatastoreHelper { .setServerStatusChangeBillingCost(Money.of(USD, 19)) // Always set a default premium list. Tests that don't want it can delete it. .setPremiumList(persistPremiumList(tld, DEFAULT_PREMIUM_LIST_CONTENTS.get())) - .setPremiumPricingEngineClass(StaticPremiumListPricingEngine.class) + .setPremiumPricingEngine(StaticPremiumListPricingEngine.NAME) .build(); }