nicoscandolo commented on code in PR #2251:
URL: https://github.com/apache/stormcrawler/pull/2251#discussion_r4340483938
##########
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:
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.
##########
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:
Done, on both overloads: `@param protocolMetadataPrefix` with the separator
note, and `@since 4.0`.
##########
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:
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.
##########
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:
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.
##########
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:
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.
--
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]
