diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 6dfb65359a1..441d85ee85c 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -196,6 +196,10 @@ Bug Fixes * SOLR-6136: ConcurrentUpdateSolrServer includes a Spin Lock (Brandon Chapman, Timothy Potter) +* SOLR-6257: More than two "!"-s in a doc ID throws an + ArrayIndexOutOfBoundsException when using the composite id router. + (Steve Rowe) + Optimizations --------------------- diff --git a/solr/core/src/test/org/apache/solr/cloud/TestHashPartitioner.java b/solr/core/src/test/org/apache/solr/cloud/TestHashPartitioner.java index 8ddca1d2e9b..ee386ac71a4 100644 --- a/solr/core/src/test/org/apache/solr/cloud/TestHashPartitioner.java +++ b/solr/core/src/test/org/apache/solr/cloud/TestHashPartitioner.java @@ -23,6 +23,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import org.apache.lucene.util.TestUtil; import org.apache.solr.SolrTestCaseJ4; import org.apache.solr.common.cloud.CompositeIdRouter; import org.apache.solr.common.cloud.DocCollection; @@ -197,25 +198,73 @@ public class TestHashPartitioner extends SolrTestCaseJ4 { doQuery(coll, "d/1!", "shard3,shard4"); // top bit of hash(b)==0, so shard3 and shard4 } - /*** - public void testPrintHashCodes() throws Exception { - // from negative to positive, the upper bits of the hash ranges should be - // shard1: 11 - // shard2: 10 - // shard3: 00 - // shard4: 01 - - String[] highBitsToShard = {"shard3","shard4","shard1","shard2"}; - - - for (int i = 0; i<26; i++) { - String id = new String(Character.toChars('a'+i)); - int hash = hash(id); - System.out.println("hash of " + id + " is " + Integer.toHexString(hash) + " high bits=" + (hash>>>30) - + " shard="+highBitsToShard[hash>>>30]); + /** Make sure CompositeIdRouter doesn't throw exceptions for non-conforming IDs */ + public void testNonConformingCompositeIds() throws Exception { + DocRouter router = DocRouter.getDocRouter(CompositeIdRouter.NAME); + DocCollection coll = createCollection(4, router); + String[] ids = { "A!B!C!D", "!!!!!!", "A!!!!B", "A!!B!!C", "A/59!B", "A/8/!B/19/", + "A!B/-5", "!/130!", "!!A/1000", "A//8!B///10!C////" }; + for (int i = 0 ; i < ids.length ; ++i) { + try { + Slice targetSlice = coll.getRouter().getTargetSlice(ids[i], null, null, coll); + assertNotNull(targetSlice); + } catch (Exception e) { + throw new Exception("Exception routing id '" + ids[i] + "'", e); + } } } - ***/ + + /** Make sure CompositeIdRouter can route random IDs without throwing exceptions */ + public void testRandomCompositeIds() throws Exception { + DocRouter router = DocRouter.getDocRouter(CompositeIdRouter.NAME); + DocCollection coll = createCollection(TestUtil.nextInt(random(), 1, 10), router); + StringBuilder idBuilder = new StringBuilder(); + for (int i = 0 ; i < 10000 ; ++i) { + idBuilder.setLength(0); + int numParts = TestUtil.nextInt(random(), 1, 30); + for (int part = 0; part < numParts; ++part) { + switch (random().nextInt(5)) { + case 0: idBuilder.append('!'); break; + case 1: idBuilder.append('/'); break; + case 2: idBuilder.append(TestUtil.nextInt(random(),-100, 1000)); break; + default: { + int length = TestUtil.nextInt(random(), 1, 10); + char[] str = new char[length]; + TestUtil.randomFixedLengthUnicodeString(random(), str, 0, length); + idBuilder.append(str); + break; + } + } + } + String id = idBuilder.toString(); + try { + Slice targetSlice = router.getTargetSlice(id, null, null, coll); + assertNotNull(targetSlice); + } catch (Exception e) { + throw new Exception("Exception routing id '" + id + "'", e); + } + } + } + + /*** + public void testPrintHashCodes() throws Exception { + // from negative to positive, the upper bits of the hash ranges should be + // shard1: 11 + // shard2: 10 + // shard3: 00 + // shard4: 01 + + String[] highBitsToShard = {"shard3","shard4","shard1","shard2"}; + + + for (int i = 0; i<26; i++) { + String id = new String(Character.toChars('a'+i)); + int hash = hash(id); + System.out.println("hash of " + id + " is " + Integer.toHexString(hash) + " high bits=" + (hash>>>30) + + " shard="+highBitsToShard[hash>>>30]); + } + } + ***/ diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/CompositeIdRouter.java b/solr/solrj/src/java/org/apache/solr/common/cloud/CompositeIdRouter.java index d01644ff6aa..5b1652d4088 100644 --- a/solr/solrj/src/java/org/apache/solr/common/cloud/CompositeIdRouter.java +++ b/solr/solrj/src/java/org/apache/solr/common/cloud/CompositeIdRouter.java @@ -187,10 +187,36 @@ public class CompositeIdRouter extends HashBasedRouter { boolean triLevel; int pieces; - public KeyParser(String key) { - String[] parts = key.split(SEPARATOR); + public KeyParser(final String key) { this.key = key; - pieces = parts.length; + List partsList = new ArrayList<>(3); + int firstSeparatorPos = key.indexOf(SEPARATOR); + if (-1 == firstSeparatorPos) { + partsList.add(key); + } else { + partsList.add(key.substring(0, firstSeparatorPos)); + int lastPos = key.length() - 1; + // Don't make any more parts if the first separator is the last char + if (firstSeparatorPos < lastPos) { + int secondSeparatorPos = key.indexOf(SEPARATOR, firstSeparatorPos + 1); + if (-1 == secondSeparatorPos) { + partsList.add(key.substring(firstSeparatorPos + 1)); + } else if (secondSeparatorPos == lastPos) { + // Don't make any more parts if the key has exactly two separators and + // they're the last two chars - back-compatibility with the behavior of + // String.split() - see SOLR-6257. + if (firstSeparatorPos < secondSeparatorPos - 1) { + partsList.add(key.substring(firstSeparatorPos + 1, secondSeparatorPos)); + } + } else { // The second separator is not the last char + partsList.add(key.substring(firstSeparatorPos + 1, secondSeparatorPos)); + partsList.add(key.substring(secondSeparatorPos + 1)); + } + // Ignore any further separators beyond the first two + } + } + pieces = partsList.size(); + String[] parts = partsList.toArray(new String[pieces]); numBits = new int[2]; if (key.endsWith("!") && pieces < 3) pieces++;