From c20d44a1ffb3272bbe64098f18587eb8e52f54ee Mon Sep 17 00:00:00 2001 From: Adrien Grand Date: Thu, 13 Jun 2013 09:23:01 +0200 Subject: [PATCH] Forbid usage of Character.codePoint(At|Before) and Collections.sort. Character.codePointAt and codePointBefore have two versions: one which only accepts an offset, and one which accepts an offset and a limit. The former can be dangerous when working with buffers of characters because if the offset is the last char of the buffer, a char outside the buffer might be used to compute the code point, so one should always use the version which accepts a limit. Collections.sort is wasteful on random-access lists: it dumps data into an array, sorts the list and then adds elements back to the list. However, the sorting can easily be performed in-place by using Lucene's CollectionUtil.(merge|quick|tim)Sort. --- core-signatures.txt | 8 ++++++++ .../metadata/MetaDataCreateIndexService.java | 6 ++---- .../common/inject/internal/Errors.java | 3 ++- .../common/network/NetworkUtils.java | 3 ++- .../zen/elect/ElectMasterService.java | 4 ++-- .../index/gateway/CommitPoints.java | 4 ++-- .../elasticsearch/monitor/jvm/HotThreads.java | 3 ++- .../InternalFullDateHistogramFacet.java | 3 ++- .../histogram/InternalFullHistogramFacet.java | 3 ++- .../InternalTermsStatsDoubleFacet.java | 4 +++- .../longs/InternalTermsStatsLongFacet.java | 3 ++- .../InternalTermsStatsStringFacet.java | 3 ++- .../search/highlight/PlainHighlighter.java | 10 ++++++---- .../FragmentBuilderHelper.java | 20 ++++++------------- .../elasticsearch/search/suggest/Suggest.java | 15 +++++--------- 15 files changed, 48 insertions(+), 44 deletions(-) diff --git a/core-signatures.txt b/core-signatures.txt index a502959bdaf..1b7c2301b1a 100644 --- a/core-signatures.txt +++ b/core-signatures.txt @@ -25,3 +25,11 @@ java.util.concurrent.Executors#newSingleThreadScheduledExecutor() java.util.concurrent.Executors#newScheduledThreadPool(int) java.util.concurrent.Executors#defaultThreadFactory() java.util.concurrent.Executors#privilegedThreadFactory() + +java.lang.Character#codePointBefore(char[],int) @ Implicit start offset is error-prone when the char[] is a buffer and the first chars are random chars +java.lang.Character#codePointAt(char[],int) @ Implicit end offset is error-prone when the char[] is a buffer and the last chars are random chars + +@defaultMessage Collections.sort dumps data into an array, sorts the array and reinserts data into the list, one should rather use Lucene's CollectionUtil sort methods which sort in place + +java.util.Collections#sort(java.util.List) +java.util.Collections#sort(java.util.List,java.util.Comparator) diff --git a/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java b/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java index 931a0dee914..68747ea0376 100644 --- a/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java +++ b/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java @@ -24,7 +24,7 @@ import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Sets; import com.google.common.io.Closeables; - +import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.ElasticSearchException; import org.elasticsearch.Version; import org.elasticsearch.cluster.ClusterService; @@ -64,10 +64,8 @@ import org.elasticsearch.threadpool.ThreadPool; import java.io.File; import java.io.FileInputStream; -import java.io.FileReader; import java.io.IOException; import java.io.InputStreamReader; -import java.nio.charset.Charset; import java.util.*; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.atomic.AtomicBoolean; @@ -481,7 +479,7 @@ public class MetaDataCreateIndexService extends AbstractComponent { } } - Collections.sort(templates, new Comparator() { + CollectionUtil.quickSort(templates, new Comparator() { @Override public int compare(IndexTemplateMetaData o1, IndexTemplateMetaData o2) { return o2.order() - o1.order(); diff --git a/src/main/java/org/elasticsearch/common/inject/internal/Errors.java b/src/main/java/org/elasticsearch/common/inject/internal/Errors.java index 285134dd113..cd9d32f79e1 100644 --- a/src/main/java/org/elasticsearch/common/inject/internal/Errors.java +++ b/src/main/java/org/elasticsearch/common/inject/internal/Errors.java @@ -19,6 +19,7 @@ package org.elasticsearch.common.inject.internal; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; +import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.common.inject.*; import org.elasticsearch.common.inject.spi.*; @@ -439,7 +440,7 @@ public final class Errors implements Serializable { } List result = Lists.newArrayList(root.errors); - Collections.sort(result, new Comparator() { + CollectionUtil.timSort(result, new Comparator() { public int compare(Message a, Message b) { return a.getSource().compareTo(b.getSource()); } diff --git a/src/main/java/org/elasticsearch/common/network/NetworkUtils.java b/src/main/java/org/elasticsearch/common/network/NetworkUtils.java index ad95e09bb19..756593d1d65 100644 --- a/src/main/java/org/elasticsearch/common/network/NetworkUtils.java +++ b/src/main/java/org/elasticsearch/common/network/NetworkUtils.java @@ -20,6 +20,7 @@ package org.elasticsearch.common.network; import com.google.common.collect.Lists; +import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.ElasticSearchIllegalStateException; import org.elasticsearch.common.logging.ESLogger; import org.elasticsearch.common.logging.Loggers; @@ -109,7 +110,7 @@ public abstract class NetworkUtils { final Method getIndexMethod = NetworkInterface.class.getDeclaredMethod("getIndex"); getIndexMethod.setAccessible(true); - Collections.sort(intfsList, new Comparator() { + CollectionUtil.timSort(intfsList, new Comparator() { @Override public int compare(NetworkInterface o1, NetworkInterface o2) { try { diff --git a/src/main/java/org/elasticsearch/discovery/zen/elect/ElectMasterService.java b/src/main/java/org/elasticsearch/discovery/zen/elect/ElectMasterService.java index e0192a45739..cc0ea179c1e 100644 --- a/src/main/java/org/elasticsearch/discovery/zen/elect/ElectMasterService.java +++ b/src/main/java/org/elasticsearch/discovery/zen/elect/ElectMasterService.java @@ -20,11 +20,11 @@ package org.elasticsearch.discovery.zen.elect; import com.google.common.collect.Lists; +import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.settings.Settings; -import java.util.Collections; import java.util.Comparator; import java.util.Iterator; import java.util.List; @@ -110,7 +110,7 @@ public class ElectMasterService extends AbstractComponent { it.remove(); } } - Collections.sort(possibleNodes, nodeComparator); + CollectionUtil.quickSort(possibleNodes, nodeComparator); return possibleNodes; } diff --git a/src/main/java/org/elasticsearch/index/gateway/CommitPoints.java b/src/main/java/org/elasticsearch/index/gateway/CommitPoints.java index aa6bb932135..b071d7b51b0 100644 --- a/src/main/java/org/elasticsearch/index/gateway/CommitPoints.java +++ b/src/main/java/org/elasticsearch/index/gateway/CommitPoints.java @@ -21,13 +21,13 @@ package org.elasticsearch.index.gateway; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; +import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; import java.io.IOException; -import java.util.Collections; import java.util.Comparator; import java.util.Iterator; import java.util.List; @@ -40,7 +40,7 @@ public class CommitPoints implements Iterable { private final ImmutableList commitPoints; public CommitPoints(List commitPoints) { - Collections.sort(commitPoints, new Comparator() { + CollectionUtil.quickSort(commitPoints, new Comparator() { @Override public int compare(CommitPoint o1, CommitPoint o2) { return (o2.version() < o1.version() ? -1 : (o2.version() == o1.version() ? 0 : 1)); diff --git a/src/main/java/org/elasticsearch/monitor/jvm/HotThreads.java b/src/main/java/org/elasticsearch/monitor/jvm/HotThreads.java index e7a72b8f9ea..a65feb6ae89 100644 --- a/src/main/java/org/elasticsearch/monitor/jvm/HotThreads.java +++ b/src/main/java/org/elasticsearch/monitor/jvm/HotThreads.java @@ -19,6 +19,7 @@ package org.elasticsearch.monitor.jvm; +import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.ElasticSearchIllegalArgumentException; import org.elasticsearch.common.unit.TimeValue; @@ -130,7 +131,7 @@ public class HotThreads { // sort by delta CPU time on thread. List hotties = new ArrayList(threadInfos.values()); // skip that for now - Collections.sort(hotties, new Comparator() { + CollectionUtil.quickSort(hotties, new Comparator() { public int compare(MyThreadInfo o1, MyThreadInfo o2) { if ("cpu".equals(type)) { return (int) (o2.cpuTime - o1.cpuTime); diff --git a/src/main/java/org/elasticsearch/search/facet/datehistogram/InternalFullDateHistogramFacet.java b/src/main/java/org/elasticsearch/search/facet/datehistogram/InternalFullDateHistogramFacet.java index 7842b09ad1a..418f3c36d4d 100644 --- a/src/main/java/org/elasticsearch/search/facet/datehistogram/InternalFullDateHistogramFacet.java +++ b/src/main/java/org/elasticsearch/search/facet/datehistogram/InternalFullDateHistogramFacet.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.facet.datehistogram; +import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.common.CacheRecycler; import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesReference; @@ -163,7 +164,7 @@ public class InternalFullDateHistogramFacet extends InternalDateHistogramFacet { // we need to sort it InternalFullDateHistogramFacet internalFacet = (InternalFullDateHistogramFacet) facets.get(0); List entries = internalFacet.getEntries(); - Collections.sort(entries, comparatorType.comparator()); + CollectionUtil.timSort(entries, comparatorType.comparator()); internalFacet.releaseCache(); return internalFacet; } diff --git a/src/main/java/org/elasticsearch/search/facet/histogram/InternalFullHistogramFacet.java b/src/main/java/org/elasticsearch/search/facet/histogram/InternalFullHistogramFacet.java index 0e322e1ff9f..db95bfa4ad2 100644 --- a/src/main/java/org/elasticsearch/search/facet/histogram/InternalFullHistogramFacet.java +++ b/src/main/java/org/elasticsearch/search/facet/histogram/InternalFullHistogramFacet.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.facet.histogram; +import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.common.CacheRecycler; import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesReference; @@ -160,7 +161,7 @@ public class InternalFullHistogramFacet extends InternalHistogramFacet { // we need to sort it InternalFullHistogramFacet internalFacet = (InternalFullHistogramFacet) facets.get(0); List entries = internalFacet.getEntries(); - Collections.sort(entries, comparatorType.comparator()); + CollectionUtil.timSort(entries, comparatorType.comparator()); internalFacet.releaseCache(); return internalFacet; } diff --git a/src/main/java/org/elasticsearch/search/facet/termsstats/doubles/InternalTermsStatsDoubleFacet.java b/src/main/java/org/elasticsearch/search/facet/termsstats/doubles/InternalTermsStatsDoubleFacet.java index 1c6e9c735cc..6ea8c2692c1 100644 --- a/src/main/java/org/elasticsearch/search/facet/termsstats/doubles/InternalTermsStatsDoubleFacet.java +++ b/src/main/java/org/elasticsearch/search/facet/termsstats/doubles/InternalTermsStatsDoubleFacet.java @@ -19,6 +19,8 @@ package org.elasticsearch.search.facet.termsstats.doubles; +import org.apache.lucene.util.CollectionUtil; + import com.google.common.collect.ImmutableList; import org.elasticsearch.common.CacheRecycler; import org.elasticsearch.common.Strings; @@ -175,7 +177,7 @@ public class InternalTermsStatsDoubleFacet extends InternalTermsStatsFacet { InternalTermsStatsDoubleFacet tsFacet = (InternalTermsStatsDoubleFacet) facets.get(0); if (!tsFacet.entries.isEmpty()) { List entries = tsFacet.mutableList(); - Collections.sort(entries, comparatorType.comparator()); + CollectionUtil.timSort(entries, comparatorType.comparator()); } } return facets.get(0); diff --git a/src/main/java/org/elasticsearch/search/facet/termsstats/longs/InternalTermsStatsLongFacet.java b/src/main/java/org/elasticsearch/search/facet/termsstats/longs/InternalTermsStatsLongFacet.java index d4f6ca53468..7ccc63da9c3 100644 --- a/src/main/java/org/elasticsearch/search/facet/termsstats/longs/InternalTermsStatsLongFacet.java +++ b/src/main/java/org/elasticsearch/search/facet/termsstats/longs/InternalTermsStatsLongFacet.java @@ -20,6 +20,7 @@ package org.elasticsearch.search.facet.termsstats.longs; import com.google.common.collect.ImmutableList; +import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.common.CacheRecycler; import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesReference; @@ -175,7 +176,7 @@ public class InternalTermsStatsLongFacet extends InternalTermsStatsFacet { InternalTermsStatsLongFacet tsFacet = (InternalTermsStatsLongFacet) facets.get(0); if (!tsFacet.entries.isEmpty()) { List entries = tsFacet.mutableList(); - Collections.sort(entries, comparatorType.comparator()); + CollectionUtil.timSort(entries, comparatorType.comparator()); } } return facets.get(0); diff --git a/src/main/java/org/elasticsearch/search/facet/termsstats/strings/InternalTermsStatsStringFacet.java b/src/main/java/org/elasticsearch/search/facet/termsstats/strings/InternalTermsStatsStringFacet.java index 7682685e9b0..65d66f418b7 100644 --- a/src/main/java/org/elasticsearch/search/facet/termsstats/strings/InternalTermsStatsStringFacet.java +++ b/src/main/java/org/elasticsearch/search/facet/termsstats/strings/InternalTermsStatsStringFacet.java @@ -20,6 +20,7 @@ package org.elasticsearch.search.facet.termsstats.strings; import com.google.common.collect.ImmutableList; +import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.common.CacheRecycler; import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesArray; @@ -180,7 +181,7 @@ public class InternalTermsStatsStringFacet extends InternalTermsStatsFacet { InternalTermsStatsStringFacet tsFacet = (InternalTermsStatsStringFacet) facets.get(0); if (!tsFacet.entries.isEmpty()) { List entries = tsFacet.mutableList(); - Collections.sort(entries, comparatorType.comparator()); + CollectionUtil.timSort(entries, comparatorType.comparator()); } } return facets.get(0); diff --git a/src/main/java/org/elasticsearch/search/highlight/PlainHighlighter.java b/src/main/java/org/elasticsearch/search/highlight/PlainHighlighter.java index b7c2e4b445a..6d7ebd5c6f3 100644 --- a/src/main/java/org/elasticsearch/search/highlight/PlainHighlighter.java +++ b/src/main/java/org/elasticsearch/search/highlight/PlainHighlighter.java @@ -19,14 +19,13 @@ package org.elasticsearch.search.highlight; import com.google.common.collect.ImmutableList; - import com.google.common.collect.ImmutableSet; import com.google.common.collect.Maps; import org.apache.lucene.analysis.Analyzer; import org.apache.lucene.analysis.TokenStream; import org.apache.lucene.search.Query; import org.apache.lucene.search.highlight.*; -import org.apache.lucene.search.highlight.Formatter; +import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.ElasticSearchIllegalArgumentException; import org.elasticsearch.common.io.FastStringReader; import org.elasticsearch.common.text.StringText; @@ -37,7 +36,10 @@ import org.elasticsearch.search.fetch.FetchSubPhase; import org.elasticsearch.search.internal.SearchContext; import org.elasticsearch.search.lookup.SearchLookup; -import java.util.*; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; +import java.util.Map; /** * @@ -135,7 +137,7 @@ public class PlainHighlighter implements Highlighter { throw new FetchPhaseExecutionException(context, "Failed to highlight field [" + highlighterContext.fieldName + "]", e); } if (field.scoreOrdered()) { - Collections.sort(fragsList, new Comparator() { + CollectionUtil.quickSort(fragsList, new Comparator() { public int compare(TextFragment o1, TextFragment o2) { return Math.round(o2.getScore() - o1.getScore()); } diff --git a/src/main/java/org/elasticsearch/search/highlight/vectorhighlight/FragmentBuilderHelper.java b/src/main/java/org/elasticsearch/search/highlight/vectorhighlight/FragmentBuilderHelper.java index aeb8ac72bae..a036c3abfdf 100644 --- a/src/main/java/org/elasticsearch/search/highlight/vectorhighlight/FragmentBuilderHelper.java +++ b/src/main/java/org/elasticsearch/search/highlight/vectorhighlight/FragmentBuilderHelper.java @@ -20,28 +20,20 @@ package org.elasticsearch.search.highlight.vectorhighlight; -import java.util.Collections; -import java.util.Comparator; -import java.util.List; - import org.apache.lucene.analysis.Analyzer; -import org.apache.lucene.analysis.miscellaneous.WordDelimiterFilter; import org.apache.lucene.document.Field; import org.apache.lucene.search.vectorhighlight.FastVectorHighlighter; import org.apache.lucene.search.vectorhighlight.FieldFragList.WeightedFragInfo; import org.apache.lucene.search.vectorhighlight.FieldFragList.WeightedFragInfo.SubInfo; import org.apache.lucene.search.vectorhighlight.FragmentsBuilder; +import org.apache.lucene.util.CollectionUtil; import org.apache.lucene.util.Version; -import org.elasticsearch.index.analysis.CustomAnalyzer; -import org.elasticsearch.index.analysis.EdgeNGramTokenFilterFactory; -import org.elasticsearch.index.analysis.EdgeNGramTokenizerFactory; -import org.elasticsearch.index.analysis.NGramTokenFilterFactory; -import org.elasticsearch.index.analysis.NGramTokenizerFactory; -import org.elasticsearch.index.analysis.NamedAnalyzer; -import org.elasticsearch.index.analysis.TokenFilterFactory; -import org.elasticsearch.index.analysis.WordDelimiterTokenFilterFactory; +import org.elasticsearch.index.analysis.*; import org.elasticsearch.index.mapper.FieldMapper; +import java.util.Comparator; +import java.util.List; + /** * Simple helper class for {@link FastVectorHighlighter} {@link FragmentsBuilder} implemenations. */ @@ -65,7 +57,7 @@ public final class FragmentBuilderHelper { * the FastVectorHighlighter. Yet, this is really a lucene problem and should be fixed in lucene rather * than in this hack... aka. "we are are working on in!" */ final List subInfos = fragInfo.getSubInfos(); - Collections.sort(subInfos, new Comparator() { + CollectionUtil.quickSort(subInfos, new Comparator() { @Override public int compare(SubInfo o1, SubInfo o2) { int startOffset = o1.getTermsOffsets().get(0).getStartOffset(); diff --git a/src/main/java/org/elasticsearch/search/suggest/Suggest.java b/src/main/java/org/elasticsearch/search/suggest/Suggest.java index 9efd09bd007..a244a872697 100644 --- a/src/main/java/org/elasticsearch/search/suggest/Suggest.java +++ b/src/main/java/org/elasticsearch/search/suggest/Suggest.java @@ -18,15 +18,7 @@ */ package org.elasticsearch.search.suggest; -import java.io.IOException; -import java.util.ArrayList; -import java.util.Collections; -import java.util.Comparator; -import java.util.HashMap; -import java.util.Iterator; -import java.util.List; -import java.util.Map; - +import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.ElasticSearchException; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -39,6 +31,9 @@ import org.elasticsearch.search.suggest.Suggest.Suggestion.Entry; import org.elasticsearch.search.suggest.Suggest.Suggestion.Entry.Option; import org.elasticsearch.search.suggest.term.TermSuggestion; +import java.io.IOException; +import java.util.*; + /** * Top level suggest result, containing the result for each suggestion. */ @@ -354,7 +349,7 @@ public class Suggest implements Iterable comparator) { - Collections.sort(options, comparator); + CollectionUtil.timSort(options, comparator); } protected Entry reduce(List> toReduce) {