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

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.CanonicalWebUrl;
 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 @@ public class IdentifiedUser extends CurrentUser {
   @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 @@ public class IdentifiedUser extends CurrentUser {
     }
   }
 
+  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() //

gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java

@@ -31,6 +31,8 @@ import com.google.gerrit.extensions.common.SuggestedReviewerInfo;
 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 @@ class ChangeApiImpl implements ChangeApi {
     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 @@ class ChangeApiImpl implements ChangeApi {
   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 @@ class ChangeApiImpl implements ChangeApi {
       Check check,
       ChangeEdits.Detail editDetail,
       @Assisted ChangeResource change) {
+    this.user = user;
     this.changeApi = changeApi;
     this.revert = revert;
     this.revisions = revisions;
@@ -244,6 +249,10 @@ class ChangeApiImpl implements ChangeApi {
   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);

gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangesImpl.java

@@ -29,6 +29,8 @@ import com.google.gerrit.extensions.restapi.IdString;
 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 @@ import java.util.List;
 
 @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 @@ class ChangesImpl implements Changes {
     }
 
     try {
+      CurrentUser u = user.get();
+      if (u.isIdentifiedUser()) {
+        ((IdentifiedUser) u).clearStarredChanges();
+      }
       List<?> result = qc.apply(TopLevelResource.INSTANCE);
       if (result.isEmpty()) {
         return ImmutableList.of();