rzo1 commented on code in PR #2256:
URL: https://github.com/apache/stormcrawler/pull/2256#discussion_r4359201656
##########
external/tika/src/main/java/org/apache/stormcrawler/tika/ParserBolt.java:
##########
@@ -614,12 +644,15 @@ private PipesForkParser buildPipesForkParser(Map<String,
Object> conf) {
}
pipesConfig.setJvmArgs(forkedJvmArgs(conf));
- // an HtmlMapper can't be passed to the forked JVM, which always uses
Tika's default
- if (!DefaultHtmlMapper.class.equals(htmlMapperClass)) {
+ // Tika only sends the components registered in its index files to the
forked JVM,
+ // which falls back to its default mapper otherwise
+ if (IdentityHtmlMapper.class.equals(htmlMapperClass)) {
+ pipesHtmlMapper = IdentityHtmlMapper.INSTANCE;
Review Comment:
My local AI flagged this one, and I think it's valid:
> This changes which HtmlMapper the forked JVM uses. Before, it fell back to
Tika's `DefaultHtmlMapper`, which drops most attributes. Now it gets the
`IdentityHtmlMapper`, which keeps all of them, including ones with a prefix
like `xlink:href` (used by inline SVG icons, e.g. `<svg><use
xlink:href="#icon"></use></svg>`) or `x-on:click` (Alpine.js).
>
> The fork sends its result back as XML, written by `ToXMLContentHandler`.
That writer outputs `xlink:href="#icon"` as it is, but without declaring the
`xlink` namespace. On our side, `reparseIntoHandler` reads that XML back with a
namespace-aware SAX parser, which rejects an undeclared prefix with "prefix not
bound". The page is then reported as a parse error and nothing is emitted, not
even the text or the outlinks.
>
> To reproduce: set `parser.tika.timeout`, leave everything else at the
defaults (no jsoup filters needed), and parse `<html><body><svg><use
xlink:href="#i"></use></svg><a href="l.html">l</a></body></html>`. On main this
page parses fine and emits the outlink; with this PR it fails. In-process
parsing (no timeout) is not affected.
Only Pipes mode is affected, but inline SVG is everywhere, so we should fix
it before 4.0, here or in a follow-up. One option is to re-read the XML with a
non-namespace-aware parser and pass the qName as the local name. A Pipes test
with an `xlink:href` and an `x-on:click` attribute would cover it.
##########
external/tika/src/main/java/org/apache/stormcrawler/tika/ParserBolt.java:
##########
@@ -239,6 +247,8 @@ public void prepare(
parseFilters = ParseFilters.fromConf(conf);
+ jsoupFilters = JSoupFilters.fromConf(conf);
Review Comment:
With the archetype config, the jsoup filters now also run on PDFs and Office
files. An untitled PDF ends up with an empty `parse.title`, because Tika always
writes a `<title>` element and `XPathFilter` keeps the first match even if it's
blank, so the META fallback is never tried. HTML pages with an empty `<title>`
have the same problem today. I'd suggest skipping blank values in `XPathFilter`
(both the jsoup and the DOM one), so the next expression gets tried and empty
values never get stored. Probably better as a small separate PR. Not sure the
shared config key is what we want here, but fine either way.
##########
external/tika/src/main/java/org/apache/stormcrawler/tika/ParserBolt.java:
##########
@@ -731,6 +764,9 @@ private PipesParseOutcome parseWithPipes(
// the fork loads the "parse-context" section of the Tika
configuration itself
ParseContext parseContext = new ParseContext();
setPdfParserConfig(parseContext);
+ if (pipesHtmlMapper != null) {
+ parseContext.set(HtmlMapper.class, pipesHtmlMapper);
Review Comment:
If `parse-context.idx` isn't merged into the shaded jar (e.g. a project not
built from the archetype, whose shade config doesn't include it), every parse
in Pipes mode fails, because the fork can't deserialise the mapper we pass
here. `prepare()` doesn't notice, since `startFork()` runs its test parse
without the mapper. Could we make this fail-safe? For example, check in
`prepare()` whether `IdentityHtmlMapper` is registered
(`ComponentNameResolver.getFriendlyName(IdentityHtmlMapper.class) != null`,
worth testing it both with and without the idx file). If not, log a warning
that points to the `AppendingTransformer` for
`META-INF/tika/parse-context.idx`, and don't send the mapper, so the fork falls
back to `DefaultHtmlMapper` as before.
--
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]
