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).
This commit is contained in:
Ben McIlwain 2021-06-25 19:10:21 -04:00 committed by GitHub
parent a3e8bf219f
commit b7ce08dfdc
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 83 additions and 24 deletions

View file

@ -44,6 +44,7 @@ import google.registry.schema.tld.PremiumListDao;
import java.io.ByteArrayOutputStream; import java.io.ByteArrayOutputStream;
import java.io.IOException; import java.io.IOException;
import java.math.BigDecimal; import java.math.BigDecimal;
import java.math.RoundingMode;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import java.util.Objects; import java.util.Objects;
@ -183,11 +184,22 @@ public final class PremiumList extends BaseDomainLabelList<Money, PremiumList.Pr
.createQueryComposer(PremiumEntry.class) .createQueryComposer(PremiumEntry.class)
.where("revisionId", EQ, revisionId) .where("revisionId", EQ, revisionId)
.stream() .stream()
.collect(toImmutableMap(PremiumEntry::getDomainLabel, PremiumEntry::getPrice)); .collect(
toImmutableMap(
PremiumEntry::getDomainLabel,
// Set the correct amount of precision for the premium list's currency.
entry -> convertAmountToMoney(entry.getPrice()).getAmount()));
} }
return labelsToPrices; 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. * Returns a Bloom filter to determine whether a label might be premium, or is definitely not.
* *

View file

