rzo1 opened a new issue, #2108:
URL: https://github.com/apache/stormcrawler/issues/2108
## What happens
`ParseResult.get(url)` is documented as a convenience accessor, but on a
miss it creates a `ParseData` and stores it in the map before returning it.
`put(url, key, value)` and `set(url, metadata)` go through the same path. Both
parser bolts iterate the whole map and emit one document tuple per entry, so an
entry created by a lookup is emitted like a parsed document. The auto-created
entry also has a null content field, while the `(text, metadata)` constructor
sets an empty byte array, so the two paths do not agree on what an empty
document looks like.
## Where
`core/src/main/java/org/apache/stormcrawler/parse/ParseResult.java:72-79`:
```java
ParseData parse = parseMap.get(url);
if (parse == null) {
parse = new ParseData();
parseMap.put(url, parse);
```
Emitted at
`core/src/main/java/org/apache/stormcrawler/bolt/JSoupParserBolt.java:498` and
`external/tika/src/main/java/org/apache/stormcrawler/tika/ParserBolt.java:301`.
Constructors that disagree on the content default: `ParseData.java:27` and
`:37` against `:31`.
## Why it matters
Nothing in the tree hits this today: every non-test caller in core and in
the external modules passes the URL of the page being parsed, so the map only
ever gains entries for documents that exist. The problem is the API. A parse
filter written against the Javadoc may reasonably call `get()` to check whether
an earlier filter produced a sub-document, and if the URL it probes comes from
page content, the probe creates an entry that the bolt emits as a document.
Sub-documents are not filtered by `URLFilters`, only outlinks are, so that
entry reaches the indexer as a document URL with no content. This is API
hygiene rather than something operators need to act on.
## Reproduction
Save as
`core/src/test/java/org/apache/stormcrawler/parse/ParseResultLookupTest.java`.
```java
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to you under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.stormcrawler.parse;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
/**
* Documents the current behaviour of ParseResult.get(): it inserts an entry
for a URL that was
* never parsed, and that entry has a null content field. A read-only lookup
should leave the map
* untouched, and an entry that reaches the parser bolt should not carry
null content.
*/
class ParseResultLookupTest {
@Test
void getInsertsAnEntryForAnUnknownUrl() {
ParseResult parse = new ParseResult();
Assertions.assertEquals(0, parse.size());
ParseData data = parse.get("https://example.com/never-parsed";);
// current behaviour: the lookup created and stored an entry
Assertions.assertEquals(1, parse.size());
Assertions.assertTrue(parse.getParseMap().containsKey("https://example.com/never-parsed";));
// should be: 0 and false
// current behaviour: the auto-created entry has null content
Assertions.assertNull(data.getContent());
Assertions.assertNull(data.getText());
// should be: an empty byte array, as set by the (text, metadata)
constructor
}
@Test
void metadataOnlyConstructorLeavesContentNull() {
ParseData withText = new ParseData("text", new
org.apache.stormcrawler.Metadata());
Assertions.assertNotNull(withText.getContent());
ParseData metadataOnly = new ParseData(new
org.apache.stormcrawler.Metadata());
// current behaviour: inconsistent with the constructor above
Assertions.assertNull(metadataOnly.getContent());
}
}
```
Run it:
```
mvn -pl core test -Dtest=ParseResultLookupTest
```
It asserts the present behaviour and passes, with comments saying what the
behaviour should be; a failing test would prejudge whether `get()` keeps its
name and its semantics.
```
[INFO] Running org.apache.stormcrawler.parse.ParseResultLookupTest
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.035
s
```
The first case shows `size()` going from 0 to 1 after a lookup of a URL that
was never parsed, and the created entry carrying null content. The second shows
the constructor disagreement.
## Suggested fix
Add a non-mutating lookup to `ParseResult`, for example `getIfPresent(url)`
returning null, and say in the `get()` Javadoc that it creates an entry.
Renaming `get()` to `getOrCreate()` and keeping `get()` as a deprecated
delegate would make the two apart at the call site, at the cost of a source
change for filters outside this repository. As a backstop, have
`JSoupParserBolt` and the Tika `ParserBolt` skip entries with no content, no
text and empty metadata when they emit. Also set `content` to an empty array in
the no-arg and metadata-only `ParseData` constructors so no path emits a null
content field.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
