다시 한번 Apache 제품을 확인하고 있습니다. 이번에는 오픈소스 검색 서버 플랫폼인 Solr를 선택했습니다. Solr를 사용하면 데이터베이스 및 온라인 리소스의 정보를 빠르고 효율적으로 검색할 수 있습니다. 이렇게 복잡한 작업에 직면하면 숙련된 Apache 개발자라도 실수하기 쉽습니다. 이번 글에서는 이러한 유형의 실수를 살펴보겠습니다.
당신은 누구입니까, 솔르?
얼마 전 가장 유명한 Apache 프로젝트 중 하나인 NetBeans IDE를 확인했습니다. 우리는 검사 중에 분석기에서 발행된 흥미로운 경고를 많이 발견했습니다. 그건 그렇고, 개발자들은 그것을 빨리 알아채고 나보다 먼저 끌어오기 요청을 했습니다. :) 이번에 우리는 그들의 또 다른 주요 제품 중 하나인 Solr 전체 텍스트 검색 플랫폼을 살펴보기로 결정했습니다.
2006년에 처음 출시된 Apache Solr는 동적 클러스터링 및 데이터베이스 통합부터 복잡한 형식의 문서 처리에 이르기까지 광범위한 기능을 제공합니다. 이 검색 엔진을 사용하면 웹사이트의 정보를 빠른 속도로 검색하고 분석할 수 있으며 Linux에서 실행되는 하드웨어에서 검색 서버를 호스팅하는 기능도 제공합니다. 솔르는...
간단히 말해서 너무 많은 세부사항으로 여러분을 귀찮게 하고 싶지 않습니다. 빅데이터를 최적화하는 편리한 소프트웨어 플랫폼이라는 사실만 알면 충분하다. 소스 코드를 살펴보고 흥미롭거나 특이한 것을 검색해 보는 것은 어떨까요? 이것이 바로 우리가 지금 하려는 일입니다.
여기서 뭔가를 섞은 것 같군요
프로그래머는 연산자가 뒤섞인 오타로부터 결코 안전하지 않습니다. 그 중 하나는 다음과 같습니다.
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;
}
로그인 후 복사
이 방법은 meta 변수를 사용하여 시스템 스냅샷에 대한 정보를 저장합니다. 위의 코드 조각에는 meta != null 검사도 있습니다. 그렇다면 IllegalArgumentException이 발생합니다. 그것만으로도 이상한 것 같습니다. 좋습니다. 이제 루프 본문을 살펴보겠습니다. getReplicaSnapshotsForShard가 호출되는 곳입니다. 여기서 meta가 항상 null이면 NullPointerException이 발생합니다. 개발자가 오타를 내고 연산자를 혼동한 것 같습니다. 따라서 meta가 null과 같으면 예외가 발생해야 합니다.
PVS-Studio 분석기가 교정자로 작동하여 발견된 오타를 보고했습니다.
V6008 '메타'의 Null 역참조입니다. SolrSnapshotsTool.java 262
연산자가 뒤섞여 발생하는 이러한 오류는 생각보다 더 흔하다는 점을 덧붙이고 싶습니다. 예를 들어, NetBeans 프로젝트에서 이미 유사한 항목을 두 개 이상 보았습니다.
private SourcesModel getModel() {
SourcesModel tm = model.get();
if (tm == null) {
tm.sourcePath.removePropertyChangeListener (this);
tm.debugger.getSmartSteppingFilter ().
removePropertyChangeListener (this);
}
return tm;
}
로그인 후 복사
V6008 'tm'의 Null 역참조입니다. SourcesModel.java 713
public void propertyChange(PropertyChangeEvent evt) {
....
synchronized (this) {
artifacts = null;
if (listeners == null && listeners.isEmpty()) {
return;
}
....
}
}
로그인 후 복사
V6008 '리스너'에 대한 Null 역참조입니다. MavenArtifactsImplementation.java 613
새로운 오타 패턴을 발견했을 수도 있습니다. 계속 지켜보겠습니다.
다음 조각으로 넘어가겠습니다. 이 수업에서 개발자는 get 메서드
중 하나에서 무엇을 반환할지 혼동했습니다.
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;
}
....
}
로그인 후 복사
이 클래스는 특정 작성된 수학 함수를 구문 분석하기 위해 설계되었습니다. 파서의 동작을 변경하는 두 가지 속성이 있습니다. parseMultipleSources는 숫자 값의 모든 소스를 분석하고 parseToEnd는 문자열이 포함된 함수를 끝까지 구문 분석해야 하는지 확인합니다.
이제 이 필드에 대한 get 및 set 메소드를 살펴보겠습니다. parseMultipleSources 필드는 getParseToEnd에 반환됩니다. 프로그래머가 여기에 어떤 필드를 반환해야 할지 혼동했습니다.
분석기는 일치하지 않는 반환 필드를 쉽게 감지합니다.
V6091 의심스러운 getter 구현입니다. 아마도 'parseToEnd' 필드가 대신 반환되어야 할 것입니다. FunctionQParser.java 87, FunctionQParser.java 57
다음 코드 조각의 오타로 인해 NullPointerException이 발생할 수 있습니다.
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);
}
로그인 후 복사
자세히 살펴보겠습니다. value를 먼저 null과 비교한 후 다음 줄에서는 length() 메서드가 호출됩니다. 가치. 하지만 변수는 null일 수 있습니다! 아마도 개발자는 length()를 호출하는 대신 len 변수를 사용해야 했습니다.
분석기 메시지 덕분에 이 오타를 발견했습니다.
V6008 '값'의 null 역참조 가능성이 있습니다. IndexSizeEstimator.java 735, IndexSizeEstimator.java 736
오타가 있는 또 다른 코드 조각을 살펴보겠습니다.
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(....);
}
}
....
}
로그인 후 복사
여기서는 for 루프에 주목할 가치가 있습니다. 평소와 같이 프로그래머는 루프와 idx 카운터 변수를 선언한 다음 목록에 있는 항목 수를 가져옵니다. 그러나 문제가 있습니다. 루프를 반복할 때마다 0: list.get(0) 인덱스
에 있는 요소만 사용합니다.
PVS-Studio 분석기가 이 오류를 감지했습니다:
V6016 루프 내부의 상수 인덱스를 통해 'list' 객체의 요소에 대한 의심스러운 액세스입니다. AscEvaluator.java 56
The following example shows two methods with different names. However, they do the same thing.
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 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.
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 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.
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 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.
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 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.
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 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.
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 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.
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 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.
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 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 us to write cleaner code.
By the way, we just recently released an article 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.
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:
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 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:
@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 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:
Returns:
a clone of this instance.
Throws:
CloneNotSupportedException - 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:
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.
By the way, Solr isn't the only Apache product we checked:
- 21 bugs in 21st version of Apache NetBeans
- Big / Bug Data: analyzing the Apache Flink source code
- Apache Hadoop code quality: production vs test
위 내용은 Apache Solr의 버그 검색어의 상세 내용입니다. 자세한 내용은 PHP 중국어 웹사이트의 기타 관련 기사를 참조하세요!
![[웹 프런트엔드] Node.js 빠른 시작](https://img.php.cn/upload/course/000/000/067/662b5d34ba7c0227.png)