@ -143,12 +143,7 @@ public class PremiumListDao {
RevisionIdAndLabel revisionIdAndLabel = RevisionIdAndLabel revisionIdAndLabel =
RevisionIdAndLabel.create(loadedList.getRevisionId(), label); RevisionIdAndLabel.create(loadedList.getRevisionId(), label);
try { try {
Optional<BigDecimal> price = premiumEntryCache.get(revisionIdAndLabel); return premiumEntryCache.get(revisionIdAndLabel).map(loadedList::convertAmountToMoney);
return price.map(
p ->
Money.of(
loadedList.getCurrency(),
p.setScale(loadedList.getCurrency().getDecimalPlaces())));
} catch (InvalidCacheLoadException | ExecutionException e) { } catch (InvalidCacheLoadException | ExecutionException e) {
throw new RuntimeException( throw new RuntimeException(
String.format( String.format(

View file

@ -20,6 +20,8 @@ import static google.registry.testing.DatabaseHelper.createTld;
import static google.registry.testing.DatabaseHelper.persistPremiumList; import static google.registry.testing.DatabaseHelper.persistPremiumList;
import static google.registry.testing.DatabaseHelper.persistReservedList; import static google.registry.testing.DatabaseHelper.persistReservedList;
import static google.registry.testing.DatabaseHelper.persistResource; 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 static org.junit.jupiter.api.Assertions.assertThrows;
import com.google.common.collect.ImmutableList; 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.model.registry.label.PremiumList.PremiumListEntry;
import google.registry.schema.tld.PremiumListDao; import google.registry.schema.tld.PremiumListDao;
import google.registry.testing.AppEngineExtension; import google.registry.testing.AppEngineExtension;
import java.math.BigDecimal;
import org.joda.money.Money; import org.joda.money.Money;
import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Test;
@ -125,4 +128,30 @@ public class PremiumListTest {
.build()); .build());
assertThat(e).hasMessageThat().contains("must be in puny-coded, lower-case form"); 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")));
}
} }

View file

@ -60,25 +60,24 @@ public class PremiumListDaoTest {
.withPremiumListsCache(standardDays(1)) .withPremiumListsCache(standardDays(1))
.build(); .build();
private ImmutableMap<String, BigDecimal> testPrices; private static final ImmutableMap<String, BigDecimal> TEST_PRICES =
ImmutableMap.of(
"silver",
BigDecimal.valueOf(10.23),
"gold",
BigDecimal.valueOf(1305.47),
"palladium",
BigDecimal.valueOf(1552.78));
private PremiumList testList; private PremiumList testList;
@BeforeEach @BeforeEach
void beforeEach() { void beforeEach() {
testPrices =
ImmutableMap.of(
"silver",
BigDecimal.valueOf(10.23),
"gold",
BigDecimal.valueOf(1305.47),
"palladium",
BigDecimal.valueOf(1552.78));
testList = testList =
new PremiumList.Builder() new PremiumList.Builder()
.setName("testname") .setName("testname")
.setCurrency(USD) .setCurrency(USD)
.setLabelsToPrices(testPrices) .setLabelsToPrices(TEST_PRICES)
.setCreationTime(fakeClock.nowUtc()) .setCreationTime(fakeClock.nowUtc())
.build(); .build();
} }
@ -92,7 +91,7 @@ public class PremiumListDaoTest {
Optional<PremiumList> persistedListOpt = PremiumListDao.getLatestRevision("testname"); Optional<PremiumList> persistedListOpt = PremiumListDao.getLatestRevision("testname");
assertThat(persistedListOpt).isPresent(); assertThat(persistedListOpt).isPresent();
PremiumList persistedList = persistedListOpt.get(); PremiumList persistedList = persistedListOpt.get();
assertThat(persistedList.getLabelsToPrices()).containsExactlyEntriesIn(testPrices); assertThat(persistedList.getLabelsToPrices()).containsExactlyEntriesIn(TEST_PRICES);
assertThat(persistedList.getCreationTime()).isEqualTo(fakeClock.nowUtc()); assertThat(persistedList.getCreationTime()).isEqualTo(fakeClock.nowUtc());
}); });
} }
@ -155,15 +154,15 @@ public class PremiumListDaoTest {
PremiumListDao.save( PremiumListDao.save(
new PremiumList.Builder() new PremiumList.Builder()
.setName("list1") .setName("list1")
.setCurrency(JPY) .setCurrency(USD)
.setLabelsToPrices(ImmutableMap.of("wrong", BigDecimal.valueOf(1000.50))) .setLabelsToPrices(ImmutableMap.of("wrong", BigDecimal.valueOf(1000.50)))
.setCreationTime(fakeClock.nowUtc()) .setCreationTime(fakeClock.nowUtc())
.build()); .build());
PremiumListDao.save( PremiumListDao.save(
new PremiumList.Builder() new PremiumList.Builder()
.setName("list1") .setName("list1")
.setCurrency(JPY) .setCurrency(USD)
.setLabelsToPrices(testPrices) .setLabelsToPrices(TEST_PRICES)
.setCreationTime(fakeClock.nowUtc()) .setCreationTime(fakeClock.nowUtc())
.build()); .build());
jpaTm() jpaTm()
@ -172,9 +171,33 @@ public class PremiumListDaoTest {
Optional<PremiumList> persistedList = PremiumListDao.getLatestRevision("list1"); Optional<PremiumList> persistedList = PremiumListDao.getLatestRevision("list1");
assertThat(persistedList).isPresent(); assertThat(persistedList).isPresent();
assertThat(persistedList.get().getName()).isEqualTo("list1"); assertThat(persistedList.get().getName()).isEqualTo("list1");
assertThat(persistedList.get().getCurrency()).isEqualTo(JPY); assertThat(persistedList.get().getCurrency()).isEqualTo(USD);
assertThat(persistedList.get().getLabelsToPrices()) 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() new PremiumList.Builder()
.setName("premlist") .setName("premlist")
.setCurrency(USD) .setCurrency(USD)
.setLabelsToPrices(testPrices) .setLabelsToPrices(TEST_PRICES)
.setCreationTime(fakeClock.nowUtc()) .setCreationTime(fakeClock.nowUtc())
.build()); .build());
assertThat(PremiumListDao.getPremiumPrice("premlist", "silver")).hasValue(Money.of(USD, 10.23)); assertThat(PremiumListDao.getPremiumPrice("premlist", "silver")).hasValue(Money.of(USD, 10.23));