#108 Normalize misspelled HTTP headers

[HarshDevelops]

Thank you for contributing to Apache StormCrawler.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes

• Is there a issue associated with this PR? Is it referenced in the commit message?

• Does your PR title start with #XXXX where XXXX is the issue number you are trying to resolve?

• Has your PR been rebased against the latest commit within the target branch (typically main)?

• Is your initial contribution a single, squashed commit?

• Is the code properly formatted with mvn git-code-format:format-code -Dgcf.globPattern="**/*" -Dskip.format.code=false?

For code changes

• Have you ensured that the full suite of tests is executed via mvn clean verify?
• Have you written or updated unit tests to verify your changes?
• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
• If applicable, have you updated the LICENSE file, including the main LICENSE file?
• If applicable, have you updated the NOTICE file, including the main NOTICE file?

Note

Please ensure that once the PR is submitted, you check GitHub Actions for build issues and submit an update to your PR as soon as possible.

Closes #108

Summary:

• Normalize HTTP header metadata when a known header name is misspelled or its hyphens are omitted.
• Leave non-HTTP metadata unchanged.
• Add regression coverage for both cases.

Testing:

• mvn -pl core -Dtest=MetadataTest test
• mvn -pl core test
• mvn git-code-format:format-code -Dgcf.globPattern="**/*" -Dskip.format.code=false
• mvn --no-transfer-progress clean verify reached stormcrawler-opensearch, then failed locally because Docker/Testcontainers was unavailable; skipped tests then caused the module JaCoCo threshold to fail.

[jnioche]

I like the purpose but would rather have that outside of Metadata itself. We have comparable utility classes in o.a.s.util.*, this would be the right place for it I think.
Metadata is not just about HTTP Headers, I'd rather keep it slim. Does that make sense @HarshDevelops ?

[sebastian-nagel]

Thanks, @HarshDevelops! The implementation looks straightforward and solid.

However, it might need a rework... Not your fault, but because the web has changed since the implementation of Nutch's SpellCheckedMetadata. Also the way it's used in Nutch has changed or will change, cf. NUTCH-2563, NUTCH-3002/NUTCH-3122 and this discussion.

outside of Metadata itself. We have comparable utility classes in o.a.s.util.*, this would be the right place for it I think.

Definitely, that was even the case in Nutch, although not from the beginning on (apache/nutch@bbbc29d and apache/nutch@f510e19).

In addition, approximate header extraction shouldn't be the default.

Some context and explanations:

• Nutch's SpellCheckedMetadata class is there since the 2006, that is the times of HTTP/1.1, with HTTP/1.0 or 0.9 still frequently used. These are text-based protocols. When entire HTTP messages are hand-written (maybe as templates), typos could easily slip in.
• Nowadays, HTTP/2 is dominant. As a binary protocol, it's much less likely that header names are mispelled. Headers in HTTP/2 are lower-case, so case-insensitive header processing is mandatory.
• My impression is, that spellchecking headers unconditionally can introduce more harm than benefits. Below is one example, a WARC capture of a HTTP 302 redirect. The redirect requires the Location header. In addition, the response contains an X-Location header. The spell checker would map it also to Location.
• Standard header names prefixed by X- or another prefix are quite common, often they are added by a proxy or the front-end server, rewriting the original response headers.
• Would be better to do the approximate header lookup / mapping, only if an expected standard header is not found. In case of a HTTP redirect, if the Location header is missing.

WARC/1.0
WARC-Type: response
WARC-Date: 2023-03-26T14:32:42Z
WARC-Record-ID: <urn:uuid:5543a3db-2cff-43bc-af20-bdc5fd66d517>
Content-Length: 879
Content-Type: application/http; msgtype=response
WARC-Warcinfo-ID: <urn:uuid:4d3666d6-9165-4654-a45f-59bd4e135488>
WARC-Concurrent-To: <urn:uuid:1a58671a-0c2c-4d50-8bfd-ba9b7302810f>
WARC-IP-Address: 54.162.128.250
WARC-Target-URI: https://legacy.latam.coderhouse.com/blog/gente-pongan-moderadores
WARC-Payload-Digest: sha1:ZYUB2QWIWMHTIA6667UBTITTQMBDP3QH
WARC-Block-Digest: sha1:GDQFDDKZHNZJT6XQVOTKYG4C3MKV63QR
WARC-Identified-Payload-Type: text/html

