jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/376721 )
Change subject: Adding the fb-contrib extension to spotbugs
......................................................................
Adding the fb-contrib extension to spotbugs
While this has quite a few false-positive, it seems to have a few rules which
make sense. Let me know if you think this is useful or should be dropped.
Change-Id: I41093398b1894083f1ca7237f14ecbf192241eda
---
M pom.xml
A src/dev-tools/spotbugs-excludes.xml
M src/main/java/org/wikimedia/search/extra/fuzzylike/FuzzyLikeThisQuery.java
M src/main/java/org/wikimedia/search/extra/latency/LatencyStatsAction.java
M src/main/java/org/wikimedia/search/extra/latency/SearchLatencyListener.java
M
src/main/java/org/wikimedia/search/extra/levenshtein/LevenshteinDistanceScore.java
M src/main/java/org/wikimedia/search/extra/lombok.config
M src/main/java/org/wikimedia/search/extra/regex/SourceRegexQueryBuilder.java
M
src/main/java/org/wikimedia/search/extra/regex/UnacceleratedSourceRegexQuery.java
M
src/main/java/org/wikimedia/search/extra/regex/expression/AbstractCompositeExpression.java
M src/main/java/org/wikimedia/search/extra/regex/ngram/NGramAutomaton.java
M
src/main/java/org/wikimedia/search/extra/router/AbstractRouterQueryBuilder.java
M
src/main/java/org/wikimedia/search/extra/router/DegradedRouterQueryBuilder.java
13 files changed, 111 insertions(+), 34 deletions(-)
Approvals:
jenkins-bot: Verified
DCausse: Looks good to me, approved
diff --git a/pom.xml b/pom.xml
index b1e55d3..64c3bb0 100644
--- a/pom.xml
+++ b/pom.xml
@@ -78,6 +78,14 @@
<configuration>
<effort>high</effort>
<threshold>low</threshold>
+
<excludeFilterFile>${project.basedir}/src/dev-tools/spotbugs-excludes.xml</excludeFilterFile>
+ <plugins>
+ <plugin>
+ <groupId>com.mebigfatguy.fb-contrib</groupId>
+ <artifactId>fb-contrib</artifactId>
+ <version>7.0.4.sb</version>
+ </plugin>
+ </plugins>
</configuration>
<executions>
<execution>
diff --git a/src/dev-tools/spotbugs-excludes.xml
b/src/dev-tools/spotbugs-excludes.xml
new file mode 100644
index 0000000..3039b90
--- /dev/null
+++ b/src/dev-tools/spotbugs-excludes.xml
@@ -0,0 +1,35 @@
+<?xml version="1.0"?>
+<FindBugsFilter>
+ <!-- the following bugs are always ignored -->
+ <Match>
+ <!--
+ Manually sizing collections is error prone and in most cases tend
+ to be a micro optimization.
+ -->
+ <Bug pattern="PSC_PRESIZE_COLLECTIONS"/>
+ </Match>
+
+ <Match>
+ <!-- Softening exception is an acceptable pattern in this project. -->
+ <Or>
+ <Bug pattern="EXS_EXCEPTION_SOFTENING_HAS_CHECKED"/>
+ <Bug pattern="EXS_EXCEPTION_SOFTENING_NO_CONSTRAINTS"/>
+ </Or>
+ </Match>
+
+ <Match>
+ <!--
+ Inverting comparison order when String literals are involved leads
+ to unnatural code. Proper null checking should always be done
+ anyway.
+ -->
+ <Bug pattern="LSC_LITERAL_STRING_COMPARISON"/>
+ </Match>
+
+ <!-- specific exceptions -->
+ <Match
classregex="org\.wikimedia\.search\.extra\.regex\.SourceRegexQueryBuilder.*">
+ <!-- In this case, an array is accessed with known locations in array
-->
+ <Bug pattern="CLI_CONSTANT_LIST_INDEX"/>
+ </Match>
+
+</FindBugsFilter>
\ No newline at end of file
diff --git
a/src/main/java/org/wikimedia/search/extra/fuzzylike/FuzzyLikeThisQuery.java
b/src/main/java/org/wikimedia/search/extra/fuzzylike/FuzzyLikeThisQuery.java
index 2ed2746..e876512 100644
--- a/src/main/java/org/wikimedia/search/extra/fuzzylike/FuzzyLikeThisQuery.java
+++ b/src/main/java/org/wikimedia/search/extra/fuzzylike/FuzzyLikeThisQuery.java
@@ -17,6 +17,7 @@
* limitations under the License.
*/
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import lombok.EqualsAndHashCode;
import org.apache.lucene.analysis.Analyzer;
import org.apache.lucene.analysis.TokenStream;
@@ -186,7 +187,10 @@
}
}
- @Override
+ @Override
+ @SuppressFBWarnings(
+ value = "PCAIL_POSSIBLE_CONSTANT_ALLOCATION_IN_LOOP",
+ justification = "builder should not be reused")
public Query rewrite(IndexReader reader) throws IOException
{
//load up the list of possible terms
diff --git
a/src/main/java/org/wikimedia/search/extra/latency/LatencyStatsAction.java
b/src/main/java/org/wikimedia/search/extra/latency/LatencyStatsAction.java
index 2b67355..1232f9e 100644
--- a/src/main/java/org/wikimedia/search/extra/latency/LatencyStatsAction.java
+++ b/src/main/java/org/wikimedia/search/extra/latency/LatencyStatsAction.java
@@ -2,6 +2,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Sets;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import lombok.AccessLevel;
import lombok.Getter;
import org.elasticsearch.action.Action;
@@ -127,6 +128,9 @@
super(node);
empty();
}
+ @SuppressFBWarnings(
+ value = "PCOA_PARTIALLY_CONSTRUCTED_OBJECT_ACCESS",
+ justification = "readFrom has a well understood contract")
LatencyStatsNodeResponse(StreamInput in) throws IOException {
readFrom(in);
}
@@ -143,7 +147,7 @@
statDetails.readFrom(in);
}
- void empty() {
+ final void empty() {
statDetails = new StatDetails();
}
@@ -192,6 +196,9 @@
.collect(toList());
}
+ @SuppressFBWarnings(
+ value = "PCOA_PARTIALLY_CONSTRUCTED_OBJECT_ACCESS",
+ justification = "readFrom has a well understood contract")
StatDetails(StreamInput in) throws IOException {
readFrom(in);
}
@@ -206,6 +213,7 @@
}
@Override
+ @SuppressFBWarnings("PRMC_POSSIBLY_REDUNDANT_METHOD_CALLS")
public XContentBuilder toXContent(XContentBuilder builder, Params
params) throws IOException {
Map<String, List<LatencyStat>> byBucket = latencies.stream()
.collect(groupingBy(LatencyStat::getBucket));
diff --git
a/src/main/java/org/wikimedia/search/extra/latency/SearchLatencyListener.java
b/src/main/java/org/wikimedia/search/extra/latency/SearchLatencyListener.java
index 0e9792e..0e2bedd 100644
---
a/src/main/java/org/wikimedia/search/extra/latency/SearchLatencyListener.java
+++
b/src/main/java/org/wikimedia/search/extra/latency/SearchLatencyListener.java
@@ -1,6 +1,7 @@
package org.wikimedia.search.extra.latency;
import com.google.common.annotations.VisibleForTesting;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import org.HdrHistogram.Histogram;
import org.HdrHistogram.Recorder;
import org.elasticsearch.common.component.AbstractLifecycleComponent;
@@ -45,7 +46,7 @@
@Override
protected void doStart() {
- if (cancelRotation != null) {
+ if (cancelRotation == null) {
cancelRotation =
threadPoolSupplier.get().scheduleWithFixedDelay(this::rotate, ROTATION_DELAY,
ThreadPool.Names.GENERIC);
}
}
@@ -84,6 +85,7 @@
.collect(toList());
}
+ @SuppressFBWarnings("PRMC_POSSIBLY_REDUNDANT_METHOD_CALLS")
public void onQueryPhase(SearchContext searchContext, long tookInNanos) {
if (searchContext.groupStats() == null) {
return;
diff --git
a/src/main/java/org/wikimedia/search/extra/levenshtein/LevenshteinDistanceScore.java
b/src/main/java/org/wikimedia/search/extra/levenshtein/LevenshteinDistanceScore.java
index 6bfd9a1..e6af566 100644
---
a/src/main/java/org/wikimedia/search/extra/levenshtein/LevenshteinDistanceScore.java
+++
b/src/main/java/org/wikimedia/search/extra/levenshtein/LevenshteinDistanceScore.java
@@ -81,7 +81,7 @@
}
@Override
- public Explanation explainScore(int docId, Explanation
subQueryScore) throws IOException {
+ public Explanation explainScore(int docId, Explanation
subQueryScore) {
double score = score(docId, subQueryScore.getValue());
String explanation = "LevenshteinDistanceScore";
explanation += " with parameters:\n text:" + value;
diff --git a/src/main/java/org/wikimedia/search/extra/lombok.config
b/src/main/java/org/wikimedia/search/extra/lombok.config
index e572d96..7cc30ff 100644
--- a/src/main/java/org/wikimedia/search/extra/lombok.config
+++ b/src/main/java/org/wikimedia/search/extra/lombok.config
@@ -1 +1,2 @@
-lombok.equalsAndHashCode.callSuper = call
\ No newline at end of file
+lombok.equalsAndHashCode.callSuper = call
+lombok.extern.findbugs.addSuppressFBWarnings = true
diff --git
a/src/main/java/org/wikimedia/search/extra/regex/SourceRegexQueryBuilder.java
b/src/main/java/org/wikimedia/search/extra/regex/SourceRegexQueryBuilder.java
index 50023d5..2f8724a 100644
---
a/src/main/java/org/wikimedia/search/extra/regex/SourceRegexQueryBuilder.java
+++
b/src/main/java/org/wikimedia/search/extra/regex/SourceRegexQueryBuilder.java
@@ -1,5 +1,6 @@
package org.wikimedia.search.extra.regex;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import lombok.AccessLevel;
import lombok.EqualsAndHashCode;
import lombok.Getter;
@@ -36,6 +37,7 @@
@Accessors(chain = true, fluent = true)
@Getter
@Setter
+@SuppressFBWarnings("CLI_CONSTANT_LIST_INDEX")
public class SourceRegexQueryBuilder extends
AbstractQueryBuilder<SourceRegexQueryBuilder> {
public static final ParseField NAME = new ParseField("source_regex",
"sourceRegex", "source-regex");
@@ -48,27 +50,29 @@
public static final boolean DEFAULT_LOAD_FROM_SOURCE = true;
public static final int DEFAULT_GRAM_SIZE = 3;
- private static final ConstructingObjectParser<SourceRegexQueryBuilder,
QueryParseContext> PARSER;
+ private static final ConstructingObjectParser<SourceRegexQueryBuilder,
QueryParseContext> PARSER = constructParser();
- static {
- PARSER = new ConstructingObjectParser<SourceRegexQueryBuilder,
QueryParseContext>(NAME.getPreferredName(),
+ private static ConstructingObjectParser<SourceRegexQueryBuilder,
QueryParseContext> constructParser() {
+ ConstructingObjectParser<SourceRegexQueryBuilder, QueryParseContext>
parser =
+ new ConstructingObjectParser<>(NAME.getPreferredName(),
(o) -> new SourceRegexQueryBuilder((String) o[0], (String)
o[1]));
- PARSER.declareString(constructorArg(), FIELD);
- PARSER.declareString(constructorArg(), REGEX);
- PARSER.declareBoolean(SourceRegexQueryBuilder::loadFromSource,
LOAD_FROM_SOURCE);
- PARSER.declareString(SourceRegexQueryBuilder::ngramField, NGRAM_FIELD);
- PARSER.declareInt(SourceRegexQueryBuilder::gramSize, GRAM_SIZE);
- PARSER.declareInt((x,i) -> x.settings().maxExpand(i),
Settings.MAX_EXPAND);
- PARSER.declareInt((x,i) -> x.settings().maxStatesTraced(i),
Settings.MAX_STATES_TRACED);
- PARSER.declareInt((x,i) -> x.settings().maxDeterminizedStates(i),
Settings.MAX_DETERMINIZED_STATES);
- PARSER.declareInt((x,i) -> x.settings().maxNgramsExtracted(i),
Settings.MAX_NGRAMS_EXTRACTED);
- PARSER.declareInt((x,i) -> x.settings().maxInspect(i),
Settings.MAX_INSPECT);
- PARSER.declareBoolean((x,b) -> x.settings().caseSensitive(b),
Settings.CASE_SENSITIVE);
- PARSER.declareString((x,s) ->
x.settings().locale(LocaleUtils.parse(s)), Settings.LOCALE);
- PARSER.declareBoolean((x,b) -> x.settings().rejectUnaccelerated(b),
Settings.REJECT_UNACCELERATED);
- PARSER.declareInt((x,i) -> x.settings().maxNgramClauses(i),
Settings.MAX_NGRAM_CLAUSES);
- PARSER.declareString((x,s) -> x.settings().timeout(s),
Settings.TIMEOUT);
- declareStandardFields(PARSER);
+ parser.declareString(constructorArg(), FIELD);
+ parser.declareString(constructorArg(), REGEX);
+ parser.declareBoolean(SourceRegexQueryBuilder::loadFromSource,
LOAD_FROM_SOURCE);
+ parser.declareString(SourceRegexQueryBuilder::ngramField, NGRAM_FIELD);
+ parser.declareInt(SourceRegexQueryBuilder::gramSize, GRAM_SIZE);
+ parser.declareInt((x,i) -> x.settings().maxExpand(i),
Settings.MAX_EXPAND);
+ parser.declareInt((x,i) -> x.settings().maxStatesTraced(i),
Settings.MAX_STATES_TRACED);
+ parser.declareInt((x,i) -> x.settings().maxDeterminizedStates(i),
Settings.MAX_DETERMINIZED_STATES);
+ parser.declareInt((x,i) -> x.settings().maxNgramsExtracted(i),
Settings.MAX_NGRAMS_EXTRACTED);
+ parser.declareInt((x,i) -> x.settings().maxInspect(i),
Settings.MAX_INSPECT);
+ parser.declareBoolean((x,b) -> x.settings().caseSensitive(b),
Settings.CASE_SENSITIVE);
+ parser.declareString((x,s) ->
x.settings().locale(LocaleUtils.parse(s)), Settings.LOCALE);
+ parser.declareBoolean((x,b) -> x.settings().rejectUnaccelerated(b),
Settings.REJECT_UNACCELERATED);
+ parser.declareInt((x,i) -> x.settings().maxNgramClauses(i),
Settings.MAX_NGRAM_CLAUSES);
+ parser.declareString((x,s) -> x.settings().timeout(s),
Settings.TIMEOUT);
+ declareStandardFields(parser);
+ return parser;
}
private final String field;
diff --git
a/src/main/java/org/wikimedia/search/extra/regex/UnacceleratedSourceRegexQuery.java
b/src/main/java/org/wikimedia/search/extra/regex/UnacceleratedSourceRegexQuery.java
index 3fe3052..976390e 100644
---
a/src/main/java/org/wikimedia/search/extra/regex/UnacceleratedSourceRegexQuery.java
+++
b/src/main/java/org/wikimedia/search/extra/regex/UnacceleratedSourceRegexQuery.java
@@ -152,7 +152,7 @@
private static final Collector NULL_COLLECTOR = new SimpleCollector() {
@Override
- public void collect(int doc) throws IOException {}
+ public void collect(int doc) {}
@Override
public boolean needsScores() {
return true;
diff --git
a/src/main/java/org/wikimedia/search/extra/regex/expression/AbstractCompositeExpression.java
b/src/main/java/org/wikimedia/search/extra/regex/expression/AbstractCompositeExpression.java
index 6d4dfc7..bd6ed0e 100644
---
a/src/main/java/org/wikimedia/search/extra/regex/expression/AbstractCompositeExpression.java
+++
b/src/main/java/org/wikimedia/search/extra/regex/expression/AbstractCompositeExpression.java
@@ -2,6 +2,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import lombok.EqualsAndHashCode;
import java.util.ArrayList;
@@ -244,6 +245,10 @@
}
@Override
+ @SuppressFBWarnings(
+ value = "STT_TOSTRING_STORED_IN_FIELD",
+ justification = "Here we do want to cache the value of " +
+ "toString() as its computation can be expensive.")
public String toString() {
if (toString != null) {
return toString;
@@ -261,7 +266,7 @@
} else {
b.append(toStringJoiner());
}
- b.append(component.toString());
+ b.append(component);
}
b.append(')');
if (b.length() > MAX_COMPONENT_STRING_LENGTH) {
diff --git
a/src/main/java/org/wikimedia/search/extra/regex/ngram/NGramAutomaton.java
b/src/main/java/org/wikimedia/search/extra/regex/ngram/NGramAutomaton.java
index ce9967c..de73ff0 100644
--- a/src/main/java/org/wikimedia/search/extra/regex/ngram/NGramAutomaton.java
+++ b/src/main/java/org/wikimedia/search/extra/regex/ngram/NGramAutomaton.java
@@ -1,6 +1,7 @@
package org.wikimedia.search.extra.regex.ngram;
import com.google.common.collect.ImmutableSet;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import lombok.EqualsAndHashCode;
import org.apache.lucene.analysis.Analyzer;
import org.apache.lucene.analysis.TokenStream;
@@ -27,6 +28,9 @@
* A finite automaton who's transitions are ngrams that must be in the string
or
* ngrams we can't check for. Not thread safe one bit.
*/
+@SuppressFBWarnings(value = "DLC_DUBIOUS_LIST_COLLECTION", justification =
"Need more time to investigate")
+// TODO: It might be possible to convert acceptStates to a Set, which would be
+// more efficient and better represent the intent if this is actually the case.
public class NGramAutomaton {
private final Automaton source;
private final int gramSize;
@@ -82,10 +86,10 @@
b.append("
[shape=circle,label=\"").append(state).append("\"];\n");
}
if (state.initial) {
- b.append(" initial -> ").append(state.dotName()).append("\n");
+ b.append(" initial -> ").append(state.dotName()).append('\n');
}
for (NGramTransition transition : state.outgoingTransitions) {
- b.append(" ").append(transition).append("\n");
+ b.append(" ").append(transition).append('\n');
}
}
return b.append("}\n").toString();
@@ -222,7 +226,7 @@
throw new IllegalArgumentException("Analyzer provided
generate more than one tokens, " +
"if using 3grams make sure to use a 3grams
analyzer, " +
"for input [" + ngram + "] first is [" + ngram +
"] " +
- "but [" + cattr.toString() + "] was generated.");
+ "but [" + cattr + "] was generated.");
}
}
} catch (IOException ioe) {
@@ -356,7 +360,7 @@
StringBuilder b = new StringBuilder();
b.append(from.dotName()).append(" -> ").append(to.dotName());
if (ngram != null) {
- b.append(" [label=\"").append(ngram.replace(" ",
"_")).append("\"]");
+ b.append(" [label=\"").append(ngram.replace(' ',
'_')).append("\"]");
}
return b.toString();
}
diff --git
a/src/main/java/org/wikimedia/search/extra/router/AbstractRouterQueryBuilder.java
b/src/main/java/org/wikimedia/search/extra/router/AbstractRouterQueryBuilder.java
index 180616f..5a151b8 100644
---
a/src/main/java/org/wikimedia/search/extra/router/AbstractRouterQueryBuilder.java
+++
b/src/main/java/org/wikimedia/search/extra/router/AbstractRouterQueryBuilder.java
@@ -1,6 +1,7 @@
package org.wikimedia.search.extra.router;
import com.google.common.annotations.VisibleForTesting;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import lombok.AccessLevel;
import lombok.EqualsAndHashCode;
import lombok.Getter;
@@ -90,14 +91,17 @@
}
@Override
+ @SuppressFBWarnings("ACEM_ABSTRACT_CLASS_EMPTY_METHODS")
protected Query doToQuery(QueryShardContext queryShardContext) throws
IOException {
throw new UnsupportedOperationException("This query must be
rewritten.");
}
+ @SuppressFBWarnings("ACEM_ABSTRACT_CLASS_EMPTY_METHODS")
protected void addXContent(XContentBuilder builder, Params params) throws
IOException {
}
@Override
+ @SuppressFBWarnings("PRMC_POSSIBLY_REDUNDANT_METHOD_CALLS")
protected void doXContent(XContentBuilder builder, Params params) throws
IOException {
builder.startObject(getWriteableName());
if (fallback() != null) {
@@ -116,6 +120,7 @@
builder.endObject();
}
+ @SuppressFBWarnings(value = "OCP_OVERLY_CONCRETE_PARAMETER", justification
= "No need to be generic in this case")
static <C extends Condition, CPS extends AbstractConditionParserState<C>>
C parseCondition(
ObjectParser<CPS, QueryParseContext> condParser, XContentParser
parser, QueryParseContext parseContext
) throws IOException {
@@ -125,6 +130,7 @@
}
+ @SuppressFBWarnings(value = "LEST_LOST_EXCEPTION_STACK_TRACE",
justification = "The new exception contains all needed context")
static <QB extends AbstractRouterQueryBuilder<?, QB>> Optional<QB>
fromXContent(
ObjectParser<QB, QueryParseContext> objectParser,
QueryParseContext parseContext) throws IOException {
XContentParser parser = parseContext.parser();
@@ -234,7 +240,7 @@
abstract C condition();
- void checkValid() throws IllegalArgumentException {
+ void checkValid() {
if (query == null) {
throw new IllegalArgumentException("Missing field [query] in
condition");
}
diff --git
a/src/main/java/org/wikimedia/search/extra/router/DegradedRouterQueryBuilder.java
b/src/main/java/org/wikimedia/search/extra/router/DegradedRouterQueryBuilder.java
index 88a98b9..86921ad 100644
---
a/src/main/java/org/wikimedia/search/extra/router/DegradedRouterQueryBuilder.java
+++
b/src/main/java/org/wikimedia/search/extra/router/DegradedRouterQueryBuilder.java
@@ -142,7 +142,7 @@
load((bucket, percentile, stats) -> stats.get1MinuteLoadAverage()),
latency((bucket, percentile, stats) -> stats.getLatency(bucket,
percentile)) {
@Override
- public void checkValid(String bucket, Double percentile) throws
IllegalArgumentException {
+ public void checkValid(String bucket, Double percentile) {
if (bucket == null) {
throw new IllegalArgumentException("Missing field [bucket]
in condition");
}
@@ -175,7 +175,7 @@
return values()[ord];
}
- void checkValid(String bucket, Double percentile) throws
IllegalArgumentException {
+ void checkValid(String bucket, Double percentile) {
if (bucket != null) {
throw new IllegalArgumentException("Extra field [bucket] in
condition");
}
@@ -197,7 +197,7 @@
}
@Override
- void checkValid() throws IllegalArgumentException {
+ void checkValid() {
super.checkValid();
type.checkValid(bucket, percentile);
}
--
To view, visit https://gerrit.wikimedia.org/r/376721
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I41093398b1894083f1ca7237f14ecbf192241eda
Gerrit-PatchSet: 5
Gerrit-Project: search/extra
Gerrit-Branch: master
Gerrit-Owner: Gehel <guillaume.leder...@wikimedia.org>
Gerrit-Reviewer: DCausse <dcau...@wikimedia.org>
Gerrit-Reviewer: EBernhardson <ebernhard...@wikimedia.org>
Gerrit-Reviewer: Gehel <guillaume.leder...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
