Use changenumber field when querying for change number
Keep using the index legacy document-id (legacy_id_str) for
URLs queries like: "/q/123456", "/q/Iasdw2312321",
"/q/project~123456" which are expecting to always find a
single element.
Use the `changenumber` field in ChangeFinder and the
explicit search using the `change:` predicate.
The real change number allows discoverability of imported changes.
This is the list of the affected queries:
* get drafts by user
* get starred changes by user
* query changes by changeNum
* query changes by project~changeNum
* get conflicting changes
Furhermore, `LuceneChangeIndex` will now try to use the
`changenumber` field to create the `Change.Id` for ChangeData
results fetched from the index.
Bug: Issue 318396515
Release-Notes: Use changenumber field when querying for change number
Change-Id: I64f55e8af61e6708e9ae120a411185ba7adedf75
diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
index 513abad..ffd25ba 100644
--- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
@@ -17,6 +17,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.lucene.AbstractLuceneIndex.sortFieldName;
import static com.google.gerrit.server.git.QueueProvider.QueueType.INTERACTIVE;
+import static com.google.gerrit.server.index.change.ChangeField.CHANGENUM_SPEC;
import static com.google.gerrit.server.index.change.ChangeField.NUMERIC_ID_STR_SPEC;
import static com.google.gerrit.server.index.change.ChangeField.PROJECT_SPEC;
import static com.google.gerrit.server.index.change.ChangeIndexRewriter.CLOSED_STATUSES;
@@ -528,7 +529,11 @@
ImmutableList.Builder<ChangeData> result =
ImmutableList.builderWithExpectedSize(docs.size());
for (Document doc : docs) {
- result.add(toChangeData(fields(doc, fields), fields, NUMERIC_ID_STR_SPEC.getName()));
+ String fieldName =
+ doc.getField(CHANGENUM_SPEC.getName()) != null
+ ? CHANGENUM_SPEC.getName()
+ : NUMERIC_ID_STR_SPEC.getName();
+ result.add(toChangeData(fields(doc, fields), fields, fieldName));
}
return result.build();
} catch (InterruptedException e) {
diff --git a/java/com/google/gerrit/server/change/ChangeFinder.java b/java/com/google/gerrit/server/change/ChangeFinder.java
index 9f253de..5668c27 100644
--- a/java/com/google/gerrit/server/change/ChangeFinder.java
+++ b/java/com/google/gerrit/server/change/ChangeFinder.java
@@ -208,6 +208,12 @@
return Optional.of(notes.get(0));
}
+ /**
+ * @deprecated this method is not reliable in Gerrit instances with imported changes, since
+ * multiple changes can have the same change number and make the `changeIdProjectCache` cache
+ * pointless.
+ */
+ @Deprecated(since = "3.10", forRemoval = true)
public List<ChangeNotes> find(Change.Id id) {
String project = changeIdProjectCache.getIfPresent(id);
if (project != null) {
@@ -245,7 +251,7 @@
// this case.)
Set<Change.Id> seen = Sets.newHashSetWithExpectedSize(cds.size());
for (ChangeData cd : cds) {
- if (seen.add(cd.getId())) {
+ if (seen.add(cd.virtualId())) {
try {
notes.add(cd.notes());
} catch (NoSuchChangeException e) {
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index 0a1025e..0abe9cf 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -550,7 +550,8 @@
}
continue;
}
- ChangeInfo info = cache.get(cd.getId());
+ Change.Id cdUniqueId = cd.virtualId();
+ ChangeInfo info = cache.get(cdUniqueId);
if (info != null && isCacheable) {
changeInfos.add(info);
continue;
@@ -562,7 +563,7 @@
info = format(cd, Optional.empty(), false, pluginInfosByChange.get(cd.getId()));
changeInfos.add(info);
if (isCacheable) {
- cache.put(Change.id(info._number), info);
+ cache.put(cdUniqueId, info);
}
} catch (RuntimeException e) {
Optional<RequestCancelledException> requestCancelledException =
diff --git a/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java b/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java
index 83024e3..d8afbbb 100644
--- a/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java
+++ b/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java
@@ -155,13 +155,14 @@
.byProject(key);
Map<Change.Id, CachedChange> result = new HashMap<>(cds.size());
for (ChangeData cd : cds) {
- if (result.containsKey(cd.getId())) {
+ final Change.Id cdUniqueId = cd.virtualId();
+ if (result.containsKey(cdUniqueId)) {
logger.atWarning().log(
"Duplicate changes returned from change query by project %s: %s, %s",
- key, cd.change(), result.get(cd.getId()).change());
+ key, cd.change(), result.get(cdUniqueId).change());
}
result.put(
- cd.getId(),
+ cdUniqueId,
new AutoValue_SearchingChangeCacheImpl_CachedChange(cd.change(), cd.reviewers()));
}
return List.copyOf(result.values());
diff --git a/java/com/google/gerrit/server/index/IndexUtils.java b/java/com/google/gerrit/server/index/IndexUtils.java
index f2a2904..f81a9ce 100644
--- a/java/com/google/gerrit/server/index/IndexUtils.java
+++ b/java/com/google/gerrit/server/index/IndexUtils.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.index;
+import static com.google.gerrit.server.index.change.ChangeField.CHANGENUM_SPEC;
import static com.google.gerrit.server.index.change.ChangeField.CHANGE_SPEC;
import static com.google.gerrit.server.index.change.ChangeField.NUMERIC_ID_STR_SPEC;
import static com.google.gerrit.server.index.change.ChangeField.PROJECT_SPEC;
@@ -81,10 +82,18 @@
// A Change is always sufficient.
return fs;
}
- if (fs.contains(PROJECT_SPEC.getName()) && fs.contains(NUMERIC_ID_STR_SPEC.getName())) {
+
+ Set<String> requiredFields =
+ CHANGENUM_SPEC.getName() != null
+ ? ImmutableSet.of(
+ NUMERIC_ID_STR_SPEC.getName(), PROJECT_SPEC.getName(), CHANGENUM_SPEC.getName())
+ : ImmutableSet.of(NUMERIC_ID_STR_SPEC.getName(), PROJECT_SPEC.getName());
+
+ if (fs.containsAll(requiredFields)) {
return fs;
}
- return Sets.union(fs, ImmutableSet.of(NUMERIC_ID_STR_SPEC.getName(), PROJECT_SPEC.getName()));
+
+ return Sets.union(fs, ImmutableSet.copyOf(requiredFields));
}
/**
diff --git a/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
index df30573..6c38210 100644
--- a/java/com/google/gerrit/server/mail/receive/MailProcessor.java
+++ b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -252,7 +252,7 @@
queryProvider
.get()
.enforceVisibility(true)
- .byLegacyChangeId(Change.id(metadata.changeNumber));
+ .byChangeNumber(Change.id(metadata.changeNumber));
if (changeDataList.isEmpty()) {
sendRejectionEmail(message, InboundEmailError.CHANGE_NOT_FOUND);
return;
diff --git a/java/com/google/gerrit/server/query/change/ChangePredicates.java b/java/com/google/gerrit/server/query/change/ChangePredicates.java
index 587f2de..d5f9c5b 100644
--- a/java/com/google/gerrit/server/query/change/ChangePredicates.java
+++ b/java/com/google/gerrit/server/query/change/ChangePredicates.java
@@ -18,6 +18,7 @@
import com.google.common.base.CharMatcher;
import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.common.UsedAt;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
@@ -87,7 +88,10 @@
/**
* Returns a predicate that matches changes where the provided {@link
* com.google.gerrit.entities.Account.Id} has a pending draft comment.
+ *
+ * <p>The predicates filter by "legacy_id_str" field.
*/
+ @UsedAt(UsedAt.Project.GOOGLE)
public static Predicate<ChangeData> draftBy(
DraftCommentsReader draftCommentsReader, Account.Id id) {
ImmutableSet<Predicate<ChangeData>> changeIdPredicates =
@@ -102,7 +106,10 @@
/**
* Returns a predicate that matches changes where the provided {@link
* com.google.gerrit.entities.Account.Id} has starred changes with {@code label}.
+ *
+ * <p>The predicates filter by "legacy_id_str" field.
*/
+ @UsedAt(UsedAt.Project.GOOGLE)
public static Predicate<ChangeData> starBy(
StarredChangesReader starredChangesReader, Account.Id id) {
ImmutableSet<Predicate<ChangeData>> starredChanges =
@@ -144,6 +151,19 @@
}
/**
+ * Returns a predicate that matches the change number with the provided {@link
+ * com.google.gerrit.entities.Change.Id}.
+ */
+ public static Predicate<ChangeData> changeNumber(
+ Change.Id id, ChangeQueryBuilder.Arguments args) {
+ if (args.getSchema().hasField(ChangeField.CHANGENUM_SPEC)) {
+ return new ChangeIndexCardinalPredicate(
+ ChangeField.CHANGENUM_SPEC, ChangeQueryBuilder.FIELD_CHANGE, id.toString(), 1);
+ }
+ return idStr(id);
+ }
+
+ /**
* Returns a predicate that matches changes owned by the provided {@link
* com.google.gerrit.entities.Account.Id}.
*/
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 030db1b..d598739 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -144,7 +144,7 @@
public interface ChangeIsOperandFactory extends ChangeOperandFactory {}
- private static final Pattern PAT_LEGACY_ID = Pattern.compile("^[1-9][0-9]*$");
+ private static final Pattern PAT_CHANGE_NUMBER = Pattern.compile("^[1-9][0-9]*$");
private static final Pattern PAT_PROJECT_CHANGE_NUM = Pattern.compile("^([^~]+)~([1-9][0-9]*)$");
private static final Pattern PAT_CHANGE_ID = Pattern.compile(CHANGE_ID_PATTERN);
private static final Pattern DEF_CHANGE =
@@ -589,6 +589,18 @@
@Operator
public Predicate<ChangeData> change(String query) throws QueryParseException {
+ return getPredicateChangeData(query, changeId -> ChangePredicates.changeNumber(changeId, args));
+ }
+
+ // Keep using the index legacy document-id (legacy_id_str) for URLs queries like: "/q/123456",
+ // "/q/Iasdw2312321", "/q/project~123456" that are expecting to always find a single element.
+ private Predicate<ChangeData> defaultSearch(String query) throws QueryParseException {
+ return getPredicateChangeData(query, ChangePredicates::idStr);
+ }
+
+ private Predicate<ChangeData> getPredicateChangeData(
+ String query, Function<Change.Id, Predicate<ChangeData>> changePredicateGetter)
+ throws QueryParseException {
Optional<ChangeTriplet> triplet = ChangeTriplet.parse(query);
if (triplet.isPresent()) {
return Predicate.and(
@@ -602,11 +614,10 @@
return Predicate.and(
project(projectChangeNumber.group(1)),
ChangePredicates.idStr(projectChangeNumber.group(2)));
-
- } else if (PAT_LEGACY_ID.matcher(query).matches()) {
+ } else if (PAT_CHANGE_NUMBER.matcher(query).matches()) {
Integer id = Ints.tryParse(query);
if (id != null) {
- return ChangePredicates.idStr(Change.id(id));
+ return changePredicateGetter.apply(Change.id(id));
}
} else if (PAT_CHANGE_ID.matcher(query).matches()) {
return ChangePredicates.idPrefix(parseChangeId(query));
@@ -1691,13 +1702,13 @@
} else if (DEF_CHANGE.matcher(query).matches()) {
List<Predicate<ChangeData>> predicates = Lists.newArrayListWithCapacity(2);
try {
- predicates.add(change(query));
+ predicates.add(defaultSearch(query));
} catch (QueryParseException e) {
// Skip.
}
- // For PAT_LEGACY_ID, it may also be the prefix of some commits.
- if (query.length() >= 6 && PAT_LEGACY_ID.matcher(query).matches()) {
+ // For PAT_CHANGE_NUMBER, it may also be the prefix of some commits.
+ if (query.length() >= 6 && PAT_CHANGE_NUMBER.matcher(query).matches()) {
predicates.add(commit(query));
}
@@ -1861,12 +1872,12 @@
}
private List<ChangeData> parseChangeData(String value) throws QueryParseException {
- if (PAT_LEGACY_ID.matcher(value).matches()) {
+ if (PAT_CHANGE_NUMBER.matcher(value).matches()) {
Optional<Change.Id> id = Change.Id.tryParse(value);
if (!id.isPresent()) {
throw error("Invalid change id " + value);
}
- return args.queryProvider.get().byLegacyChangeId(id.get());
+ return args.queryProvider.get().byChangeNumber(id.get());
} else if (PAT_CHANGE_ID.matcher(value).matches()) {
List<ChangeData> changes = args.queryProvider.get().byKeyPrefix(parseChangeId(value));
if (changes.isEmpty()) {
diff --git a/java/com/google/gerrit/server/query/change/ConflictsPredicate.java b/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
index fc4c1d0..87b8915 100644
--- a/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
@@ -89,7 +89,7 @@
List<Predicate<ChangeData>> and = new ArrayList<>(5);
and.add(ChangePredicates.project(c.getProject()));
and.add(ChangePredicates.ref(c.getDest().branch()));
- and.add(Predicate.not(ChangePredicates.idStr(c.getId())));
+ and.add(Predicate.not(ChangePredicates.changeNumber(c.getId(), args)));
and.add(Predicate.or(filePredicates));
ChangeDataCache changeDataCache = new ChangeDataCache(cd, args.projectCache);
diff --git a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
index f3eda15..a8ee4bc 100644
--- a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
+++ b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
@@ -88,6 +88,7 @@
private final ChangeData.Factory changeDataFactory;
private final ChangeNotes.Factory notesFactory;
private final EditByPredicateProvider editByPredicateProvider;
+ private final Provider<ChangeQueryBuilder.Arguments> queryBuilderArgsProvider;
@Inject
InternalChangeQuery(
@@ -96,11 +97,13 @@
IndexConfig indexConfig,
ChangeData.Factory changeDataFactory,
ChangeNotes.Factory notesFactory,
- EditByPredicateProvider editByPredicateProvider) {
+ EditByPredicateProvider editByPredicateProvider,
+ Provider<ChangeQueryBuilder.Arguments> queryBuilderArgsProvider) {
super(queryProcessor, indexes, indexConfig);
this.changeDataFactory = changeDataFactory;
this.notesFactory = notesFactory;
this.editByPredicateProvider = editByPredicateProvider;
+ this.queryBuilderArgsProvider = queryBuilderArgsProvider;
}
public List<ChangeData> byKey(Change.Key key) {
@@ -115,6 +118,10 @@
return query(ChangePredicates.idStr(id));
}
+ public List<ChangeData> byChangeNumber(Change.Id id) {
+ return query(ChangePredicates.changeNumber(id, queryBuilderArgsProvider.get()));
+ }
+
@UsedAt(UsedAt.Project.GOOGLE)
public List<ChangeData> byLegacyChangeIds(Collection<Change.Id> ids) {
List<Predicate<ChangeData>> preds = new ArrayList<>(ids.size());
@@ -319,7 +326,7 @@
for (List<String> part : Iterables.partition(groups, batchSize)) {
for (ChangeData cd :
queryExhaustively(querySupplier, byProjectGroupsPredicate(indexConfig, project, part))) {
- if (!seen.add(cd.getId())) {
+ if (!seen.add(cd.virtualId())) {
result.add(cd);
}
}
diff --git a/java/com/google/gerrit/server/query/change/OrSource.java b/java/com/google/gerrit/server/query/change/OrSource.java
index 9633a18..8894287 100644
--- a/java/com/google/gerrit/server/query/change/OrSource.java
+++ b/java/com/google/gerrit/server/query/change/OrSource.java
@@ -52,7 +52,7 @@
Set<Change.Id> have = new HashSet<>();
for (ResultSet<ChangeData> resultSet : results) {
for (ChangeData result : resultSet) {
- if (have.add(result.getId())) {
+ if (have.add(result.virtualId())) {
r.add(result);
}
}
