Support labels >33 chars in LabelVote#parseWithEquals

In 57ecf24c back in 2013, I accidentally wrote this:
  Short.parseShort(text.substring(e + 1), text.length())
where *obviously* I meant this:
  Short.parseShort(text.substring(e + 1, text.length()))

This called Short#parseShort(String, int), where the second arg is a
radix. (The second arg to substring is also not necessary in this case,
but would have been harmlessly correct if it were present.)

It's surprising and lucky that passing an arbitrary radix managed to
have correct behavior in practically all cases, but this was in fact the
case due a combination of two facts:
  * text.length() is always at least 3 ("X=0"), usually longer, and
    only exceeds Character.MAX_RADIX (36) when the label name exceeds 33
    characters.
  * The value itself is rarely greater than 2 or 3, essentially never
    requiring multiple digits (unless someone were foolhardy enough to
    use a very large range), so any radix greater than or equal to the
    value would result in parsing the value correctly.

As an amusing side effect, the old code would parse "Code-Review=c"
"correctly," but would fail on "Code-Review=d".

Fix this bug and increase test coverage.

Change-Id: I17f4fb963e1e0d7b7bd51d689e7557cbcfc8b474
This commit is contained in:
Dave Borowitz 2017-08-23 14:34:18 -04:00
parent f0c81b6a89
commit 0085f48710
2 changed files with 31 additions and 1 deletions

View File

@ -52,7 +52,7 @@ public abstract class LabelVote {
checkArgument(!Strings.isNullOrEmpty(text), "Empty label vote");
int e = text.lastIndexOf('=');
checkArgument(e >= 0, "Label vote missing '=': %s", text);
return create(text.substring(0, e), Short.parseShort(text.substring(e + 1), text.length()));
return create(text.substring(0, e), Short.parseShort(text.substring(e + 1)));
}
public static StringBuilder appendTo(StringBuilder sb, String label, short value) {

View File

@ -15,6 +15,8 @@
package com.google.gerrit.server.util;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import org.junit.Test;
@ -75,6 +77,25 @@ public class LabelVoteTest {
l = LabelVote.parseWithEquals("Code-Review=+2");
assertEquals("Code-Review", l.label());
assertEquals((short) 2, l.value());
l = LabelVote.parseWithEquals("R=0");
assertEquals("R", l.label());
assertEquals((short) 0, l.value());
String longName = "A-loooooooooooooooooooooooooooooooooooooooooooooooooong-label";
// Regression test: an old bug passed the string length as a radix to Short#parseShort.
assertTrue(longName.length() > Character.MAX_RADIX);
l = LabelVote.parseWithEquals(longName + "=+1");
assertEquals(longName, l.label());
assertEquals((short) 1, l.value());
assertParseWithEqualsFails(null);
assertParseWithEqualsFails("");
assertParseWithEqualsFails("Code-Review");
assertParseWithEqualsFails("=1");
assertParseWithEqualsFails("=.1");
assertParseWithEqualsFails("=a1");
assertParseWithEqualsFails("=1a");
assertParseWithEqualsFails("=.");
}
@Test
@ -85,4 +106,13 @@ public class LabelVoteTest {
assertEquals("Code-Review=+1", LabelVote.parseWithEquals("Code-Review=+1").formatWithEquals());
assertEquals("Code-Review=+2", LabelVote.parseWithEquals("Code-Review=+2").formatWithEquals());
}
private void assertParseWithEqualsFails(String value) {
try {
LabelVote.parseWithEquals(value);
fail("expected IllegalArgumentException when parsing \"" + value + "\"");
} catch (IllegalArgumentException e) {
// Expected.
}
}
}