opendev/gerrit
Code Issues Proposed changes

Fix caching of /detail for modified reviewer lists

Browse Source 
Rely only on ETag for change entity caching. The Last-Modified
value was derived from the lastUpdatedOn property, which is not
always modified when children are updated.

When adding or removing reviewers from a change perform a no-op
modification of the Change entity. This bumps its rowVersion as
a side-effect of the database mutation, causing the ETag to be
different on the next read.

Change-Id: If480f92b419041e7d62839ab12211058e78f828f
This commit is contained in:
Shawn Pearce
2013-07-23 14:23:55 -07:00
parent c541d31f1c
commit 5d182ddba9
4 changed files with 32 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java
View File

@@ -37,6 +37,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.PatchSetApproval.LabelId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountInfo;
@@ -81,7 +82,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, Input> {
private final Provider<GroupsCollection> groupsCollection;
private final GroupMembers.Factory groupMembersFactory;
private final AccountInfo.Loader.Factory accountLoaderFactory;
private final Provider<ReviewDb> db;
private final Provider<ReviewDb> dbProvider;
private final IdentifiedUser currentUser;
private final IdentifiedUser.GenericFactory identifiedUserFactory;
private final Config cfg;
@@ -109,7 +110,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, Input> {
this.groupsCollection = groupsCollection;
this.groupMembersFactory = groupMembersFactory;
this.accountLoaderFactory = accountLoaderFactory;
this.db = db;
this.dbProvider = db;
this.currentUser = currentUser;
this.identifiedUserFactory = identifiedUserFactory;
this.cfg = cfg;
@@ -215,9 +216,10 @@ public class PostReviewers implements RestModifyView<ChangeResource, Input> {
return;
}
ReviewDb db = dbProvider.get();
PatchSet.Id psid = rsrc.getChange().currentPatchSetId();
Set<Account.Id> existing = Sets.newHashSet();
for (PatchSetApproval psa : db.get().patchSetApprovals().byPatchSet(psid)) {
for (PatchSetApproval psa : db.patchSetApprovals().byPatchSet(psid)) {
existing.add(psa.getAccountId());
}
@@ -235,7 +237,19 @@ public class PostReviewers implements RestModifyView<ChangeResource, Input> {
new ReviewerInfo(id), control, ImmutableList.of(psa)));
toInsert.add(psa);
}
db.get().patchSetApprovals().insert(toInsert);
if (toInsert.isEmpty()) {
return;
}
db.changes().beginTransaction(rsrc.getChange().getId());
try {
ChangeUtil.bumpRowVersionNotLastUpdatedOn(rsrc.getChange().getId(), db);
db.patchSetApprovals().insert(toInsert);
db.commit();
} finally {
db.rollback();
}
accountLoaderFactory.create(true).fill(result.reviewers);
postAdd(rsrc.getChange(), result);
}
@@ -248,10 +262,10 @@ public class PostReviewers implements RestModifyView<ChangeResource, Input> {
// Execute hook for added reviewers
//
PatchSet patchSet = db.get().patchSets().get(change.currentPatchSetId());
PatchSet patchSet = dbProvider.get().patchSets().get(change.currentPatchSetId());
for (AccountInfo info : result.reviewers) {
Account account = accountCache.get(info._id).getAccount();
hooks.doReviewerAddedHook(change, account, patchSet, db.get());
hooks.doReviewerAddedHook(change, account, patchSet, dbProvider.get());
}
// Email the reviewers