jnioche commented on code in PR #2251:
URL: https://github.com/apache/stormcrawler/pull/2251#discussion_r4337736496
##########
core/src/main/java/org/apache/stormcrawler/util/CharsetIdentification.java:
##########
@@ -61,6 +61,18 @@ public class CharsetIdentification {
*/
public static String getCharsetFast(
final Metadata metadata, final byte[] content, final int
maxLengthCharsetDetection) {
+ return getCharsetFast(metadata, content, maxLengthCharsetDetection,
"");
Review Comment:
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`, or at least say in their javadoc that they
expect unprefixed header keys?
##########
core/src/main/java/org/apache/stormcrawler/util/CharsetIdentification.java:
##########
@@ -105,7 +129,7 @@ public static String getCharset(
}
// then look at what we get from HTTP headers and HTML content
- String httpCharset = getCharsetFromHTTP(metadata);
+ String httpCharset = getCharsetFromHTTP(metadata,
protocolMetadataPrefix);
Review Comment:
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.
##########
core/src/test/java/org/apache/stormcrawler/bolt/JSoupParserBoltTest.java:
##########
@@ -365,4 +369,36 @@ void testBaseHrefOutlinkResolution() throws IOException {
"https://www.example.be/fr/rayons/stockage-archivage-de-donnees/218725";,
statusTuples.get(0).get(0));
}
+
+ /**
+ * The fetcher stores the response headers under protocol.md.prefix, so
the charset declared in
+ * the Content-Type has to be read with that prefix. Without it the header
is never seen, the
+ * header and the meta charset can never agree, and ICU guesses a UTF-8
page whose detection
+ * window is plain ASCII as ISO-8859-1, see issue #2250.
+ */
+ @Test
+ void prefixedContentTypeCharsetIsUsed() throws IOException {
Review Comment:
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.
##########
core/src/main/java/org/apache/stormcrawler/util/CharsetIdentification.java:
##########
@@ -69,7 +81,7 @@ public static String getCharsetFast(
}
// then look at what we get from HTTP headers
- charset = getCharsetFromHTTP(metadata);
+ charset = getCharsetFromHTTP(metadata, protocolMetadataPrefix);
Review Comment:
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.
##########
core/src/main/java/org/apache/stormcrawler/util/CharsetIdentification.java:
##########
@@ -61,6 +61,18 @@ public class CharsetIdentification {
*/
public static String getCharsetFast(
final Metadata metadata, final byte[] content, final int
maxLengthCharsetDetection) {
+ return getCharsetFast(metadata, content, maxLengthCharsetDetection,
"");
+ }
+
+ /**
+ * Same as {@link #getCharsetFast(Metadata, byte[], int)}, reading the
HTTP Content-Type under
+ * the prefix the fetcher stores the response headers with
(protocol.md.prefix).
+ */
+ public static String getCharsetFast(
+ final Metadata metadata,
+ final byte[] content,
+ final int maxLengthCharsetDetection,
+ final String protocolMetadataPrefix) {
Review Comment:
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.
--
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]
