From b7ce08dfdca989d8943a6c623b4e8737f4049c84 Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Fri, 25 Jun 2021 19:10:21 -0400 Subject: [PATCH] Fix BigDecimal precision of PremiumList.getLabelsToPrices() (#1221) * Fix BigDecimal precision of PremiumList.getLabelsToPrices() Different currencies have different numbers of decimal places (e.g. USD has 2, JPY has 0, and some even have 3). Thus, when loading the contents of a premium list, we need to set the precision correctly on all of the BigDecimal prices. This issue was introduced as part of the Registry 3.0 database migration when we changed each PremiumEntry to being a Money to a BigDecimal (to remove the redundancy of storing the same currency value over and over). --- .../model/registry/label/PremiumList.java | 14 ++++- .../registry/schema/tld/PremiumListDao.java | 7 +-- .../model/registry/label/PremiumListTest.java | 29 ++++++++++ .../schema/tld/PremiumListDaoTest.java | 57 +++++++++++++------ 4 files changed, 83 insertions(+), 24 deletions(-) diff --git a/core/src/main/java/google/registry/model/registry/label/PremiumList.java b/core/src/main/java/google/registry/model/registry/label/PremiumList.java index bbf9f25ab..2aab11055 100644 --- a/core/src/main/java/google/registry/model/registry/label/PremiumList.java +++ b/core/src/main/java/google/registry/model/registry/label/PremiumList.java @@ -44,6 +44,7 @@ import google.registry.schema.tld.PremiumListDao; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.math.BigDecimal; +import java.math.RoundingMode; import java.util.List; import java.util.Map; import java.util.Objects; @@ -183,11 +184,22 @@ public final class PremiumList extends BaseDomainLabelList convertAmountToMoney(entry.getPrice()).getAmount())); } return labelsToPrices; } + /** + * Converts a raw {@link BigDecimal} amount to a {@link Money} by applying the list's currency. + */ + public Money convertAmountToMoney(BigDecimal amount) { + return Money.of(currency, amount.setScale(currency.getDecimalPlaces(), RoundingMode.HALF_EVEN)); + } + /** * Returns a Bloom filter to determine whether a label might be premium, or is definitely not. * diff --git a/core/src/main/java/google/registry/schema/tld/PremiumListDao.java b/core/src/main/java/google/registry/schema/tld/PremiumListDao.java index 782e10aed..c98e0e990 100644 --- a/core/src/main/java/google/registry/schema/tld/PremiumListDao.java +++ b/core/src/main/java/google/registry/schema/tld/PremiumListDao.java @@ -143,12 +143,7 @@ public class PremiumListDao { RevisionIdAndLabel revisionIdAndLabel = RevisionIdAndLabel.create(loadedList.getRevisionId(), label); try { - Optional price = premiumEntryCache.get(revisionIdAndLabel); - return price.map( - p -> - Money.of( - loadedList.getCurrency(), - p.setScale(loadedList.getCurrency().getDecimalPlaces()))); + return premiumEntryCache.get(revisionIdAndLabel).map(loadedList::convertAmountToMoney); } catch (InvalidCacheLoadException | ExecutionException e) { throw new RuntimeException( String.format( diff --git a/core/src/test/java/google/registry/model/registry/label/PremiumListTest.java b/core/src/test/java/google/registry/model/registry/label/PremiumListTest.java index d1243bf69..760b0005a 100644 --- a/core/src/test/java/google/registry/model/registry/label/PremiumListTest.java +++ b/core/src/test/java/google/registry/model/registry/label/PremiumListTest.java @@ -20,6 +20,8 @@ import static google.registry.testing.DatabaseHelper.createTld; import static google.registry.testing.DatabaseHelper.persistPremiumList; import static google.registry.testing.DatabaseHelper.persistReservedList; import static google.registry.testing.DatabaseHelper.persistResource; +import static org.joda.money.CurrencyUnit.JPY; +import static org.joda.money.CurrencyUnit.USD; import static org.junit.jupiter.api.Assertions.assertThrows; import com.google.common.collect.ImmutableList; @@ -28,6 +30,7 @@ import google.registry.model.registry.Registry; import google.registry.model.registry.label.PremiumList.PremiumListEntry; import google.registry.schema.tld.PremiumListDao; import google.registry.testing.AppEngineExtension; +import java.math.BigDecimal; import org.joda.money.Money; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -125,4 +128,30 @@ public class PremiumListTest { .build()); assertThat(e).hasMessageThat().contains("must be in puny-coded, lower-case form"); } + + @Test + void testConvertAmountToMoney_USD() { + PremiumList premiumList = new PremiumList.Builder().setName("foo").setCurrency(USD).build(); + assertThat(premiumList.convertAmountToMoney(new BigDecimal("20.000"))) + .isEqualTo(Money.of(USD, new BigDecimal("20.00"))); + assertThat(premiumList.convertAmountToMoney(new BigDecimal("37"))) + .isEqualTo(Money.of(USD, new BigDecimal("37.00"))); + assertThat(premiumList.convertAmountToMoney(new BigDecimal("42.5"))) + .isEqualTo(Money.of(USD, new BigDecimal("42.50"))); + assertThat(premiumList.convertAmountToMoney(new BigDecimal("15.678"))) + .isEqualTo(Money.of(USD, new BigDecimal("15.68"))); + } + + @Test + void testConvertAmountToMoney_JPY() { + PremiumList premiumList = new PremiumList.Builder().setName("foo").setCurrency(JPY).build(); + assertThat(premiumList.convertAmountToMoney(new BigDecimal("20.000"))) + .isEqualTo(Money.of(JPY, new BigDecimal("20"))); + assertThat(premiumList.convertAmountToMoney(new BigDecimal("37"))) + .isEqualTo(Money.of(JPY, new BigDecimal("37"))); + assertThat(premiumList.convertAmountToMoney(new BigDecimal("42.5"))) + .isEqualTo(Money.of(JPY, new BigDecimal("42"))); + assertThat(premiumList.convertAmountToMoney(new BigDecimal("15.678"))) + .isEqualTo(Money.of(JPY, new BigDecimal("16"))); + } } diff --git a/core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java b/core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java index 763d10e84..14e6074c2 100644 --- a/core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java +++ b/core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java @@ -60,25 +60,24 @@ public class PremiumListDaoTest { .withPremiumListsCache(standardDays(1)) .build(); - private ImmutableMap testPrices; + private static final ImmutableMap TEST_PRICES = + ImmutableMap.of( + "silver", + BigDecimal.valueOf(10.23), + "gold", + BigDecimal.valueOf(1305.47), + "palladium", + BigDecimal.valueOf(1552.78)); private PremiumList testList; @BeforeEach void beforeEach() { - testPrices = - ImmutableMap.of( - "silver", - BigDecimal.valueOf(10.23), - "gold", - BigDecimal.valueOf(1305.47), - "palladium", - BigDecimal.valueOf(1552.78)); testList = new PremiumList.Builder() .setName("testname") .setCurrency(USD) - .setLabelsToPrices(testPrices) + .setLabelsToPrices(TEST_PRICES) .setCreationTime(fakeClock.nowUtc()) .build(); } @@ -92,7 +91,7 @@ public class PremiumListDaoTest { Optional persistedListOpt = PremiumListDao.getLatestRevision("testname"); assertThat(persistedListOpt).isPresent(); PremiumList persistedList = persistedListOpt.get(); - assertThat(persistedList.getLabelsToPrices()).containsExactlyEntriesIn(testPrices); + assertThat(persistedList.getLabelsToPrices()).containsExactlyEntriesIn(TEST_PRICES); assertThat(persistedList.getCreationTime()).isEqualTo(fakeClock.nowUtc()); }); } @@ -155,15 +154,15 @@ public class PremiumListDaoTest { PremiumListDao.save( new PremiumList.Builder() .setName("list1") - .setCurrency(JPY) + .setCurrency(USD) .setLabelsToPrices(ImmutableMap.of("wrong", BigDecimal.valueOf(1000.50))) .setCreationTime(fakeClock.nowUtc()) .build()); PremiumListDao.save( new PremiumList.Builder() .setName("list1") - .setCurrency(JPY) - .setLabelsToPrices(testPrices) + .setCurrency(USD) + .setLabelsToPrices(TEST_PRICES) .setCreationTime(fakeClock.nowUtc()) .build()); jpaTm() @@ -172,9 +171,33 @@ public class PremiumListDaoTest { Optional persistedList = PremiumListDao.getLatestRevision("list1"); assertThat(persistedList).isPresent(); assertThat(persistedList.get().getName()).isEqualTo("list1"); - assertThat(persistedList.get().getCurrency()).isEqualTo(JPY); + assertThat(persistedList.get().getCurrency()).isEqualTo(USD); assertThat(persistedList.get().getLabelsToPrices()) - .containsExactlyEntriesIn(testPrices); + .containsExactlyEntriesIn(TEST_PRICES); + }); + } + + @Test + void getLabelsToPrices_worksForJpy() { + PremiumListDao.save( + new PremiumList.Builder() + .setName("list1") + .setCurrency(JPY) + .setLabelsToPrices(TEST_PRICES) + .setCreationTime(fakeClock.nowUtc()) + .build()); + jpaTm() + .transact( + () -> { + PremiumList premiumList = PremiumListDao.getLatestRevision("list1").get(); + assertThat(premiumList.getLabelsToPrices()) + .containsExactly( + "silver", + BigDecimal.valueOf(10), + "gold", + BigDecimal.valueOf(1305), + "palladium", + BigDecimal.valueOf(1553)); }); } @@ -193,7 +216,7 @@ public class PremiumListDaoTest { new PremiumList.Builder() .setName("premlist") .setCurrency(USD) - .setLabelsToPrices(testPrices) + .setLabelsToPrices(TEST_PRICES) .setCreationTime(fakeClock.nowUtc()) .build()); assertThat(PremiumListDao.getPremiumPrice("premlist", "silver")).hasValue(Money.of(USD, 10.23));