opendev
/
gerrit
Code Issues Proposed changes

IdString: Don't compare equal to strings 

Object#equals is supposed to define an equivalence relation[1][2]. This
includes the property of symmetry: a.equals(b) if and only if
b.equals(a). The old implementation of IdString#equals was asymmetrical:
it returned true when comparing to a String with the same urlencoded
value, but of course String#equals will always return false when
comparing to an IdString.

Fix IdString#equals to only compare equal to other IdStrings. It's
trivial and readable to fix the few usages in Gerrit core.

Since this method is part of the extension API, it's possible (though
personally I think highly unlikely) that plugins in the wild are
depending upon the current behavior, similar to how core was. Breakages
may be subtle, if for example callers were trying to look up Strings in
a Map<IdString, V>. However, it's worth noting that the asymmetric
behavior of equals meant that plugins were _already_ in for subtly
confusing or broken behavior.

At a minimum need to mention this behavior change in release notes.

[1] https://en.wikipedia.org/wiki/Equivalence_relation
[2] Effective Java, Item 8: "Obey the general contract when overriding
    equals"

Change-Id: I9d79b90640ea095b71803185d581cda77b93964e
(cherry picked from commit e24bb8be8d)
This commit is contained in:
Dave Borowitz 2017-06-16 10:22:02 -04:00 committed by David Ostrovsky
parent 08f5fd823a
commit 0b3affa4e2
3 changed files with 2 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/IdString.java
View File

@ -60,8 +60,6 @@ public class IdString {
public boolean equals(Object other) {
if (other instanceof IdString) {
return urlEncoded.equals(((IdString) other).urlEncoded);
} else if (other instanceof String) {
return urlEncoded.equals(other);
}
return false;
}

2
gerrit-server/src/main/java/com/google/gerrit/server/change/Revisions.java
View File

@ -71,7 +71,7 @@ public class Revisions implements ChildCollection<ChangeResource, RevisionResour
@Override
public RevisionResource parse(ChangeResource change, IdString id)
throws ResourceNotFoundException, AuthException, OrmException, IOException {
if (id.equals("current")) {
if (id.get().equals("current")) {
PatchSet ps = psUtil.current(dbProvider.get(), change.getNotes());
if (ps != null && visible(change, ps)) {
return new RevisionResource(change, ps).doNotCache();

2
gerrit-server/src/main/java/com/google/gerrit/server/config/ConfigCollection.java
View File

@ -45,7 +45,7 @@ public class ConfigCollection implements RestCollection<TopLevelResource, Config
@SuppressWarnings("unlikely-arg-type")
@Override
public ConfigResource parse(TopLevelResource root, IdString id) throws ResourceNotFoundException {
if (id.equals("server")) {
if (id.get().equals("server")) {
return new ConfigResource();
}
throw new ResourceNotFoundException(id);