ParseResult: non-mutating lookup, explicit getOrCreate() and no empty documents emitted

[abhinav-phi]

Fixes #2108

The problem

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 (JSoupParserBolt and the Tika ParserBolt) 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.

Nothing in the tree hits this today: every non-test caller 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; if the URL it probes comes from page content, the probe creates an entry that the bolt then 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.

What this PR changes

1. ParseResult now separates lookup from creation

• getIfPresent(url) — new read-only accessor: returns the ParseData stored for the URL or null, and never modifies the ParseResult. This is what a parse filter should use to probe whether a sub-document already exists.
• getOrCreate(url) — the creating accessor used by put(), set() and the parser bolts for the parent URL; modifications made to the returned instance are stored in the ParseResult.
• get(url) is removed. Since the next release is 4.0.0, a major one, the ambiguous method is dropped outright rather than deprecated. Filters that used get(url) to check for a sub-document must switch to getIfPresent(url); those that used it to build one must switch to getOrCreate(url), put(url, key, value) or set(url, metadata).

All in-tree callers of parse.get(url) — the parse filters in core (CommaSeparatedToMultivaluedMetadata, CollectionTagger, DomainParseFilter, LDJsonParseFilter, LinkParseFilter, MD5SignatureParseFilter, MimeTypeNormalization, XPathFilter), the jsoup filters, LanguageID (langid), JsRenderingDetector (playwright) and both parser bolts — have been migrated to getOrCreate(url). This is a mechanical, behaviour-identical change.

2. Parser bolts skip empty sub-documents, never the document itself

JSoupParserBolt and the Tika ParserBolt skip entries which carry no content, no text and no metadata when they emit — except the entry for the URL being parsed, which is always emitted even if a filter emptied it, so that its status keeps being updated downstream. Phantoms created by a lookup on a never-parsed URL are by construction sub-documents, so the exemption is scoped to the tuple's own URL.

Each skip increments a skipped_empty_documents counter (visible in the bolt's metrics) and logs the skipped URL at debug level.

3. Consistent ParseData constructors

The no-arg and metadata-only ParseData constructors now set content to an empty byte array, like the (text, metadata) constructor, so no code path can emit a null content field.

4. Documentation for filter authors

The custom parse filter example in docs/src/main/asciidoc/extending.adoc now uses getOrCreate(url), and docs/src/main/asciidoc/internals.adoc documents the emission semantics: use getOrCreate(url) to add data for a URL, getIfPresent(url) for a read-only lookup, and set at least one of content, text or metadata on a sub-document for it to be emitted.

Release note for filter authors: ParseResult.get(url) has been removed — use getIfPresent(url) for a read-only lookup or getOrCreate(url) to create an entry. Sub-documents are only emitted if they carry content, text or metadata; entries with none of the three are skipped (counted under skipped_empty_documents) unless they belong to the URL being parsed.

Tests

• ParseResultTest (new, core): verifies that getIfPresent() returns null for an unknown URL without mutating the result and returns the stored instance otherwise; that getOrCreate(), put() and set() create entries and getOrCreate() returns the same instance on repeated calls; that getValues() does not mutate the result; and that all ParseData constructors produce a non-null, empty content array.
• SubDocumentsFilterTest#testEmptySubDocumentsAreNotEmitted (new, core): a parse filter creates an entry for a URL that was never parsed; the JSoupParserBolt emits only the parent document.
• SubDocumentsFilterTest#testEmptiedParentDocumentIsStillEmitted (new, core): a filter empties the document itself (no content, no text, no metadata); it is still emitted.
• ParserBoltTest#testEmptySubDocumentsAreNotEmitted and testEmptiedParentDocumentIsStillEmitted (new, tika): same two checks for the Tika ParserBolt.
• SubDocumentsParseFilter (test fixture): the sub-documents it creates now carry a metadata entry — since empty sub-documents are no longer emitted, the fixture creates realistic sub-documents and still asserts that they are emitted.

Verification

• mvn -pl external/tika -am clean test — core 425 tests, 0 failures; tika 4 tests, 0 failures.
• mvn -pl core,external/tika -Prat verify -DskipTests -Dskip.format.code=false — checkstyle 0 violations, google-java-format validation passed, Apache RAT passed.

[dpol1]

Thanks for the PR @abhinav-phi. Compat side looks well handled - deprecated get() keeps auto-create, all in-tree callers moved to getOrCreate. Two questions on the emit side.

The empty-entry skip is silent - no log, counter or opt-out. URL-only subdocuments as a signal is a real pattern (this PR patched its own fixture for exactly that); should we at least count the skips and add a release-note line for filter authors?

Also wondering: could the skip apply only to entries other than the tuple's own URL? Phantoms are by construction subdocuments, and exempting the parent would rule out the corner case where the main document vanishes and the URL keeps refetching.

[abhinav-phi]

Thanks for the review @dpol1 — both points are addressed in 1c7b442:

Skip scoped to sub-documents. The bolts now skip empty entries only for URLs other than the one being parsed. The entry for the document itself is always emitted, even when a filter emptied it, so its status keeps being updated downstream — the corner case you describe is ruled out. Locked in by testEmptiedParentDocumentIsStillEmitted in both SubDocumentsFilterTest (core) and the tika ParserBoltTest.

Skips are no longer silent. Each skip increments a skipped_empty_documents scoped counter (alongside the existing tuple_success metrics) and logs the skipped URL at debug level.

Release note for filter authors. The repo has no in-tree changelog file, so the note lives in the ParseFilter section of docs/src/main/asciidoc/internals.adoc and in the PR description: sub-documents should set at least one of content, text or metadata to be emitted, and getIfPresent(url) / getOrCreate(url) are the recommended accessors going forward.

All core + tika tests pass (426 core, 4 tika), and the checkstyle / google-java-format / RAT gates are green.

[jnioche]

the next release will be a major one, we might as well get rid of the method straight away.

[abhinav-phi]

Agreed — since the next release is a major one, get(url) is now removed outright in 8aea8e9 instead of being deprecated. All in-tree callers had already been migrated to getOrCreate(url), and filters that only want to check for an existing entry can use the new getIfPresent(url).

[jnioche]

if we totally deprecate get() - we will probably never come across those empty docs. Happy to keep the check and the log message but probably could get rid of that lengthy AI generated Javadoc

[abhinav-phi]

Kept the check and the log (as a backstop for external filters that still create empty entries via put/set/getOrCreate), plus the skipped_empty_documents counter. The lengthy block comment above the loop is now two lines, and the new accessors' Javadoc has been cut down to the essentials — 8aea8e9.

[abhinav-phi]

The rat failure on 8aea8e9 was a compile error I introduced with the removal of ParseResult.get(): JsRenderingDetectorTest holds the ParseResult in a variable named p, so the earlier mechanical rename of parse.get(...) missed its 14 call sites. Fixed in 5a44a45 by switching those to getOrCreate("u"), and verified locally with a full mvn clean verify across the whole reactor (all 18 modules build, checkstyle and google-java-format clean).

The CI run for 5a44a45 is currently waiting for a maintainer to approve it (workflow runs from first-time contributors require manual approval) — once approved it should go green.