Browse Source

Remove usages of CharSequence in Sets (#35501)

The javadocs of the CharSequence interface state that not all of its
implementations define the general contracts of the Object#equals and
Object#hashCode methods, therefore it is dangerous to use different CharSequence
instances as elements in a set or as keys in a map. While we probably mostly use
Strings in sets, in some places this is not enforced. To prevent this from
accidentally happening, this change replaces all occurances of Set<CharSequence>
which are currently mostly used in the completion suggester code with the more
concrete usage of Set<String>.
Christoph Büscher 7 years ago
parent
commit
2da4bc85cf

+ 4 - 4
server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java

@@ -514,11 +514,11 @@ public class CompletionFieldMapper extends FieldMapper implements ArrayValueMapp
                        XContentParser parser, Map<String, CompletionInputMetaData> inputMap) throws IOException {
         String currentFieldName = null;
         if (token == Token.VALUE_STRING) {
-            inputMap.put(parser.text(), new CompletionInputMetaData(parser.text(), Collections.emptyMap(), 1));
+            inputMap.put(parser.text(), new CompletionInputMetaData(parser.text(), Collections.<String, Set<String>>emptyMap(), 1));
         } else if (token == Token.START_OBJECT) {
             Set<String> inputs = new HashSet<>();
             int weight = 1;
-            Map<String, Set<CharSequence>> contextsMap = new HashMap<>();
+            Map<String, Set<String>> contextsMap = new HashMap<>();
             while ((token = parser.nextToken()) != Token.END_OBJECT) {
                 if (token == Token.FIELD_NAME) {
                     currentFieldName = parser.currentName();
@@ -603,10 +603,10 @@ public class CompletionFieldMapper extends FieldMapper implements ArrayValueMapp
 
     static class CompletionInputMetaData {
         public final String input;
-        public final Map<String, Set<CharSequence>> contexts;
+        public final Map<String, Set<String>> contexts;
         public final int weight;
 
-        CompletionInputMetaData(String input, Map<String, Set<CharSequence>> contexts, int weight) {
+        CompletionInputMetaData(String input, Map<String, Set<String>> contexts, int weight) {
             this.input = input;
             this.contexts = contexts;
             this.weight = weight;

+ 1 - 1
server/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggester.java

@@ -61,7 +61,7 @@ public class CompletionSuggester extends Suggester<CompletionSuggestionContext>
             int numResult = 0;
             for (TopSuggestDocs.SuggestScoreDoc suggestDoc : collector.get().scoreLookupDocs()) {
                 // collect contexts
-                Map<String, Set<CharSequence>> contexts = Collections.emptyMap();
+                Map<String, Set<String>> contexts = Collections.emptyMap();
                 if (fieldType.hasContextMappings()) {
                     List<CharSequence> rawContexts = collector.getContexts(suggestDoc.doc);
                     if (rawContexts.size() > 0) {

+ 11 - 11
server/src/main/java/org/elasticsearch/search/suggest/completion/CompletionSuggestion.java

@@ -280,13 +280,13 @@ public final class CompletionSuggestion extends Suggest.Suggestion<CompletionSug
         }
 
         public static class Option extends Suggest.Suggestion.Entry.Option {
-            private Map<String, Set<CharSequence>> contexts = Collections.emptyMap();
+            private Map<String, Set<String>> contexts = Collections.emptyMap();
             private ScoreDoc doc;
             private SearchHit hit;
 
             public static final ParseField CONTEXTS = new ParseField("contexts");
 
-            public Option(int docID, Text text, float score, Map<String, Set<CharSequence>> contexts) {
+            public Option(int docID, Text text, float score, Map<String, Set<String>> contexts) {
                 super(text, score);
                 this.doc = new ScoreDoc(docID, score);
                 this.contexts = Objects.requireNonNull(contexts, "context map cannot be null");
@@ -307,7 +307,7 @@ public final class CompletionSuggestion extends Suggest.Suggestion<CompletionSug
                 for (int i = 0; i < contextSize; i++) {
                     String contextName = in.readString();
                     int nContexts = in.readVInt();
-                    Set<CharSequence> contexts = new HashSet<>(nContexts);
+                    Set<String> contexts = new HashSet<>(nContexts);
                     for (int j = 0; j < nContexts; j++) {
                         contexts.add(in.readString());
                     }
@@ -322,7 +322,7 @@ public final class CompletionSuggestion extends Suggest.Suggestion<CompletionSug
                 throw new UnsupportedOperationException();
             }
 
-            public Map<String, Set<CharSequence>> getContexts() {
+            public Map<String, Set<String>> getContexts() {
                 return contexts;
             }
 
@@ -352,7 +352,7 @@ public final class CompletionSuggestion extends Suggest.Suggestion<CompletionSug
                 }
                 if (contexts.size() > 0) {
                     builder.startObject(CONTEXTS.getPreferredName());
-                    for (Map.Entry<String, Set<CharSequence>> entry : contexts.entrySet()) {
+                    for (Map.Entry<String, Set<String>> entry : contexts.entrySet()) {
                         builder.startArray(entry.getKey());
                         for (CharSequence context : entry.getValue()) {
                             builder.value(context.toString());
@@ -377,13 +377,13 @@ public final class CompletionSuggestion extends Suggest.Suggestion<CompletionSug
                         (p,c) -> parseContexts(p), CompletionSuggestion.Entry.Option.CONTEXTS);
             }
 
-            private static Map<String, Set<CharSequence>> parseContexts(XContentParser parser) throws IOException {
-                Map<String, Set<CharSequence>> contexts = new HashMap<>();
+            private static Map<String, Set<String>> parseContexts(XContentParser parser) throws IOException {
+                Map<String, Set<String>> contexts = new HashMap<>();
                 while((parser.nextToken()) != XContentParser.Token.END_OBJECT) {
                     ensureExpectedToken(XContentParser.Token.FIELD_NAME, parser.currentToken(), parser::getTokenLocation);
                     String key = parser.currentName();
                     ensureExpectedToken(XContentParser.Token.START_ARRAY, parser.nextToken(), parser::getTokenLocation);
-                    Set<CharSequence> values = new HashSet<>();
+                    Set<String> values = new HashSet<>();
                     while((parser.nextToken()) != XContentParser.Token.END_ARRAY) {
                         ensureExpectedToken(XContentParser.Token.VALUE_STRING, parser.currentToken(), parser::getTokenLocation);
                         values.add(parser.text());
@@ -399,7 +399,7 @@ public final class CompletionSuggestion extends Suggest.Suggestion<CompletionSug
                 Text text = new Text((String) values.get(Suggestion.Entry.Option.TEXT.getPreferredName()));
                 Float score = (Float) values.get(Suggestion.Entry.Option.SCORE.getPreferredName());
                 @SuppressWarnings("unchecked")
-                Map<String, Set<CharSequence>> contexts = (Map<String, Set<CharSequence>>) values
+                Map<String, Set<String>> contexts = (Map<String, Set<String>>) values
                         .get(CompletionSuggestion.Entry.Option.CONTEXTS.getPreferredName());
                 if (contexts == null) {
                     contexts = Collections.emptyMap();
@@ -427,7 +427,7 @@ public final class CompletionSuggestion extends Suggest.Suggestion<CompletionSug
                     out.writeBoolean(false);
                 }
                 out.writeInt(contexts.size());
-                for (Map.Entry<String, Set<CharSequence>> entry : contexts.entrySet()) {
+                for (Map.Entry<String, Set<String>> entry : contexts.entrySet()) {
                     out.writeString(entry.getKey());
                     out.writeVInt(entry.getValue().size());
                     for (CharSequence ctx : entry.getValue()) {
@@ -444,7 +444,7 @@ public final class CompletionSuggestion extends Suggest.Suggestion<CompletionSug
                 stringBuilder.append(" score:");
                 stringBuilder.append(getScore());
                 stringBuilder.append(" context:[");
-                for (Map.Entry<String, Set<CharSequence>> entry: contexts.entrySet()) {
+                for (Map.Entry<String, Set<String>> entry: contexts.entrySet()) {
                     stringBuilder.append(" ");
                     stringBuilder.append(entry.getKey());
                     stringBuilder.append(":");

+ 4 - 4
server/src/main/java/org/elasticsearch/search/suggest/completion/context/CategoryContextMapping.java

@@ -111,9 +111,9 @@ public class CategoryContextMapping extends ContextMapping<CategoryQueryContext>
      *  </ul>
      */
     @Override
-    public Set<CharSequence> parseContext(ParseContext parseContext, XContentParser parser)
+    public Set<String> parseContext(ParseContext parseContext, XContentParser parser)
             throws IOException, ElasticsearchParseException {
-        final Set<CharSequence> contexts = new HashSet<>();
+        final Set<String> contexts = new HashSet<>();
         Token token = parser.currentToken();
         if (token == Token.VALUE_STRING || token == Token.VALUE_NUMBER || token == Token.VALUE_BOOLEAN) {
             contexts.add(parser.text());
@@ -134,8 +134,8 @@ public class CategoryContextMapping extends ContextMapping<CategoryQueryContext>
     }
 
     @Override
-    public Set<CharSequence> parseContext(Document document) {
-        Set<CharSequence> values = null;
+    public Set<String> parseContext(Document document) {
+        Set<String> values = null;
         if (fieldName != null) {
             IndexableField[] fields = document.getFields(fieldName);
             values = new HashSet<>(fields.length);

+ 2 - 2
server/src/main/java/org/elasticsearch/search/suggest/completion/context/ContextMapping.java

@@ -95,12 +95,12 @@ public abstract class ContextMapping<T extends ToXContent> implements ToXContent
     /**
      * Parses a set of index-time contexts.
      */
-    public abstract Set<CharSequence> parseContext(ParseContext parseContext, XContentParser parser) throws IOException, ElasticsearchParseException;
+    public abstract Set<String> parseContext(ParseContext parseContext, XContentParser parser) throws IOException, ElasticsearchParseException;
 
     /**
      * Retrieves a set of context from a <code>document</code> at index-time.
      */
-    protected abstract Set<CharSequence> parseContext(ParseContext.Document document);
+    protected abstract Set<String> parseContext(ParseContext.Document document);
 
     /**
      * Prototype for the query context

+ 12 - 11
server/src/main/java/org/elasticsearch/search/suggest/completion/context/ContextMappings.java

@@ -22,6 +22,7 @@ package org.elasticsearch.search.suggest.completion.context;
 import org.apache.lucene.search.suggest.document.CompletionQuery;
 import org.apache.lucene.search.suggest.document.ContextQuery;
 import org.apache.lucene.search.suggest.document.ContextSuggestField;
+import org.apache.lucene.util.CharsRef;
 import org.apache.lucene.util.CharsRefBuilder;
 import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.Version;
@@ -94,7 +95,7 @@ public class ContextMappings implements ToXContent, Iterable<ContextMapping<?>>
      * Adds a context-enabled field for all the defined mappings to <code>document</code>
      * see {@link org.elasticsearch.search.suggest.completion.context.ContextMappings.TypedContextField}
      */
-    public void addField(ParseContext.Document document, String name, String input, int weight, Map<String, Set<CharSequence>> contexts) {
+    public void addField(ParseContext.Document document, String name, String input, int weight, Map<String, Set<String>> contexts) {
         document.add(new TypedContextField(name, input, weight, contexts, document));
     }
 
@@ -121,10 +122,10 @@ public class ContextMappings implements ToXContent, Iterable<ContextMapping<?>>
      * at index time
      */
     private class TypedContextField extends ContextSuggestField {
-        private final Map<String, Set<CharSequence>> contexts;
+        private final Map<String, Set<String>> contexts;
         private final ParseContext.Document document;
 
-        TypedContextField(String name, String value, int weight, Map<String, Set<CharSequence>> contexts,
+        TypedContextField(String name, String value, int weight, Map<String, Set<String>> contexts,
                           ParseContext.Document document) {
             super(name, value, weight);
             this.contexts = contexts;
@@ -133,18 +134,18 @@ public class ContextMappings implements ToXContent, Iterable<ContextMapping<?>>
 
         @Override
         protected Iterable<CharSequence> contexts() {
-            Set<CharSequence> typedContexts = new HashSet<>();
+            Set<CharsRef> typedContexts = new HashSet<>();
             final CharsRefBuilder scratch = new CharsRefBuilder();
             scratch.grow(1);
             for (int typeId = 0; typeId < contextMappings.size(); typeId++) {
                 scratch.setCharAt(0, (char) typeId);
                 scratch.setLength(1);
                 ContextMapping<?> mapping = contextMappings.get(typeId);
-                Set<CharSequence> contexts = new HashSet<>(mapping.parseContext(document));
+                Set<String> contexts = new HashSet<>(mapping.parseContext(document));
                 if (this.contexts.get(mapping.name()) != null) {
                     contexts.addAll(this.contexts.get(mapping.name()));
                 }
-                for (CharSequence context : contexts) {
+                for (String context : contexts) {
                     scratch.append(context);
                     typedContexts.add(scratch.toCharsRef());
                     scratch.setLength(1);
@@ -153,7 +154,7 @@ public class ContextMappings implements ToXContent, Iterable<ContextMapping<?>>
             if (typedContexts.isEmpty()) {
                 throw new IllegalArgumentException("Contexts are mandatory in context enabled completion field [" + name + "]");
             }
-            return typedContexts;
+            return new ArrayList<CharSequence>(typedContexts);
         }
     }
 
@@ -198,18 +199,18 @@ public class ContextMappings implements ToXContent, Iterable<ContextMapping<?>>
      * @return a map of context names and their values
      *
      */
-    public Map<String, Set<CharSequence>> getNamedContexts(List<CharSequence> contexts) {
-        Map<String, Set<CharSequence>> contextMap = new HashMap<>(contexts.size());
+    public Map<String, Set<String>> getNamedContexts(List<CharSequence> contexts) {
+        Map<String, Set<String>> contextMap = new HashMap<>(contexts.size());
         for (CharSequence typedContext : contexts) {
             int typeId = typedContext.charAt(0);
             assert typeId < contextMappings.size() : "Returned context has invalid type";
             ContextMapping<?> mapping = contextMappings.get(typeId);
-            Set<CharSequence> contextEntries = contextMap.get(mapping.name());
+            Set<String> contextEntries = contextMap.get(mapping.name());
             if (contextEntries == null) {
                 contextEntries = new HashSet<>();
                 contextMap.put(mapping.name(), contextEntries);
             }
-            contextEntries.add(typedContext.subSequence(1, typedContext.length()));
+            contextEntries.add(typedContext.subSequence(1, typedContext.length()).toString());
         }
         return contextMap;
     }

+ 8 - 8
server/src/main/java/org/elasticsearch/search/suggest/completion/context/GeoContextMapping.java

@@ -144,14 +144,14 @@ public class GeoContextMapping extends ContextMapping<GeoQueryContext> {
      * see {@code GeoPoint(String)} for GEO POINT
      */
     @Override
-    public Set<CharSequence> parseContext(ParseContext parseContext, XContentParser parser) throws IOException, ElasticsearchParseException {
+    public Set<String> parseContext(ParseContext parseContext, XContentParser parser) throws IOException, ElasticsearchParseException {
         if (fieldName != null) {
             MappedFieldType fieldType = parseContext.mapperService().fullName(fieldName);
             if (!(fieldType instanceof GeoPointFieldMapper.GeoPointFieldType)) {
                 throw new ElasticsearchParseException("referenced field must be mapped to geo_point");
             }
         }
-        final Set<CharSequence> contexts = new HashSet<>();
+        final Set<String> contexts = new HashSet<>();
         Token token = parser.currentToken();
         if (token == Token.START_ARRAY) {
             token = parser.nextToken();
@@ -178,7 +178,7 @@ public class GeoContextMapping extends ContextMapping<GeoQueryContext> {
         } else if (token == Token.VALUE_STRING) {
             final String geoHash = parser.text();
             final CharSequence truncatedGeoHash = geoHash.subSequence(0, Math.min(geoHash.length(), precision));
-            contexts.add(truncatedGeoHash);
+            contexts.add(truncatedGeoHash.toString());
         } else {
             // or a single location
             GeoPoint point = GeoUtils.parseGeoPoint(parser);
@@ -188,8 +188,8 @@ public class GeoContextMapping extends ContextMapping<GeoQueryContext> {
     }
 
     @Override
-    public Set<CharSequence> parseContext(Document document) {
-        final Set<CharSequence> geohashes = new HashSet<>();
+    public Set<String> parseContext(Document document) {
+        final Set<String> geohashes = new HashSet<>();
 
         if (fieldName != null) {
             IndexableField[] fields = document.getFields(fieldName);
@@ -222,10 +222,10 @@ public class GeoContextMapping extends ContextMapping<GeoQueryContext> {
             }
         }
 
-        Set<CharSequence> locations = new HashSet<>();
-        for (CharSequence geohash : geohashes) {
+        Set<String> locations = new HashSet<>();
+        for (String geohash : geohashes) {
             int precision = Math.min(this.precision, geohash.length());
-            CharSequence truncatedGeohash = geohash.subSequence(0, precision);
+            String truncatedGeohash = geohash.substring(0, precision);
             locations.add(truncatedGeohash);
         }
         return locations;

+ 3 - 3
server/src/test/java/org/elasticsearch/search/suggest/CompletionSuggestionOptionTests.java

@@ -48,10 +48,10 @@ public class CompletionSuggestionOptionTests extends ESTestCase {
         Text text = new Text(randomAlphaOfLengthBetween(5, 15));
         int docId = randomInt();
         int numberOfContexts = randomIntBetween(0, 3);
-        Map<String, Set<CharSequence>> contexts = new HashMap<>();
+        Map<String, Set<String>> contexts = new HashMap<>();
         for (int i = 0; i < numberOfContexts; i++) {
             int numberOfValues = randomIntBetween(0, 3);
-            Set<CharSequence> values = new HashSet<>();
+            Set<String> values = new HashSet<>();
             for (int v = 0; v < numberOfValues; v++) {
                 values.add(randomAlphaOfLengthBetween(5, 15));
             }
@@ -106,7 +106,7 @@ public class CompletionSuggestionOptionTests extends ESTestCase {
     }
 
     public void testToXContent() throws IOException {
-        Map<String, Set<CharSequence>> contexts = Collections.singletonMap("key", Collections.singleton("value"));
+        Map<String, Set<String>> contexts = Collections.singletonMap("key", Collections.singleton("value"));
         CompletionSuggestion.Entry.Option option = new CompletionSuggestion.Entry.Option(1, new Text("someText"), 1.3f, contexts);
         BytesReference xContent = toXContent(option, XContentType.JSON, randomBoolean());
         assertEquals("{\"text\":\"someText\",\"score\":1.3,\"contexts\":{\"key\":[\"value\"]}}"

+ 1 - 1
server/src/test/java/org/elasticsearch/search/suggest/SuggestionTests.java

@@ -249,7 +249,7 @@ public class SuggestionTests extends ESTestCase {
                     + "}", xContent.utf8ToString());
         }
         {
-            Map<String, Set<CharSequence>> contexts = Collections.singletonMap("key", Collections.singleton("value"));
+            Map<String, Set<String>> contexts = Collections.singletonMap("key", Collections.singleton("value"));
             CompletionSuggestion.Entry.Option option = new CompletionSuggestion.Entry.Option(1, new Text("someText"), 1.3f, contexts);
             CompletionSuggestion.Entry entry = new CompletionSuggestion.Entry(new Text("entryText"), 42, 313);
             entry.addOption(option);

+ 1 - 1
server/src/test/java/org/elasticsearch/search/suggest/completion/CategoryContextMappingTests.java

@@ -724,7 +724,7 @@ public class CategoryContextMappingTests extends ESSingleNodeTestCase {
         document.add(new Field(keyword.name(), new BytesRef("category1"), keyword));
         // Ignore doc values
         document.add(new SortedSetDocValuesField(keyword.name(), new BytesRef("category1")));
-        Set<CharSequence> context = mapping.parseContext(document);
+        Set<String> context = mapping.parseContext(document);
         assertThat(context.size(), equalTo(1));
         assertTrue(context.contains("category1"));