zydronium/google-nomulus
1
0
Fork 0
mirror of https://github.com/google/nomulus.git synced 2025-06-29 15:53:35 +02:00
Code Issues Projects Releases Packages Wiki Activity

Fix entry-overwriting bug in PremiumList.saveAndUpdateEntries()

Browse source 
The saveAndUpdateEntries() method was always saving entries under the current entity's revisionKey, and then always deleting them under the old entity's revisionKey - even if the revisionKeys were the same (i.e., we didn't change any entry content in the list, in which case the builder's build() method doesn't create a new revisionKey).

This would have caused the existing entries to be unnecessarily resaved, and (worse) then caused those same existing entries to all be deleted.

This fixes the bug by only resaving entries when the revisionKey has changed.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=124873871
This commit is contained in:
nickfelt 2016-06-14 13:18:47 -07:00 • committed by Ben McIlwain
parent 45747fd792
commit 6466ad51f6
2 changed files with 51 additions and 10 deletions
Download patch file Download diff file Expand all files Collapse all files

27
java/google/registry/model/registry/label/PremiumList.java
View file

@ -266,14 +266,21 @@ public final class PremiumList extends BaseDomainLabelList<Money, PremiumList.Pr
*/ */
public PremiumList saveAndUpdateEntries() { public PremiumList saveAndUpdateEntries() {
final Optional<PremiumList> oldPremiumList = get(name); final Optional<PremiumList> oldPremiumList = get(name);
// Save the new child entities in a series of transactions. // Only update entries if there's actually a new revision of the list to save (which there will
for (final List<PremiumListEntry> batch // be if the list content changes, vs just the description/metadata).
: partition(premiumListMap.values(), TRANSACTION_BATCH_SIZE)) { boolean entriesToUpdate =
ofy().transactNew(new VoidWork() { !oldPremiumList.isPresent()
@Override || !Objects.equals(oldPremiumList.get().revisionKey, this.revisionKey);
public void vrun() { // If needed, save the new child entities in a series of transactions.
ofy().save().entities(batch); if (entriesToUpdate) {
}}); for (final List<PremiumListEntry> batch
: partition(premiumListMap.values(), TRANSACTION_BATCH_SIZE)) {
ofy().transactNew(new VoidWork() {
@Override
public void vrun() {
ofy().save().entities(batch);
}});
}
} }
// Save the new PremiumList itself. // Save the new PremiumList itself.
PremiumList updated = ofy().transactNew(new Work<PremiumList>() { PremiumList updated = ofy().transactNew(new Work<PremiumList>() {
@ -296,8 +303,8 @@ public final class PremiumList extends BaseDomainLabelList<Money, PremiumList.Pr
}}); }});
// Update the cache. // Update the cache.
PremiumList.cache.put(name, updated); PremiumList.cache.put(name, updated);
// Delete the entities under the old PremiumList, if any. // If needed and there are any, delete the entities under the old PremiumList.
if (oldPremiumList.isPresent()) { if (entriesToUpdate && oldPremiumList.isPresent()) {
oldPremiumList.get().deleteEntries(); oldPremiumList.get().deleteEntries();
} }
return updated; return updated;

34
javatests/google/registry/model/registry/label/PremiumListTest.java
View file

@ -179,6 +179,40 @@ public class PremiumListTest {
assertThat(entry.label).isEqualTo("lol"); assertThat(entry.label).isEqualTo("lol");
} }
@Test
public void test_saveAndUpdateEntries_twiceOnUnchangedList() throws Exception {
PremiumList pl =
new PremiumList.Builder()
.setName("pl")
.setPremiumListMapFromLines(ImmutableList.of("test,USD 1"))
.build()
.saveAndUpdateEntries();
Map<String, PremiumListEntry> entries = pl.getPremiumListEntries();
assertThat(entries.keySet()).containsExactly("test");
assertThat(PremiumList.get("pl").get().getPremiumListEntries()).isEqualTo(entries);
// Save again with no changes, and clear the cache to force a re-load from datastore.
pl.saveAndUpdateEntries();
ofy().clearSessionCache();
assertThat(PremiumList.get("pl").get().getPremiumListEntries()).isEqualTo(entries);
}
@Test
public void test_saveAndUpdateEntries_twiceOnListWithOnlyMetadataChanges() throws Exception {
PremiumList pl =
new PremiumList.Builder()
.setName("pl")
.setPremiumListMapFromLines(ImmutableList.of("test,USD 1"))
.build()
.saveAndUpdateEntries();
Map<String, PremiumListEntry> entries = pl.getPremiumListEntries();
assertThat(entries.keySet()).containsExactly("test");
assertThat(PremiumList.get("pl").get().getPremiumListEntries()).isEqualTo(entries);
// Save again with description changed, and clear the cache to force a re-load from datastore.
pl.asBuilder().setDescription("foobar").build().saveAndUpdateEntries();
ofy().clearSessionCache();
assertThat(PremiumList.get("pl").get().getPremiumListEntries()).isEqualTo(entries);
}
@Test @Test
public void testDelete() throws Exception { public void testDelete() throws Exception {
persistPremiumList("gtld1", "trombone,USD 10"); persistPremiumList("gtld1", "trombone,USD 10");