From e24bb8be8df1084406d2900c4bd0a7b51bb811d1 Mon Sep 17 00:00:00 2001
From: Dave Borowitz <dborowitz@google.com>
Date: Fri, 16 Jun 2017 10:22:02 -0400
Subject: [PATCH] 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
---
 .../java/com/google/gerrit/extensions/restapi/IdString.java     | 2 --
 .../main/java/com/google/gerrit/server/change/Revisions.java    | 2 +-
 .../java/com/google/gerrit/server/config/ConfigCollection.java  | 2 +-
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/IdString.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/IdString.java
index 58be3221a2..736c3ba9ab 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/IdString.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/IdString.java
@@ -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;
   }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Revisions.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Revisions.java
index 5ecb90428e..851202b778 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Revisions.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Revisions.java
@@ -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();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/ConfigCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/ConfigCollection.java
index d5d29608a7..f268110e02 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/ConfigCollection.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/ConfigCollection.java
@@ -44,7 +44,7 @@ public class ConfigCollection implements RestCollection<TopLevelResource, Config
 
   @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);