#2250 CharsetIdentification: read the content type under the protocol prefix by nicoscandolo · Pull Request #2251 · apache/stormcrawler

nicoscandolo · 2026-10-20T17:57:24Z

CharsetIdentification read the HTTP Content-Type without protocol.md.prefix, so the charset declared by the server was never seen: it could not agree with the meta charset, and UTF-8 pages whose detection window is plain ASCII were decoded as ISO-8859-1.

getCharset and getCharsetFast get an overload that takes the protocol metadata prefix; the existing signatures delegate with an empty prefix, so other callers are unchanged.
JSoupParserBolt passes protocolMetadataPrefix, the same prefix it already uses to read the mime type.
New test JSoupParserBoltTest#prefixedContentTypeCharsetIsUsed; the mocked call in charsetDetectionFailureIsReportedAsParseError now matches the new signature.

…otocol prefix

jnioche

Thanks @nicoscandolo, the fix looks right and matters more than it might seem: crawler-default.yaml sets protocol.md.prefix: "protocol.", so with the default config the HTTP charset was never seen.

One remaining caller isn't in the diff, so I can't comment on it inline: external/playwright/.../parsefilter/JsRenderingDetector.java:226 still calls CharsetIdentification.getCharsetFast(md, content, -1). The parse metadata carries the prefixed headers, so #2250 is still present there. That filter would need to read protocol.md.prefix from stormConf in configure() and use the new overload.

A few more points inline.