HTTP/1.1 302 Found
Connection: keep-alive
Date: Sun, 26 Mar 2023 14:32:42 GMT
Content-Type: text/html; charset=utf-8
Content-Length: 160
X-Dns-Prefetch-Control: off
Expect-Ct: max-age=0
Strict-Transport-Security: max-age=15552000; includeSubDomains
X-Download-Options: noopen
X-Content-Type-Options: nosniff
X-Permitted-Cross-Domain-Policies: none
Referrer-Policy: no-referrer
X-Xss-Protection: 0
Access-Control-Allow-Origin: *
Access-Control-Allow-Headers: Origin, X-Requested-With, Content-Type, Accept
Cache-Control: public, max-age=3600
Location: https://latam.coderhouse.com/blog/gente-pongan-moderadores
Vary: Accept, Accept-Encoding
Server: Coderhouse-IT
X-Location: cached
Via: 1.1 vegur

<p>Found. Redirecting to <a href="https://latam.coderhouse.com/blog/gente-pongan-moderadores">https://latam.coderhouse.com/blog/gente-pongan-moderadores</a></p>

[rzo1]

Thanks @HarshDevelops for the work here, and @sebastian-nagel for the detailed
context. I agree with Sebastian's closing point: the approximate lookup should
only happen when an expected standard header is not found, rather than
unconditionally on every key.

I replicated the matching logic from this PR against the real HttpHeaders
dictionary to see how it behaves in practice. Three things came out of it:

1. Sebastian's X-Location example reproduces, and isn't alone:

X-Location  -> Location  (distance=1, threshold=3)
X-Server    -> Server    (distance=1, threshold=2)

Because this hooks into normalizeKey(), it fires in addValue(), so it is
destructive at write time. X-Location: cached gets appended into Location's
value array, and since getFirstValue() returns values[0], which value wins
depends on server-controlled header order. In the wrong order we'd follow a
redirect to cached.

2. The dictionary is a poor fit for crawling. org.apache.http.HttpHeaders
gives 55 constants, a good share of them WebDAV (Dav, Depth, Destination,
Overwrite, Lock-Token, Status-URI), while missing nearly everything we
actually care about: Set-Cookie, Strict-Transport-Security,
Content-Security-Policy, Alt-Svc, Link, Content-Disposition,
Access-Control-Allow-Origin. So we take on fuzzy-match risk for headers nobody
sends, and get no help with the ones that matter.

3. There's a performance cost on a path that can't absorb one.
normalizeKey() backs all 8 read/write methods on Metadata, so it runs for
every key, of every tuple, in every bolt. As written, every miss (which means
all of our internal keys like fetch.loadingTime or protocol.set.cookie)
falls through to a Levenshtein scan across all 55 constants, allocating two
int[] per comparison. That turns our hottest metadata path from a single
toLowerCase() into something substantially more expensive, for keys that are
never HTTP headers to begin with.

Sebastian's lazy/on-miss design happens to fix all three at once: the caller
names the header it wants, so it becomes a targeted 1:1 probe instead of a
nearest-neighbour search. X-Location can never be consulted while Location
is present. Nothing is merged, the original key survives, and the cost is only
paid on a genuine miss.

It also lines up with @jnioche's earlier point, and in fact requires it: "is the
expected header missing?" is only knowable at the call site, so this can't live
inside Metadata anyway.

Suggested way forward

Something like o.a.s.util.HttpHeaderResolver#getFirstValue(Metadata, String):
exact match -> separator/case-insensitive alias -> fuzzy fallback last, opt-in
via config and off by default. Then call it from the few places that need a
specific header (redirect Location, Content-Type, Last-Modified) rather
than globally.

One thing worth saying clearly: the alias half of this PR is the valuable part
and carries no risk. ContentType, contenttype, content_type ->
Content-Type are all exact dictionary hits that need no Levenshtein at all.
That could land on its own, separately from the spellchecking, if you'd like to
split it. It would close the useful part of #108 without any of the above.

Also, given NUTCH-2563 / NUTCH-3122, Nutch seems to be moving in this same
direction, so going the on-miss route keeps us aligned rather than porting a
class upstream is walking back.

Happy to help review the reworked version.