Cancel deprecation of change identifiers
Since 2.16 the documentation of change identifiers states that the
identifiers other than "<project>~<numericid>" are deprecated and will
be removed in a future release. Since then the identifiers have still
not been removed and there is no clear plan to do so.
It is likely that "deprecated" identifiers are still used in links in
places where they can't be updated, for example in emails and forum
posts. Due to this, and since continuing to support all of the types
does not add any technical burden, ESC decided that the deprecation
should be cancelled.
Remove the deprecation notice from the documentation and remove the
change.api.allowedIdentifier setting which is now redundant.
Bug: Issue 11772
Change-Id: Iad8ef1d47c8b1c836bb59e3ad3ed4a3f52281294
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 95ba5b6..9a899f9 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -1180,18 +1180,6 @@
+
Default is true.
-[[change.api.allowedIdentifier]]change.api.allowedIdentifier::
-+
-Change identifier(s) that are allowed on the API. See
-link:rest-api-changes.html#change-id[Change Id] for more information.
-+
-Possible values are `ALL`, `TRIPLET`, `NUMERIC_ID`, `I_HASH`, and
-`COMMIT_HASH` or any combination of those as a string list.
-`PROJECT_NUMERIC_ID` is always allowed and doesn't need to be listed
-explicitly.
-+
-Default is `ALL`.
-
[[change.allowDrafts]]change.allowDrafts::
+
Legacy support for drafts workflow. If set to true, pushing a new change
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index a691db1..3bbb642 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -5498,8 +5498,7 @@
Identifier that uniquely identifies one change. It contains the URL-encoded
project name as well as the change number: "'$$<project>~<numericId>$$'"
-Depending on the server's configuration, Gerrit can still support the following
-deprecated identifiers. These will be removed in a future release:
+Gerrit also supports the following identifiers:
* an ID of the change in the format "'$$<project>~<branch>~<Change-Id>$$'",
where for the branch the `refs/heads/` prefix can be omitted
@@ -5508,10 +5507,6 @@
("I8473b95934b5732ac55d26311a706c9c2bde9940")
* a numeric change ID ("4247")
-If you need more time to migrate off of old change IDs, please see
-link:config-gerrit.html#change.api.allowedIdentifier[change.api.allowedIdentifier]
-for more information on how to enable the use of deprecated identifiers.
-
[[change-message-id]]
=== \{change-message-id\}
ID of a change message returned in a link:#change-message-info[ChangeMessageInfo].
diff --git a/java/com/google/gerrit/extensions/restapi/DeprecatedIdentifierException.java b/java/com/google/gerrit/extensions/restapi/DeprecatedIdentifierException.java
deleted file mode 100644
index aa28cfc..0000000
--- a/java/com/google/gerrit/extensions/restapi/DeprecatedIdentifierException.java
+++ /dev/null
@@ -1,25 +0,0 @@
-// Copyright (C) 2017 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.extensions.restapi;
-
-/** Named resource was accessed using a deprecated identifier. */
-public class DeprecatedIdentifierException extends BadRequestException {
- private static final long serialVersionUID = 1L;
-
- /** Requested resource using a deprecated identifier. */
- public DeprecatedIdentifierException(String msg) {
- super(msg);
- }
-}
diff --git a/java/com/google/gerrit/server/change/ChangeFinder.java b/java/com/google/gerrit/server/change/ChangeFinder.java
index 41d89ed..4e30b03 100644
--- a/java/com/google/gerrit/server/change/ChangeFinder.java
+++ b/java/com/google/gerrit/server/change/ChangeFinder.java
@@ -17,10 +17,8 @@
import com.google.common.base.Throwables;
import com.google.common.cache.Cache;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import com.google.common.primitives.Ints;
-import com.google.gerrit.extensions.restapi.DeprecatedIdentifierException;
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.metrics.Counter1;
@@ -32,8 +30,6 @@
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.cache.CacheModule;
-import com.google.gerrit.server.config.ConfigUtil;
-import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData;
@@ -50,7 +46,6 @@
import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
-import org.eclipse.jgit.lib.Config;
@Singleton
public class ChangeFinder {
@@ -80,7 +75,6 @@
private final Provider<ReviewDb> reviewDb;
private final ChangeNotes.Factory changeNotesFactory;
private final Counter1<ChangeIdType> changeIdCounter;
- private final ImmutableSet<ChangeIdType> allowedIdTypes;
@Inject
ChangeFinder(
@@ -89,8 +83,7 @@
Provider<InternalChangeQuery> queryProvider,
Provider<ReviewDb> reviewDb,
ChangeNotes.Factory changeNotesFactory,
- MetricMaker metricMaker,
- @GerritServerConfig Config config) {
+ MetricMaker metricMaker) {
this.indexConfig = indexConfig;
this.changeIdProjectCache = changeIdProjectCache;
this.queryProvider = queryProvider;
@@ -103,11 +96,6 @@
.setRate()
.setUnit("requests"),
Field.ofEnum(ChangeIdType.class, "change_id_type"));
- List<ChangeIdType> configuredChangeIdTypes =
- ConfigUtil.getEnumList(config, "change", "api", "allowedIdentifier", ChangeIdType.ALL);
- // Ensure that PROJECT_NUMERIC_ID can't be removed
- configuredChangeIdTypes.add(ChangeIdType.PROJECT_NUMERIC_ID);
- this.allowedIdTypes = ImmutableSet.copyOf(configuredChangeIdTypes);
}
public ChangeNotes findOne(String id) throws OrmException {
@@ -123,29 +111,9 @@
*
* @param id change identifier.
* @return possibly-empty list of notes for all matching changes; may or may not be visible.
- * @throws OrmException if an error occurred querying the database.
+ * @throws OrmException if an error occurred querying the database
*/
public List<ChangeNotes> find(String id) throws OrmException {
- try {
- return find(id, false);
- } catch (DeprecatedIdentifierException e) {
- // This can't happen because we don't enforce deprecation
- throw new OrmException(e);
- }
- }
-
- /**
- * Find changes matching the given identifier.
- *
- * @param id change identifier.
- * @param enforceDeprecation boolean to see if we should throw {@link
- * DeprecatedIdentifierException} in case the identifier is deprecated
- * @return possibly-empty list of notes for all matching changes; may or may not be visible.
- * @throws OrmException if an error occurred querying the database
- * @throws DeprecatedIdentifierException if the identifier is deprecated.
- */
- public List<ChangeNotes> find(String id, boolean enforceDeprecation)
- throws OrmException, DeprecatedIdentifierException {
if (id.isEmpty()) {
return Collections.emptyList();
}
@@ -156,7 +124,7 @@
// Try project~numericChangeId
Integer n = Ints.tryParse(id.substring(z + 1));
if (n != null) {
- checkIdType(ChangeIdType.PROJECT_NUMERIC_ID, enforceDeprecation, n.toString());
+ changeIdCounter.increment(ChangeIdType.PROJECT_NUMERIC_ID);
return fromProjectNumber(id.substring(0, z), n.intValue());
}
}
@@ -165,7 +133,7 @@
// Try numeric changeId
Integer n = Ints.tryParse(id);
if (n != null) {
- checkIdType(ChangeIdType.NUMERIC_ID, enforceDeprecation, n.toString());
+ changeIdCounter.increment(ChangeIdType.NUMERIC_ID);
return find(new Change.Id(n));
}
}
@@ -176,7 +144,7 @@
// Try commit hash
if (id.matches("^([0-9a-fA-F]{" + RevId.ABBREV_LEN + "," + RevId.LEN + "})$")) {
- checkIdType(ChangeIdType.COMMIT_HASH, enforceDeprecation, id);
+ changeIdCounter.increment(ChangeIdType.COMMIT_HASH);
return asChangeNotes(query.byCommit(id));
}
@@ -185,7 +153,7 @@
Optional<ChangeTriplet> triplet = ChangeTriplet.parse(id, y, z);
if (triplet.isPresent()) {
ChangeTriplet t = triplet.get();
- checkIdType(ChangeIdType.TRIPLET, enforceDeprecation, triplet.get().toString());
+ changeIdCounter.increment(ChangeIdType.TRIPLET);
return asChangeNotes(query.byBranchKey(t.branch(), t.id()));
}
}
@@ -193,7 +161,7 @@
// Try isolated Ihash... format ("Change-Id: Ihash").
List<ChangeNotes> notes = asChangeNotes(query.byKeyPrefix(id));
if (!notes.isEmpty()) {
- checkIdType(ChangeIdType.I_HASH, enforceDeprecation, id);
+ changeIdCounter.increment(ChangeIdType.I_HASH);
}
return notes;
}
@@ -263,18 +231,4 @@
}
return notes;
}
-
- private void checkIdType(ChangeIdType type, boolean enforceDeprecation, String val)
- throws DeprecatedIdentifierException {
- if (enforceDeprecation
- && !allowedIdTypes.contains(ChangeIdType.ALL)
- && !allowedIdTypes.contains(type)) {
- throw new DeprecatedIdentifierException(
- String.format(
- "The provided change identifier %s is deprecated. "
- + "Use 'project~changeNumber' instead.",
- val));
- }
- changeIdCounter.increment(type);
- }
}
diff --git a/java/com/google/gerrit/server/restapi/change/ChangesCollection.java b/java/com/google/gerrit/server/restapi/change/ChangesCollection.java
index 561c27c..fd28ffb 100644
--- a/java/com/google/gerrit/server/restapi/change/ChangesCollection.java
+++ b/java/com/google/gerrit/server/restapi/change/ChangesCollection.java
@@ -85,7 +85,7 @@
@Override
public ChangeResource parse(TopLevelResource root, IdString id)
throws RestApiException, OrmException, PermissionBackendException, IOException {
- List<ChangeNotes> notes = changeFinder.find(id.encoded(), true);
+ List<ChangeNotes> notes = changeFinder.find(id.encoded());
if (notes.isEmpty()) {
throw new ResourceNotFoundException(id);
} else if (notes.size() != 1) {
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIdIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIdIT.java
index fe7da66..e0fc358 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIdIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIdIT.java
@@ -17,12 +17,10 @@
import static com.google.common.truth.Truth.assertThat;
import com.google.gerrit.acceptance.AbstractDaemonTest;
-import com.google.gerrit.acceptance.GerritConfig;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.extensions.api.changes.ChangeApi;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeInput;
-import com.google.gerrit.extensions.restapi.DeprecatedIdentifierException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.reviewdb.client.Project;
import org.junit.Before;
@@ -121,26 +119,4 @@
exception.expect(ResourceNotFoundException.class);
gApi.changes().id("I1234567890");
}
-
- @Test
- @GerritConfig(
- name = "change.api.allowedIdentifier",
- values = {"PROJECT_NUMERIC_ID", "NUMERIC_ID"})
- public void deprecatedChangeIdReturnsBadRequest() throws Exception {
- // project~changeNumber still works
- ChangeApi cApi1 = gApi.changes().id(project.get(), changeInfo._number);
- assertThat(cApi1.get().changeId).isEqualTo(changeInfo.changeId);
- // Change number still works
- ChangeApi cApi2 = gApi.changes().id(changeInfo._number);
- assertThat(cApi2.get().changeId).isEqualTo(changeInfo.changeId);
- // IHash throws
- ChangeInfo ci =
- gApi.changes().create(new ChangeInput(project.get(), "master", "different message")).get();
- exception.expect(DeprecatedIdentifierException.class);
- exception.expectMessage(
- "The provided change identifier "
- + ci.changeId
- + " is deprecated. Use 'project~changeNumber' instead.");
- gApi.changes().id(ci.changeId);
- }
}
