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
This commit is contained in:
parent
c9dc6e9a11
commit
a592397ebf
Documentation
java/com/google/gerrit
extensions/restapi
server
javatests/com/google/gerrit/acceptance/api/change
@ -1180,18 +1180,6 @@ used.
|
||||
+
|
||||
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
|
||||
|
@ -5498,8 +5498,7 @@ In this case, use a POST request instead:
|
||||
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 @@ deprecated identifiers. These will be removed in a future release:
|
||||
("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].
|
||||
|
@ -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);
|
||||
}
|
||||
}
|
@ -17,10 +17,8 @@ package com.google.gerrit.server.change;
|
||||
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.Project;
|
||||
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.List;
|
||||
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 @@ public class ChangeFinder {
|
||||
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 @@ public class ChangeFinder {
|
||||
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 @@ public class ChangeFinder {
|
||||
.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 @@ public class ChangeFinder {
|
||||
*
|
||||
* @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 @@ public class ChangeFinder {
|
||||
// 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 @@ public class ChangeFinder {
|
||||
// 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 @@ public class ChangeFinder {
|
||||
|
||||
// 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 @@ public class ChangeFinder {
|
||||
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 @@ public class ChangeFinder {
|
||||
// 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 @@ public class ChangeFinder {
|
||||
}
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
@ -85,7 +85,7 @@ public class ChangesCollection implements RestCollection<TopLevelResource, Chang
|
||||
@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) {
|
||||
|
@ -17,12 +17,10 @@ package com.google.gerrit.acceptance.api.change;
|
||||
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 @@ public class ChangeIdIT extends AbstractDaemonTest {
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
Loading…
x
Reference in New Issue
Block a user