IdentifiedUser: Clear starred changes before rereading from API
Starred changes are normally cached in IdentifiedUser, but an
IdentifiedUser instance is reused in the extension API across calls.
We need to not reuse cached starred changes in this path in case they
have changed, for example due to a mutation also made through the
extension API.
Change-Id: Ic6e2cda17d4eca3ef30ef19f692b1a0a1b38d0c1
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java b/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java
index be5e000..fcfbeb2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java
@@ -14,6 +14,8 @@
package com.google.gerrit.server;
+import com.google.common.base.Function;
+import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import com.google.gerrit.common.Nullable;
@@ -37,6 +39,7 @@
import com.google.gerrit.server.config.DisableReverseDnsLookup;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gwtorm.server.OrmException;
+import com.google.gwtorm.server.OrmRuntimeException;
import com.google.gwtorm.server.ResultSet;
import com.google.inject.Inject;
import com.google.inject.OutOfScopeException;
@@ -327,35 +330,29 @@
@Override
public Set<Change.Id> getStarredChanges() {
if (starredChanges == null) {
- if (dbProvider == null) {
- throw new OutOfScopeException("Not in request scoped user");
- }
- Set<Change.Id> h = Sets.newHashSet();
+ checkRequestScope();
try {
- if (starredQuery != null) {
- for (StarredChange sc : starredQuery) {
- h.add(sc.getChangeId());
- }
- starredQuery = null;
- } else {
- for (StarredChange sc : dbProvider.get().starredChanges()
- .byAccount(getAccountId())) {
- h.add(sc.getChangeId());
- }
- }
- } catch (OrmException e) {
- log.warn("Cannot query starred by user changes", e);
+ starredChanges = starredChangeIds(
+ starredQuery != null ? starredQuery : starredQuery());
+ } catch (OrmException | OrmRuntimeException e) {
+ log.warn("Cannot query starred changes", e);
}
- starredChanges = Collections.unmodifiableSet(h);
}
return starredChanges;
}
+ public Set<Change.Id> clearStarredChanges() {
+ // Async query may have started before an update that the caller expects
+ // to see the results of, so we can't trust it.
+ abortStarredChanges();
+ starredChanges = null;
+ return starredChanges;
+ }
+
public void asyncStarredChanges() {
if (starredChanges == null && dbProvider != null) {
try {
- starredQuery =
- dbProvider.get().starredChanges().byAccount(getAccountId());
+ starredQuery = starredQuery();
} catch (OrmException e) {
log.warn("Cannot query starred by user changes", e);
starredQuery = null;
@@ -374,12 +371,31 @@
}
}
+ private void checkRequestScope() {
+ if (dbProvider == null) {
+ throw new OutOfScopeException("Not in request scoped user");
+ }
+ }
+
+ private ResultSet<StarredChange> starredQuery() throws OrmException {
+ return dbProvider.get().starredChanges().byAccount(getAccountId());
+ }
+
+ private static ImmutableSet<Change.Id> starredChangeIds(
+ Iterable<StarredChange> scs) {
+ return FluentIterable.from(scs)
+ .transform(new Function<StarredChange, Change.Id>() {
+ @Override
+ public Change.Id apply(StarredChange in) {
+ return in.getChangeId();
+ }
+ }).toSet();
+ }
+
@Override
public Collection<AccountProjectWatch> getNotificationFilters() {
if (notificationFilters == null) {
- if (dbProvider == null) {
- throw new OutOfScopeException("Not in request scoped user");
- }
+ checkRequestScope();
List<AccountProjectWatch> r;
try {
r = dbProvider.get().accountProjectWatches() //
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
index 610e344..06c59ee 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
@@ -31,6 +31,8 @@
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.Abandon;
import com.google.gerrit.server.change.ChangeEdits;
import com.google.gerrit.server.change.ChangeJson;
@@ -61,6 +63,7 @@
ChangeApiImpl create(ChangeResource change);
}
+ private final Provider<CurrentUser> user;
private final Changes changeApi;
private final Revisions revisions;
private final RevisionApiImpl.Factory revisionApi;
@@ -79,7 +82,8 @@
private final ChangeEdits.Detail editDetail;
@Inject
- ChangeApiImpl(Changes changeApi,
+ ChangeApiImpl(Provider<CurrentUser> user,
+ Changes changeApi,
Revisions revisions,
RevisionApiImpl.Factory revisionApi,
Provider<SuggestReviewers> suggestReviewers,
@@ -95,6 +99,7 @@
Check check,
ChangeEdits.Detail editDetail,
@Assisted ChangeResource change) {
+ this.user = user;
this.changeApi = changeApi;
this.revert = revert;
this.revisions = revisions;
@@ -244,6 +249,10 @@
public ChangeInfo get(EnumSet<ListChangesOption> s)
throws RestApiException {
try {
+ CurrentUser u = user.get();
+ if (u.isIdentifiedUser()) {
+ ((IdentifiedUser) u).clearStarredChanges();
+ }
return changeJson.get().addOptions(s).format(change);
} catch (OrmException e) {
throw new RestApiException("Cannot retrieve change", e);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangesImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangesImpl.java
index 91809ec..c86c422 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangesImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangesImpl.java
@@ -29,6 +29,8 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.extensions.restapi.Url;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.ChangesCollection;
import com.google.gerrit.server.change.CreateChange;
import com.google.gerrit.server.project.InvalidChangeOperationException;
@@ -43,16 +45,19 @@
@Singleton
class ChangesImpl implements Changes {
+ private final Provider<CurrentUser> user;
private final ChangesCollection changes;
private final ChangeApiImpl.Factory api;
private final CreateChange createChange;
private final Provider<QueryChanges> queryProvider;
@Inject
- ChangesImpl(ChangesCollection changes,
+ ChangesImpl(Provider<CurrentUser> user,
+ ChangesCollection changes,
ChangeApiImpl.Factory api,
CreateChange createChange,
Provider<QueryChanges> queryProvider) {
+ this.user = user;
this.changes = changes;
this.api = api;
this.createChange = createChange;
@@ -123,6 +128,10 @@
}
try {
+ CurrentUser u = user.get();
+ if (u.isIdentifiedUser()) {
+ ((IdentifiedUser) u).clearStarredChanges();
+ }
List<?> result = qc.apply(TopLevelResource.INSTANCE);
if (result.isEmpty()) {
return ImmutableList.of();
