ParseResult.get() inserts an entry for a URL that was never parsed

[rzo1]

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:

        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.

/*
 * 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.

[abhinav-phi]

Hi! I'd like to take this issue.
Plan: I'll fix ParseResult.get() so it doesn't insert an entry for a URL that wasn't parsed, and add a unit test that covers this exact case.
Could you please assign this to me? Thanks!

/assign
take