Add a config option to deprecate change identifiers one-by-one
This commit adds a config option to deprecate change identifiers on a
one-by-one basis. This allows host admins to gradually turn off
deprecated identifiers.
This change only affects the API endpoints. Internals can still use old
identifiers to get changes.
Change-Id: I97a7ad4b82cb48be31af996f391d0dc9388fa406
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 45bc045..91a837d 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -1133,6 +1133,18 @@
+
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.cacheAutomerge]]change.cacheAutomerge::
+
When reviewing diff commits, the left-hand side shows the output of the
diff --git a/java/com/google/gerrit/extensions/restapi/DeprecatedIdentifierException.java b/java/com/google/gerrit/extensions/restapi/DeprecatedIdentifierException.java
new file mode 100644
index 0000000..aa28cfc
--- /dev/null
+++ b/java/com/google/gerrit/extensions/restapi/DeprecatedIdentifierException.java
@@ -0,0 +1,25 @@
+// 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/ChangeFinder.java b/java/com/google/gerrit/server/ChangeFinder.java
index 4b7cbac..cb82778 100644
--- a/java/com/google/gerrit/server/ChangeFinder.java
+++ b/java/com/google/gerrit/server/ChangeFinder.java
@@ -17,8 +17,10 @@
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.index.IndexConfig;
import com.google.gerrit.metrics.Counter1;
import com.google.gerrit.metrics.Description;
@@ -30,6 +32,8 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.change.ChangeTriplet;
+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;
@@ -46,6 +50,7 @@
import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.eclipse.jgit.lib.Config;
@Singleton
public class ChangeFinder {
@@ -60,10 +65,11 @@
};
}
- private enum ChangeIdType {
+ public enum ChangeIdType {
+ ALL,
TRIPLET,
NUMERIC_ID,
- CHANGE_ID,
+ I_HASH,
PROJECT_NUMERIC_ID,
COMMIT_HASH
}
@@ -74,6 +80,7 @@
private final Provider<ReviewDb> reviewDb;
private final ChangeNotes.Factory changeNotesFactory;
private final Counter1<ChangeIdType> changeIdCounter;
+ private final ImmutableSet<ChangeIdType> allowedIdTypes;
@Inject
ChangeFinder(
@@ -82,7 +89,8 @@
Provider<InternalChangeQuery> queryProvider,
Provider<ReviewDb> reviewDb,
ChangeNotes.Factory changeNotesFactory,
- MetricMaker metricMaker) {
+ MetricMaker metricMaker,
+ @GerritServerConfig Config config) {
this.indexConfig = indexConfig;
this.changeIdProjectCache = changeIdProjectCache;
this.queryProvider = queryProvider;
@@ -95,16 +103,41 @@
.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);
}
/**
* Find changes matching the given identifier.
*
- * @param id change identifier, either a numeric ID, a Change-Id, or project~branch~id triplet.
+ * @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.
*/
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();
}
@@ -115,7 +148,7 @@
// Try project~numericChangeId
Integer n = Ints.tryParse(id.substring(z + 1));
if (n != null) {
- changeIdCounter.increment(ChangeIdType.PROJECT_NUMERIC_ID);
+ checkIdType(ChangeIdType.PROJECT_NUMERIC_ID, enforceDeprecation, n.toString());
return fromProjectNumber(id.substring(0, z), n.intValue());
}
}
@@ -124,7 +157,7 @@
// Try numeric changeId
Integer n = Ints.tryParse(id);
if (n != null) {
- changeIdCounter.increment(ChangeIdType.NUMERIC_ID);
+ checkIdType(ChangeIdType.NUMERIC_ID, enforceDeprecation, n.toString());
return find(new Change.Id(n));
}
}
@@ -135,7 +168,7 @@
// Try commit hash
if (id.matches("^([0-9a-fA-F]{" + RevId.ABBREV_LEN + "," + RevId.LEN + "})$")) {
- changeIdCounter.increment(ChangeIdType.COMMIT_HASH);
+ checkIdType(ChangeIdType.COMMIT_HASH, enforceDeprecation, id);
return asChangeNotes(query.byCommit(id));
}
@@ -144,7 +177,7 @@
Optional<ChangeTriplet> triplet = ChangeTriplet.parse(id, y, z);
if (triplet.isPresent()) {
ChangeTriplet t = triplet.get();
- changeIdCounter.increment(ChangeIdType.TRIPLET);
+ checkIdType(ChangeIdType.TRIPLET, enforceDeprecation, triplet.get().toString());
return asChangeNotes(query.byBranchKey(t.branch(), t.id()));
}
}
@@ -152,7 +185,7 @@
// Try isolated Ihash... format ("Change-Id: Ihash").
List<ChangeNotes> notes = asChangeNotes(query.byKeyPrefix(id));
if (!notes.isEmpty()) {
- changeIdCounter.increment(ChangeIdType.CHANGE_ID);
+ checkIdType(ChangeIdType.I_HASH, enforceDeprecation, id);
}
return notes;
}
@@ -222,4 +255,18 @@
}
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/account/StarredChanges.java b/java/com/google/gerrit/server/account/StarredChanges.java
index 38c95e6..6dfd132 100644
--- a/java/com/google/gerrit/server/account/StarredChanges.java
+++ b/java/com/google/gerrit/server/account/StarredChanges.java
@@ -72,7 +72,7 @@
@Override
public AccountResource.StarredChange parse(AccountResource parent, IdString id)
- throws ResourceNotFoundException, OrmException, PermissionBackendException {
+ throws RestApiException, OrmException, PermissionBackendException {
IdentifiedUser user = parent.getUser();
ChangeResource change = changes.parse(TopLevelResource.INSTANCE, id);
if (starredChangesUtil
@@ -103,7 +103,7 @@
@Override
public RestModifyView<AccountResource, EmptyInput> create(AccountResource parent, IdString id)
- throws UnprocessableEntityException {
+ throws RestApiException {
try {
return createProvider.get().setChange(changes.parse(TopLevelResource.INSTANCE, id));
} catch (ResourceNotFoundException e) {
diff --git a/java/com/google/gerrit/server/account/Stars.java b/java/com/google/gerrit/server/account/Stars.java
index 9019ad7..5eb8d7b 100644
--- a/java/com/google/gerrit/server/account/Stars.java
+++ b/java/com/google/gerrit/server/account/Stars.java
@@ -21,7 +21,7 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ChildCollection;
import com.google.gerrit.extensions.restapi.IdString;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.extensions.restapi.RestView;
@@ -66,7 +66,7 @@
@Override
public Star parse(AccountResource parent, IdString id)
- throws ResourceNotFoundException, OrmException, PermissionBackendException {
+ throws RestApiException, OrmException, PermissionBackendException {
IdentifiedUser user = parent.getUser();
ChangeResource change = changes.parse(TopLevelResource.INSTANCE, id);
Set<String> labels = starredChangesUtil.getLabels(user.getAccountId(), change.getId());
diff --git a/java/com/google/gerrit/server/change/ChangesCollection.java b/java/com/google/gerrit/server/change/ChangesCollection.java
index a0b6c96..6ce661e 100644
--- a/java/com/google/gerrit/server/change/ChangesCollection.java
+++ b/java/com/google/gerrit/server/change/ChangesCollection.java
@@ -81,8 +81,8 @@
@Override
public ChangeResource parse(TopLevelResource root, IdString id)
- throws ResourceNotFoundException, OrmException, PermissionBackendException {
- List<ChangeNotes> notes = changeFinder.find(id.encoded());
+ throws RestApiException, OrmException, PermissionBackendException {
+ List<ChangeNotes> notes = changeFinder.find(id.encoded(), true);
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 e0fc358..0b7f340 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIdIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIdIT.java
@@ -17,10 +17,12 @@
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;
@@ -119,4 +121,27 @@
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);
+ }
}
