Merge "Fix reading changes from index for older schema versions"

gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java

@@ -17,6 +17,7 @@ package com.google.gerrit.lucene;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.gerrit.server.git.QueueProvider.QueueType.INTERACTIVE;
+import static com.google.gerrit.server.index.ChangeField.CHANGE;
 import static com.google.gerrit.server.index.ChangeField.LEGACY_ID;
 import static com.google.gerrit.server.index.ChangeField.PROJECT;
 import static com.google.gerrit.server.index.IndexRewriter.CLOSED_STATUSES;
@@ -431,18 +432,30 @@ public class LuceneChangeIndex implements ChangeIndex {
   }
 
   private Set<String> fields(QueryOptions opts) {
-    if (schemaHasRequestedField(ChangeField.LEGACY_ID, opts.fields())
+    // Ensure we request enough fields to construct a ChangeData.
-        || schemaHasRequestedField(ChangeField.CHANGE, opts.fields())) {
+    Set<String> fs = opts.fields();
-      return opts.fields();
+    if (fs.contains(CHANGE.getName())) {
+      // A Change is always sufficient.
+      return fs;
     }
-    // Request the numeric ID field even if the caller did not request it,
-    // otherwise we can't actually construct a ChangeData.
-    return Sets.union(opts.fields(), ImmutableSet.of(LEGACY_ID.getName()));
-  }
 
-  private boolean schemaHasRequestedField(FieldDef<ChangeData, ?> field,
+    if (!schema.hasField(PROJECT)) {
-      Set<String> requested) {
+      // Schema is not new enough to have project field. Ensure we have ID
-    return schema.hasField(field) && requested.contains(field.getName());
+      // field, and call createOnlyWhenNotedbDisabled from toChangeData below.
+      if (fs.contains(LEGACY_ID.getName())) {
+        return fs;
+      } else {
+        return Sets.union(fs, ImmutableSet.of(LEGACY_ID.getName()));
+      }
+    }
+
+    // New enough schema to have project field, so ensure that is requested.
+    if (fs.contains(PROJECT.getName()) && fs.contains(LEGACY_ID.getName())) {
+      return fs;
+    } else {
+      return Sets.union(fs,
+          ImmutableSet.of(LEGACY_ID.getName(), PROJECT.getName()));
+    }
   }
 
   private ChangeData toChangeData(Document doc, Set<String> fields,
@@ -455,10 +468,17 @@ public class LuceneChangeIndex implements ChangeIndex {
       cd = changeDataFactory.create(db.get(),
           ChangeProtoField.CODEC.decode(cb.bytes, cb.offset, cb.length));
     } else {
-      int id = doc.getField(idFieldName).numericValue().intValue();
+      Change.Id id =
-      Project.NameKey project =
+          new Change.Id(doc.getField(idFieldName).numericValue().intValue());
-          new Project.NameKey(doc.getField(PROJECT.getName()).stringValue());
+      IndexableField project = doc.getField(PROJECT.getName());
-      cd = changeDataFactory.create(db.get(), project, new Change.Id(id));
+      if (project == null) {
+        // Old schema without project field: we can safely assume notedb is
+        // disabled.
+        cd = changeDataFactory.createOnlyWhenNotedbDisabled(db.get(), id);
+      } else {
+        cd = changeDataFactory.create(
+            db.get(), new Project.NameKey(project.stringValue()), id);
+      }
     }
 
     if (fields.contains(PATCH_SET_FIELD)) {

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java

@@ -381,7 +381,7 @@ public class ChangeJson {
       // If any problems were fixed, the ChangeData needs to be reloaded.
       for (ProblemInfo p : out.problems) {
         if (p.status == ProblemInfo.Status.FIXED) {
-          cd = changeDataFactory.create(cd.db(), cd.getProject(), cd.getId());
+          cd = changeDataFactory.create(cd.db(), cd.project(), cd.getId());
           break;
         }
       }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.notedb;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_HOST;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -134,6 +135,16 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
       return new ChangeNotes(repoManager, migration, allUsersProvider,
           change.getProject(), change).load();
     }
+
+    // TODO(dborowitz): Remove when deleting index schemas <27.
+    public ChangeNotes createFromIdOnlyWhenNotedbDisabled(
+        ReviewDb db, Change.Id changeId) throws OrmException {
+    checkState(!migration.readChanges(), "do not call"
+        + " createFromIdOnlyWhenNotedbDisabled when notedb is enabled");
+      Change change = db.changes().get(changeId);
+      return new ChangeNotes(repoManager, migration, allUsersProvider,
+          change.getProject(), change).load();
+    }
   }
 
   private final Project.NameKey project;

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.query.change;
 
+import static com.google.common.base.Preconditions.checkState;
 import static com.google.gerrit.server.ApprovalsUtil.sortApprovals;
 
 import com.google.auto.value.AutoValue;
@@ -275,6 +276,9 @@ public class ChangeData {
     ChangeData create(ReviewDb db, Project.NameKey project, Change.Id id);
     ChangeData create(ReviewDb db, Change c);
     ChangeData create(ReviewDb db, ChangeControl c);
+
+    // TODO(dborowitz): Remove when deleting index schemas <27.
+    ChangeData createOnlyWhenNotedbDisabled(ReviewDb db, Change.Id id);
   }
 
   /**
@@ -309,8 +313,8 @@ public class ChangeData {
   private final NotesMigration notesMigration;
   private final MergeabilityCache mergeabilityCache;
   private final Change.Id legacyId;
-  private final Project.NameKey project;
   private ChangeDataSource returnedBySource;
+  private Project.NameKey project;
   private Change change;
   private ChangeNotes notes;
   private String commitMessage;
@@ -444,6 +448,43 @@ public class ChangeData {
     project = notes.getProjectName();
   }
 
+  @AssistedInject
+  private ChangeData(
+      GitRepositoryManager repoManager,
+      ChangeControl.GenericFactory changeControlFactory,
+      IdentifiedUser.GenericFactory userFactory,
+      ProjectCache projectCache,
+      MergeUtil.Factory mergeUtilFactory,
+      ChangeNotes.Factory notesFactory,
+      ApprovalsUtil approvalsUtil,
+      ChangeMessagesUtil cmUtil,
+      PatchLineCommentsUtil plcUtil,
+      PatchSetUtil psUtil,
+      PatchListCache patchListCache,
+      NotesMigration notesMigration,
+      MergeabilityCache mergeabilityCache,
+      @Assisted ReviewDb db,
+      @Assisted Change.Id id) {
+    checkState(!notesMigration.readChanges(),
+        "do not call createOnlyWhenNotedbDisabled when notedb is enabled");
+    this.db = db;
+    this.repoManager = repoManager;
+    this.changeControlFactory = changeControlFactory;
+    this.userFactory = userFactory;
+    this.projectCache = projectCache;
+    this.mergeUtilFactory = mergeUtilFactory;
+    this.notesFactory = notesFactory;
+    this.approvalsUtil = approvalsUtil;
+    this.cmUtil = cmUtil;
+    this.plcUtil = plcUtil;
+    this.psUtil = psUtil;
+    this.patchListCache = patchListCache;
+    this.notesMigration = notesMigration;
+    this.mergeabilityCache = mergeabilityCache;
+    this.legacyId = id;
+    this.project = null;
+  }
+
   public ReviewDb db() {
     return db;
   }
@@ -545,7 +586,12 @@ public class ChangeData {
     return legacyId;
   }
 
-  public Project.NameKey getProject() {
+  public Project.NameKey project() throws OrmException {
+    if (project == null) {
+      checkState(!notesMigration.readChanges(), "should not have created "
+          + " ChangeData without a project when notedb is enabled");
+      project = change().getProject();
+    }
     return project;
   }
 
@@ -580,7 +626,7 @@ public class ChangeData {
         changeControl = changeControlFactory.controlFor(db, change, user);
       } else {
         changeControl =
-            changeControlFactory.controlFor(db, project, legacyId, user);
+            changeControlFactory.controlFor(db, project(), legacyId, user);
       }
     } catch (NoSuchChangeException e) {
       throw new OrmException(e);
@@ -605,7 +651,11 @@ public class ChangeData {
   }
 
   public Change reloadChange() throws OrmException {
-    notes = notesFactory.create(db, project, legacyId);
+    if (project == null) {
+      notes = notesFactory.createFromIdOnlyWhenNotedbDisabled(db, legacyId);
+    } else {
+      notes = notesFactory.create(db, project, legacyId);
+    }
     change = notes.getChange();
     if (change == null) {
       throw new OrmException("Unable to load change " + legacyId);
@@ -615,7 +665,7 @@ public class ChangeData {
 
   public ChangeNotes notes() throws OrmException {
     if (notes == null) {
-      notes = notesFactory.create(db, project, legacyId);
+      notes = notesFactory.create(db, project(), legacyId);
     }
     return notes;
   }
@@ -698,7 +748,7 @@ public class ChangeData {
       return false;
     }
     String sha1 = ps.getRevision().get();
-    try (Repository repo = repoManager.openRepository(project);
+    try (Repository repo = repoManager.openRepository(project());
         RevWalk walk = new RevWalk(repo)) {
       RevCommit c = walk.parseCommit(ObjectId.fromString(sha1));
       commitMessage = c.getFullMessage();
@@ -821,7 +871,7 @@ public class ChangeData {
         if (ps == null || !changeControl().isPatchVisible(ps, db)) {
           return null;
         }
-        try (Repository repo = repoManager.openRepository(project)) {
+        try (Repository repo = repoManager.openRepository(project())) {
           Ref ref = repo.getRefDatabase().exactRef(c.getDest().get());
           SubmitTypeRecord str = submitTypeRecord();
           if (!str.isOk()) {
@@ -830,7 +880,7 @@ public class ChangeData {
             return false;
           }
           String mergeStrategy = mergeUtilFactory
-              .create(projectCache.get(project))
+              .create(projectCache.get(project()))
               .mergeStrategyName();
           mergeable = mergeabilityCache.get(
               ObjectId.fromString(ps.getRevision().get()),
@@ -851,7 +901,7 @@ public class ChangeData {
       }
       editsByUser = new HashSet<>();
       Change.Id id = change.getId();
-      try (Repository repo = repoManager.openRepository(project)) {
+      try (Repository repo = repoManager.openRepository(project())) {
         for (String ref
             : repo.getRefDatabase().getRefs(RefNames.REFS_USERS).keySet()) {
           if (Change.Id.fromEditRefPart(ref).equals(id)) {