# Search query for bugs in Apache Solr

Once again, we're checking the Apache product\. This time we chose Solr, an open\-source search server platform\. Solr enables you quickly and efficiently search for information in databases and online resources\. When faced with such a complex task, it's easy to make mistakes, even for experienced Apache developers\. In this article, we'll look at these types of mistakes\. 

![1147_Solr/image1.png](https://import.viva64.com/docx/blog/1147_Solr/image1.png)

## Who are you, Solr?

Not long ago, we [checked](https://pvs-studio.com/en/blog/posts/java/1117/) one of the most famous Apache projects, the NetBeans IDE\. We found many interesting warnings issued by our analyzer during the check\. By the way, the developers quickly noticed them and made a pull request before I did :\) This time around, we've decided to take a look at another one of their big products, the [Solr](https://github.com/apache/solr/) full\-text search platform\. 

First introduced in 2006, Apache Solr offers a wide range of features, from dynamic clustering and database integration to processing complex formatted documents\. This search engine enables you to search and analyze information on a website with great speed, and also offers the capability to host search servers on hardware running on Linux\. Solr has\.\.\.

Long story short, I don't want to bother you with too many details\. It's enough to know that it's a handy software platform that optimizes big data\. Why don't we look at its source code and search it for something interesting or unusual? That's exactly what we're going to do now\. 

## They mixed something up here

A programmer is never safe from typos that involve mixed\-up operators\. Here's one of them:

```cpp
public Map<String, List<String>> getIndexFilesPathForSnapshot(
    String collectionName, String snapshotName, String pathPrefix)
    throws SolrServerException, IOException {
  ....

  if (meta != null) {                  // <=
    throw new IllegalArgumentException(
                  "The snapshot named " + snapshotName +
                  " is not found for collection " + collectionName);
  }

  DocCollection collectionState = solrClient.getClusterState() 
                                            .getCollection(collectionName);
  for (Slice s : collectionState.getSlices()) {
    List<CoreSnapshotMetaData> replicaSnaps = 
                     meta.getReplicaSnapshotsForShard(s.getName());  // <=
    ....
  }
  return result;
}
```

This method uses the _meta_ variable to store information about the system snapshots\. There's also a check that _meta_ \!\= _null_ in the above code fragment\. If so, then _IllegalArgumentException_ is thrown\. That alone seems bizarre\. Okay, now let's look at the loop body\. This is where _getReplicaSnapshotsForShard_ is called\. Given that _meta_ here is always null, we get _NullPointerException_\. It seems that the developer just made a typo and mixed up operators\. So, an exception should be thrown if _meta_ equals _null_\.

The PVS\-Studio analyzer worked as a proofreader and reported a detected typo: 

[V6008](https://pvs-studio.com/en/docs/warnings/v6008/) Null dereference of 'meta'\. SolrSnapshotsTool\.java 262

I'd like to add that such errors with mixed up operators are more common than you might think\. For example, I've already seen at least two similar ones in the [NetBeans project](https://pvs-studio.com/en/blog/posts/java/1117/):

```cpp
private SourcesModel getModel() {
  SourcesModel tm = model.get();
  if (tm == null) {
    tm.sourcePath.removePropertyChangeListener (this);
    tm.debugger.getSmartSteppingFilter ().
    removePropertyChangeListener (this);
  }
  return tm;
}
```

[V6008](https://pvs-studio.com/en/docs/warnings/v6008/) Null dereference of 'tm'\. SourcesModel\.java 713

```cpp
public void propertyChange(PropertyChangeEvent evt) {
  ....
  synchronized (this) {
    artifacts = null;
    if (listeners == null && listeners.isEmpty()) {
      return;
    }
    ....
  }
}
```

[V6008](https://pvs-studio.com/en/docs/warnings/v6008/) Null dereference of 'listeners'\. MavenArtifactsImplementation\.java 613

We may have found a new typo pattern\. We'll keep on watching\. 

Let's move on to the next fragment\. In this class, a developer mixed up what to return in one of the _get_ methods\.

```cpp
public class FunctionQParser extends QParser {
  ....
  boolean parseMultipleSources = true;
  boolean parseToEnd = true;
  ....
  public void setParseMultipleSources(boolean parseMultipleSources) {
    this.parseMultipleSources = parseMultipleSources;
  }
 
  /** parse multiple comma separated value sources */
  public boolean getParseMultipleSources() {
    return parseMultipleSources;
  }
  
  public void setParseToEnd(boolean parseToEnd) {
    this.parseToEnd = parseToEnd;
  }
 
  /** throw exception if there is extra 
      stuff at the end of the parsed valuesource(s). */
  public boolean getParseToEnd() {
    return parseMultipleSources;
  }
  ....
}
```

The class is designed for parsing a certain written mathematical function\. There are two properties to change the behavior of the parser: _parseMultipleSources_ analyzes all sources of numeric values, and _parseToEnd_ checks if the function with a string should be parsed to the end\. 

Now let's look at the _get_ and _set_ methods for these fields\. The _parseMultipleSources_ field is returned in _getParseToEnd_\. The programmer mixed up what field should be returned here\. 

The analyzer easily detects mismatched returned fields:

[V6091](https://pvs-studio.com/en/docs/warnings/v6091/) Suspicious getter implementation\. The 'parseToEnd' field should probably be returned instead\. FunctionQParser\.java 87, FunctionQParser\.java 57

The typo in the following code fragment can result in _NullPointerException_\.

```cpp
public void stringField(FieldInfo fieldInfo, String value) throws IOException {
  // trim the value if needed
  int len = value != null ? 
             UnicodeUtil.calcUTF16toUTF8Length(value, 0, value.length()) : 0;
  if (value.length() > maxLength) {               // <=    
    value = value.substring(0, maxLength);
  }
  countItem(fieldInfo.name, value, len);
}
```

Let's take a closer look: _value_ is compared to _null_ first, and then, in the next line, the _length\(\)_ method is called on _value_\. But the variable can be _null_\! Most likely, the developer should've used the _len_ variable instead of calling _length\(\)_\.

We found this typo thanks to the analyzer message:

[V6008](https://pvs-studio.com/en/docs/warnings/v6008/) Potential null dereference of 'value'\. IndexSizeEstimator\.java 735, IndexSizeEstimator\.java 736

Let's look at another code fragment with a typo:

```cpp
public Object doWork(Object value) throws IOException {
  ....
  List<?> list = (List<?>) value;
  // Validate all of same type and are comparable
  Object checkingObject = list.get(0);
  for (int idx = 0; idx < list.size(); ++idx) {
    Object item = list.get(0);                         // <=
    
    if (null == item) {
      throw new IOException(....);
    } else if (!(item instanceof Comparable<?>)) {
      throw new IOException(....);
    } else if (!item.getClass()
                    .getCanonicalName()
                    .equals(checkingObject.getClass()
                                          .getCanonicalName())) {
       throw new IOException(....);
    }
  }
  ....
}
```

It's worth paying attention to the _for_ loop here\. As usual, the programmer declares the loop and the _idx_ counter variable, then they get the number of items in the list\. However, there's an issue: each iteration of the loop takes only the element at the 0: _list\.get\(0\)_ index\.

The PVS\-Studio analyzer detected this error:

[V6016](https://pvs-studio.com/en/docs/warnings/v6016/) Suspicious access to element of 'list' object by a constant index inside a loop\. AscEvaluator\.java 56

The following example shows two methods with different names\. However, they do the same thing\. 

```cpp
private static List<Feature> makeFeatures(int[] featureIds) {
  final List<Feature> features = new ArrayList<>();
  for (final int i : featureIds) {
    Map<String, Object> params = new HashMap<String, Object>();
    params.put("value", i);
    final Feature f = Feature.getInstance(solrResourceLoader, 
                       ValueFeature.class.getName(), "f" + i, params);
    f.setIndex(i);
    features.add(f);
  }
  return features;
}

private static List<Feature> makeFilterFeatures(int[] featureIds) {
  final List<Feature> features = new ArrayList<>();
  for (final int i : featureIds) {
    Map<String, Object> params = new HashMap<String, Object>();
    params.put("value", i);
    final Feature f = Feature.getInstance(solrResourceLoader, 
                       ValueFeature.class.getName(), "f" + i, params);
    f.setIndex(i);
    features.add(f);
  }
  return features;
}
```

The first one creates a list of the _Feature_ class objects\. The second one, based on the name, should return a different type or filter these Features\. If the _FilterFeature_ type existed in the source code, we could assume that the developers simply made a typo\. However, there's no such type\. Maybe the method was copied and the developers forgot about it after copying it\.

Anyway, this snippet looks very suspicious\. And the analyzer proves this:

[V6032](https://pvs-studio.com/en/docs/warnings/v6032/) It is odd that the body of method 'makeFeatures' is fully equivalent to the body of another method 'makeFilterFeatures'\. TestLTRScoringQuery\.java 66, TestLTRScoringQuery\.java 79

## Forgot to check? Got error on track

If your gut tells you that _null_ checks are unnecessary, don't trust it\. In the code below, the "extra" check could've prevented _NullPointerException_\. 

```cpp
public static Map<String, Object> postProcessCollectionJSON(
                                            Map<String, Object> collection) {
  final Map<String, Map<String, Object>> shards = collection != null   // <=
         ? (Map<String, Map<String, Object>>)
           collection.getOrDefault("shards", Collections.emptyMap())
         : Collections.emptyMap();
  final List<Health> healthStates = new ArrayList<>(shards.size());
  shards.forEach(
  ....
  );
  collection.put("health", Health.combine(healthStates).toString());   // <=
  return collection;
}
```

In the beginning of the method, the programmer checks if the _collection_ reference is empty\. If that's the case, then _shards_ are derived from the _collection_\. The most interesting thing is that _healthStates_ is added to the _collection_ at the end, regardless of whether the _collection_ reference is empty or not\. 

Here's the analyzer warning for this code fragment: 

[V6008](https://pvs-studio.com/en/docs/warnings/v6008/) Potential null dereference of 'collection'\. ClusterStatus\.java 303, ClusterStatus\.java 335

And in the next example, the developers made an obvious mistake in the class constructor to support parallel distribution of thread work\. 

```cpp
public class ParallelStream extends CloudSolrStream 
                            implements Expressible {
  ....
  private transient StreamFactory streamFactory;

  public ParallelStream(String zkHost, 
                        String collection, 
                        String expressionString, 
                        int workers, 
                        StreamComparator comp
) throws IOException {
    TupleStream tStream = this.streamFactory
                              .constructStream(expressionString);  // <=
    init(zkHost, collection, tStream, workers, comp);
  }  
  ....
}
```

The error lies in the first line of the constructor body\. The _streamFactory_ field is accessed here, but the field isn't initialized\. The developers may have forgotten to add some logic in the constructor, or accidently may have written this line\.

The PVS\-Studio warning:

[V6090](https://pvs-studio.com/en/docs/warnings/v6090/) Field 'streamFactory' is being used before it was initialized\. ParallelStream\.java 61 

However, they didn't forget to add a check in this method\. Although, I think they put it in the wrong place\.

```cpp
private void createNewCollection(final String collection)
 throws InterruptedException {
  ....
  pending.add(completionService.submit(call));
  while (pending != null && pending.size() > 0) {
    Future<Object> future = completionService.take();
    if (future == null) return;
    pending.remove(future);
  }
}
```

Let's look at the interaction with the _pending_ field: first the programmer called _add_, then they decided to make a loop in which they gradually removed elements from the method\. The most interesting thing is that they checked that _pending_ isn't _null_ in the loop condition\. It looks very suspicious, considering that there's no variable zeroing in the loop body\. Seems like they should've added a check before calling the _add_ method as well\.

The analyzer warning:

[V6060](https://pvs-studio.com/en/docs/warnings/v6060/) The 'pending' reference was utilized before it was verified against null\. AbstractBasicDistributedZkTestBase\.java 1664, AbstractBasicDistributedZkTestBase\.java 1665 

## Lost exception

Like many modern languages, Java has exception handling feature\. The most important thing is to not lose them, as it happened here\. 

```cpp
private void doSplitShardWithRule(SolrIndexSplitter.SplitMethod splitMethod) 
 throws Exception {
  ....
  try {
    ZkStateReader.from(cloudClient)
                 .waitForState(collectionName, 30, 
                             TimeUnit.SECONDS,
                             SolrCloudTestCase.activeClusterShape(1, 2));
  } catch (TimeoutException e) {
    new RuntimeException("Timeout waiting for " +         // <=
                         "1shards and 2 replicas.", e);
  }
  ....
}
```

The error lies in the _catch_ block: the developers created the _RuntimeException_ object there, even added a link to the current intercepted _TimeoutException_ and a message\. But they forgot to write the _throw_ keyword\. So, the exception is never thrown\. 

The Lost and Found Bureau, in the form of our analyzer, found the lost exception and notified us about it: 

[V6006](https://pvs-studio.com/en/docs/warnings/v6006/) The object was created but it is not being used\. The 'throw' keyword could be missing\. ShardSplitTest\.java 773

## How arithmetic errors interfere with testing

Does testing make software less buggy? Well, humans are the ones who write tests, and they can't help but make mistakes\. This is what happened in the following example\.

```cpp
Public class SpellCheckCollatorTest extends SolrTestCaseJ4 {
  private static final int NUM_DOCS_WITH_TERM_EVERYOTHER = 8;
  private static final int NUM_DOCS = 17;
  ....
  @Test
  public void testEstimatedHitCounts() {
    ....
    for (int val = 5; val <= 20; val++) {
      String hitsXPath = xpathPrefix + "long[@name='hits']"; 

      if (val <= NUM_DOCS_WITH_TERM_EVERYOTHER) {
        int max = NUM_DOCS;
        int min = (/* min collected */ val) / 
                  (/* max docs possibly scanned */ NUM_DOCS);
        hitsXPath += "[" + min + " <= . and . <= " + max + "]";
      } 
    ....
    }
  }
  ....
}
```

A string containing the _min_ variable, which in turn is the result of dividing _val_ by _NUM\_DOCS_, is written to _hitsXPath_ here\. Looking closer, you can see that the maximum and minimum values of _val_ in this fragment are 8 and 5\. The _NUM\_DOCS_ value is always 17\. In all cases, _min_ is zero in integer division\. Most likely, the programmer forgot to convert division arguments to real numbers and change the type of the _min_ variable\.

We found this error using a brand\-new diagnostic rule in the PVS\-Studio analyzer:

[V6113](https://pvs-studio.com/en/docs/warnings/v6113/) The '\(val\) / \(NUM\_DOCS\)' expression evaluates to 0 because the absolute value of the left operand 'val' is less than the value of the right operand 'NUM\_DOCS'\. SpellCheckCollatorTest\.java 683

## Danger of checking objects by reference 

The class bellow describes the _equalsTo_ comparison method\. 

```cpp
private static class RandomQuery extends Query {
  private final long seed;
  private float density;
  private final List<BytesRef> docValues;
  ....
  private boolean equalsTo(RandomQuery other) {
    return seed == other.seed && 
           docValues == other.docValues && 
           density == other.density;
  }
}
```

Comparisons of the _seed_ and _density_ fields almost don't cause any questions \(except, perhaps, for the _density_ field that is a real number\), because the values directly written into them are considered\. However, since this field has a reference type, the _docValues_ comparison via '\=\=' looks very dubious\. This check considers only the address and not the internal state of the object\. 

With such defect you can miss the case when two different lists store the same values because the lists are the same, but the references are different\. It seems that when the developers named the _equalsTo_ method, they hardly meant that it should compare references rather than the internal state of objects\. 

The PVS\-Studio analyzer warning: 

[V6013](https://pvs-studio.com/en/docs/warnings/v6013/) Objects 'docValues' and 'other\.docValues' are compared by reference\. Possibly an equality comparison was intended\. TestFieldCacheSortRandom\.java 341

## Suspicious lack of synchronization 

You won't find an error in the next fragment, but it's still potentially there\. How's that possible? Take a look at this code and find out why\. 

```cpp
public abstract class CachingDirectoryFactory extends DirectoryFactory {
  ....
  private static final Logger log = LoggerFactory.getLogger(....);
  protected Map<String, CacheValue> byPathCache = new HashMap<>();
  protected IdentityHashMap<Directory, CacheValue> byDirectoryCache = 
                                                  new IdentityHashMap<>();
  ....

  private void removeFromCache(CacheValue v) {
    log.debug("Removing from cache: {}", v);
    byDirectoryCache.remove(v.directory);
    byPathCache.remove(v.path);
  }
}
```

We won't be able to understand what's wrong here until we look at all the uses of the _byDirectoryCache_ variable\. In all other methods, the interaction occurs in the _synchronized_ block\. However, in the _removeFromCache_ method, the programmer removes the collection elements outside of the _synchronized_ blocks\. 

The analyzer detected this suspicious fragment: 

[V6102](https://pvs-studio.com/en/docs/warnings/v6102/) Inconsistent synchronization of the 'byDirectoryCache' field\. Consider synchronizing the field on all usages\. CachingDirectoryFactory\.java 92, CachingDirectoryFactory\.java 228

At this point, one could say there's an error here, and the race condition could happen\. However, it turns out that all _removeFromCache_ calls are also enclosed in _synchronized_ blocks\. So, this is mostly a false positive that could be suppressed\.

Although, we still can enhance this code, because there's a potential issue here\. For example, when you need to use this method again, you may simply forget to enclose it in a synchronized block\. Even though other methods have additional checks that the object exists in the _byDirectoryCache_ collection, an unsynchronized call may delete an element already after the check\. As a result, unnecessary actions are performed in another thread with a non\-existent element of the collection, which can lead to errors in the program logic\. 

To protect ourselves from this, we can simply add the _synchronized_ keyword to the _removeFromCache_ method\. So, even though there's no real error here, the static analyzer still [urges](https://pvs-studio.com/en/blog/posts/cpp/1115/) us to write cleaner code\.

By the way, we just recently released an [article](https://pvs-studio.com/en/blog/posts/java/1128/) on the pitfalls of using synchronization\.

## Is it possible to create a few classes named the same? 

In this fragment, the programmer didn't consider that classes can be renamed or declared with the same name in different packages\.

```cpp
private static String getFieldFlags(IndexableField f) {
  IndexOptions opts = (f == null) ? null : f.fieldType().indexOptions();

  StringBuilder flags = new StringBuilder();
  ....
  flags.append((f != null && f.getClass()
                              .getSimpleName()
                              .equals("LazyField"))  // <=
                                   ? FieldFlag.LAZY.getAbbreviation(): '-');
  ....
  return flags.toString();
}
```

This is an obviously unnecessary operation that may result in an error\. The _f_ variable has the _getClass_ method called, which returns the object type, then gets and checks the name without specifying packages\. All in all, there's no error here right now\. However, it can arise for two reasons\. 

The first one is that there may be classes with the same name in different packages\. In this case, it's unclear what kind of _LazyField_ is required, and the program will run in a different way than intended\.

The second one is related to changing the class name\. If the name is changed, the code won't run as intended at all\. And searching for all such strings in a huge code base is very difficult\. Even if you resort to searching, it's something you can just forget about\.

It'd be much safer to use the _instanceof_ operator:

```cpp
flags.append(f instanceof LazyDocument.LazyField
             ? FieldFlag.LAZY.getAbbreviation(): '-');
```

In this case, we wouldn't need to check for _null_, and the code would be much shorter\. The chance of an error would also decrease, if there are classes with the same name in different packages, or if the name of the class changes\. 

The analyzer detected a potential error and issued a warning:

[V6054](https://pvs-studio.com/en/docs/warnings/v6054/) Classes should not be compared by their name\. LukeRequestHandler\.java 247

## What about documentation, though?

As a final fragment, we'll look at the following code:

```cpp
@Override
public UpdateCommand clone() {
  try {
    return (UpdateCommand) super.clone();
  } catch (CloneNotSupportedException e) {
    return null;                         // <=
  }
}
```

Let's see what's wrong with it, because everything seems fine at first glance\. The analyzer informs us that returning _null_ in the _clone_ method is a bad idea:

[V6073](https://pvs-studio.com/en/docs/warnings/v6073/) It is not recommended to return null from 'clone' method\. UpdateCommand\.java 97

Why is it not recommended to return _null_ from _clone_? It's time to consult the [Java documentation](https://docs.oracle.com/javase/8/docs/api/java/lang/Object.html#clone--):

> Returns:
>
>  a clone of this instance\.
>
> Throws: 
>
> [CloneNotSupportedException](https://docs.oracle.com/javase/8/docs/api/java/lang/CloneNotSupportedException.html) \- if the object's class does not support the Cloneable interface\. Subclasses that override the clone method can also throw this exception to indicate that an instance cannot be cloned\.

The exception here indicates that the object can't be cloned\. The method should return only a copy of the current object and nothing else\. But why the analyzer doesn't recommend returning _null_ from _clone_? It's all about further use of the code\. If you constantly deviate from the recommendations in the documentation, it's difficult to catch non\-standard situations\.

Let's imagine a scenario where we want to use the _UpdateCommand_ class, but the source code is unavailable, and we can't decompile it\. Or we're just lazy\. We can only use the built\-in library with this class and focus on the interface\. Our program needs us to use the _clone_ method, so we write the following code:

```cpp
try {
  UpdateCommand localCopy = field.clone(); 
  System.out.println(localCopy.toString();
} catch (CloneNotSupportedException e) {
  System.out.println("Could not clone the field"); 
}
```

In this code, we try to catch the _CloneNotSupportedException_, but we can't because the exception is a _NullPointerException_ that causes the program to crash when calling _localCopy\.ToString\(\)_\. This comes as a complete surprise to the developer\. Deviating from official recommendations can be annoying, so it's better to always follow them :\)

## Conclusion

Let's stop here and take another look at the errors we found\. Most of them are the result of carelessness, but there are some that require additional thought\. For example, comparing class names without considering packages, or returning _null_ instead of throwing an exception in the _clone_ method\.

Without special development tools like static analyzers, such bugs are difficult to find, especially in projects as large as Apache Solr\. If you'd like to search for such non\-obvious errors in your project, you may try our static analyzer [here](https://pvs-studio.com/en/pvs-studio/try-free/)\.

By the way, Solr isn't the only Apache product we checked:

* [21 bugs in 21st version of Apache NetBeans](https://pvs-studio.com/en/blog/posts/java/1117/)
* [Big / Bug Data: analyzing the Apache Flink source code](https://pvs-studio.com/en/blog/posts/java/0781/)
* [Apache Hadoop code quality: production vs test](https://pvs-studio.com/en/blog/posts/java/0697/)