jnioche · 2026-10-21T13:57:47Z

     */
    public static String getCharsetFast(
            final Metadata metadata, final byte[] content, final int maxLengthCharsetDetection) {
+        return getCharsetFast(metadata, content, maxLengthCharsetDetection, "");


These 3-arg overloads now silently mean "ignore the prefix". Fetched metadata almost always carries one ("protocol." by default), so any caller that keeps using the familiar signature keeps the bug without warning, as JsRenderingDetector does. Could we deprecate the 3-arg getCharset/getCharsetFast?

Done: both 3-arg methods are @Deprecated, with a @deprecated note pointing to the prefixed overload. No production code calls them anymore. The existing CharsetIdentificationTest cases still exercise them.

thanks @nicoscandolo
actually, since we are about to do a major release, now would be a good time to simply remove them. besides, I doubt that they are used directly by any code written by users so the impact is low anyway

Makes sense, removed both 3-arg methods. Nothing outside the tests used them, and those now pass an empty prefix.

jnioche · 2026-10-21T13:57:47Z

+            final Metadata metadata,
+            final byte[] content,
+            final int maxLengthCharsetDetection,
+            final String protocolMetadataPrefix) {


Nit: please add @param protocolMetadataPrefix (noting it must include its separator, e.g. "protocol.", like Metadata#putAll(Metadata, String) documents) and an @since tag, since the existing method has @since 1.18. Same for the getCharset overload.

Done, on both overloads: @param protocolMetadataPrefix with the separator note, and @since 4.0.

jnioche · 2026-10-21T13:57:47Z


        // then look at what we get from HTTP headers
-        charset = getCharsetFromHTTP(metadata);
+        charset = getCharsetFromHTTP(metadata, protocolMetadataPrefix);


Worth a line in the release notes: under the default config this step was effectively dead before, so in fast mode the meta charset won. Now the server header takes precedence with no further check. That follows the HTML spec, but sites with a wrong default header (e.g. charset=ISO-8859-1 on a UTF-8 page with a correct <meta charset>) will now decode differently for detect.charset.fast: true users.

Agreed, worth a line. Suggested wording:

With the default protocol.md.prefix, the charset in the HTTP Content-Type was never read (#2250). It is now. With detect.charset.fast: true the server header takes precedence, as the HTML spec says, so pages whose header disagrees with their <meta charset> will decode differently. With the default detect.charset.fast: false, the header is used when it agrees with the meta charset.

jnioche · 2026-10-21T13:57:47Z


        // then look at what we get from HTTP headers and HTML content
-        String httpCharset = getCharsetFromHTTP(metadata);
+        String httpCharset = getCharsetFromHTTP(metadata, protocolMetadataPrefix);


Side effect to be aware of: before, httpCharset was always null here, so the meta charset was always passed to ICU as a hint. Now, when the header and the meta disagree (a common server misconfiguration), hintCharset stays null and ICU guesses unhinted. On an ASCII-heavy window that can produce the same ISO-8859-1 result as #2250 for a different set of pages. That may be fine since ICU largely ignores the hint anyway, but it would be good to decide it on purpose, e.g. prefer the meta charset as the hint when they disagree.

Good catch, made it explicit: when the header and the meta disagree, the meta charset is the hint (htmlCharset != null ? htmlCharset : httpCharset). That keeps what prefixed deployments did before the fix.

One side effect: without a prefix the header was already read, so for those setups a disagreement now passes the meta as the hint instead of no hint. Since ICU barely uses the hint, I didn't add a test for it: I couldn't build a realistic page whose result depends on it without the test relying on ICU internals. Happy to change it if you'd rather keep no hint on disagreement.

jnioche · 2026-10-21T13:57:48Z

+     * window is plain ASCII as ISO-8859-1, see issue #2250.
+     */
+    @Test
+    void prefixedContentTypeCharsetIsUsed() throws IOException {


This only covers getCharset, because detect.charset.fast defaults to false. Could you also run it with detect.charset.fast: true (e.g. as a parameterized test), and add a direct unit test of the new 4-arg overloads in CharsetIdentificationTest with a prefixed Content-Type? As it stands, reverting the fast-path call in JSoupParserBolt to the 3-arg form would not fail any test.

Done:

The bolt test is parameterized on detect.charset.fast. In fast mode it runs without a meta charset, since the meta would be returned otherwise.

Added fastDetectionReadsThePrefixedContentType and detectionReadsThePrefixedContentType to CharsetIdentificationTest.

I checked that reverting the fast-path call in JSoupParserBolt to the 3-arg form now fails the fast case, and that ignoring the prefix in the overloads fails all the new tests.

…ngDetector, meta charset as the hint, more tests

nicoscandolo · 2026-10-21T18:30:45Z

Thanks for the review! I pushed a commit that addresses it, replies inline.

On JsRenderingDetector: it now reads protocol.md.prefix in configure() and uses the new overload. The test uses an ISO-8859-15 page with a euro sign, a charset ICU doesn't detect, so it only passes when the prefixed header is read.

jnioche

thanks @nicoscandolo
just a couple of minor changes regarding deprecations

jnioche · 2026-10-21T19:55:32Z

+     * @deprecated reads the HTTP Content-Type without a prefix, while fetched metadata carries the
+     *     one set by protocol.md.prefix: use {@link #getCharset(Metadata, byte[], int, String)}
     */
+    @Deprecated


same as above

Done, removed as well (same commit).

…oads

jnioche

thanks @nicoscandolo

dpol1

LGTM.

jnioche · 2026-10-22T11:18:56Z

@nicoscandolo the RAT checks failed - see https://github.com/apache/stormcrawler/actions/runs/39675400580/job/119680591137?pr=2251

any chance you could fix the underlying problems?

nicoscandolo · 2026-10-22T12:33:11Z

Thanks! Fixed in the last commit: the job failed on Checkstyle (AvoidEscapedUnicodeCharacters in the three new tests). They now use the literal characters, and Checkstyle passes locally.

rzo1

Thanks, LGTM.

apache#2250 CharsetIdentification: read the content type under the pr…

e3e5bcf

…otocol prefix

jnioche reviewed Oct 21, 2026

View reviewed changes

apache#2250 Review: deprecate the unprefixed overloads, fix JsRenderi…

6e8deda

…ngDetector, meta charset as the hint, more tests

jnioche requested changes Oct 21, 2026

View reviewed changes

apache#2250 Remove the unprefixed getCharset and getCharsetFast overl…

652a1aa

…oads

jnioche approved these changes Oct 22, 2026

View reviewed changes

jnioche added this to the 4.0.0 milestone Oct 22, 2026

jnioche added the bug label Oct 22, 2026

dpol1 approved these changes Oct 22, 2026

View reviewed changes

apache#2250 Use literal characters in the new tests (checkstyle)

eca1b76

rzo1 approved these changes Oct 22, 2026

View reviewed changes

rzo1 assigned nicoscandolo Oct 22, 2026

rzo1 merged commit 9c8b85a into apache:main Oct 22, 2026
2 checks passed

Conversation

nicoscandolo commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnioche left a comment

Choose a reason for hiding this comment

Uh oh!

jnioche Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoscandolo commented Oct 21, 2026

Uh oh!

jnioche left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnioche left a comment

Choose a reason for hiding this comment

Uh oh!

dpol1 left a comment

Choose a reason for hiding this comment

Uh oh!

jnioche commented Oct 22, 2026

Uh oh!

nicoscandolo commented Oct 22, 2026

Uh oh!

rzo1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nicoscandolo commented Oct 20, 2026 •

edited

Loading

jnioche Oct 21, 2026 •

edited

Loading