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); } }