abhinav-phi commented on code in PR #2125:
URL: https://github.com/apache/stormcrawler/pull/2125#discussion_r3996892808
##########
core/src/main/java/org/apache/stormcrawler/bolt/SiteMapParserBolt.java:
##########
@@ -86,6 +86,21 @@ public class SiteMapParserBolt extends StatusEmitterBolt {
private int maxOffsetGuess = 300;
+ /**
+ * Whether a document without the {@code isSitemap} key is classified as a
sitemap by searching
+ * the first bytes for the sitemaps.org namespace. Any page that carries
the namespace string
+ * early enough is reclassified as a sitemap and never reaches the parser
bolt, so this defaults
+ * to false, like {@code feed.sniffContent} does for feeds.
+ */
+ private boolean sniffContent = false;
+
+ /**
+ * Whether the parser rejects documents which are not well formed
sitemaps. Strict parsing keeps
Review Comment:
Fixed in 86562814 — the javadoc now describes strict URL checking (host +
path scoping) and explicitly contrasts it with the namespace check; the yaml
comment and the PR description were aligned the same way.
##########
core/src/main/java/org/apache/stormcrawler/bolt/SiteMapParserBolt.java:
##########
@@ -103,22 +118,19 @@ public void execute(Tuple tuple) {
LOG.debug("Processing {}", url);
- boolean looksLikeSitemap = sniff(content);
- // can force the mimetype as we know it is XML
- if (looksLikeSitemap) {
+ String isSitemap = metadata.getFirstValue(isSitemapKey);
+
+ // only sniff when the operator asked for it: a page deciding how the
+ // pipeline treats it must not depend on a string in its body, and a
+ // sniffed document also needs a sitemap compatible content type
+ if (isSitemap == null && sniffContent && sniffsAsSitemap(ct, content))
{
Review Comment:
Fixed in 86562814 — the regression is gone: a document already declared via
`isSitemap=true` goes through `sniff(content)` as on main, so the content-type
override still applies; `sniffContent` gates only the classification of
undeclared documents. 26c7ea59 additionally parses the media type separately
from its parameters.
##########
core/src/main/java/org/apache/stormcrawler/bolt/SiteMapParserBolt.java:
##########
@@ -140,12 +152,21 @@ public void execute(Tuple tuple) {
// exception while parsing the sitemap
String errorMessage = "Exception while parsing " + url + ": " + e;
LOG.error(errorMessage);
- // send to status stream in case another component wants to update
- // its status
+ /*
+ * A document which does not parse as a sitemap is most likely an
+ * ordinary page whose persisted metadata carried isSitemap=true.
+ * Dropping the marking and emitting it as FETCH_ERROR keeps it
+ * schedulable: a terminal ERROR would remove it from the crawl for
+ * good when fetchInterval.error is negative, which lets whoever
+ * controls the content remove URLs from the corpus. The document
+ * goes on to the parser bolt on its next fetch, like any other
+ * page.
+ */
+ metadata.remove(isSitemapKey);
metadata.setValue(Constants.STATUS_ERROR_SOURCE, "sitemap
parsing");
metadata.setValue(Constants.STATUS_ERROR_MESSAGE, errorMessage);
collector.emit(
- Constants.StatusStreamName, tuple, new Values(url,
metadata, Status.ERROR));
+ Constants.StatusStreamName, tuple, new Values(url,
metadata, Status.FETCH_ERROR));
Review Comment:
Fixed — the formatter rewrote it; rat + build are green on b7565ea6 (run
34686066927).
##########
core/src/main/resources/crawler-default.yaml:
##########
@@ -275,6 +275,20 @@ config:
# filters URLs in sitemaps based on their modified Date (if any)
sitemap.filter.hours.since.modified: -1
+ # whether a document without the isSitemap key is classified as a sitemap
+ # by searching the first bytes of its content for the sitemaps.org
+ # namespace. Off by default: any page carrying the namespace string early
+ # enough would be reclassified as a sitemap and never reach the parser
+ # bolt. When enabled, a content type which rules a sitemap out (a page
+ # served as HTML) stops the sniffing.
+ sitemap.sniffContent: false
+
+ # whether the sitemap parser rejects documents which are not well formed
+ # sitemaps. Strict parsing also discards URLs a sitemap lists on hosts
+ # other than its own, and keeps an ordinary HTML page which mentions the
+ # sitemap namespace from being parsed leniently into half a sitemap.
+ sitemap.strict: true
Review Comment:
Changed in 86562814 — `sitemap.strict` now defaults to false (opt-in,
recommended for open crawls), so nothing shrinks silently. Note: the two
`stormcrawler.sitemap.extensions` fixtures still carry the same-origin `<loc>`
rewrite — with strict off they pass either way, so the edit is inert rather
than necessary. Happy to restore the originals verbatim if you prefer a zero
test-resource diff; I just did not want to spend another CI round-trip on a
cosmetic revert.
##########
core/src/main/java/org/apache/stormcrawler/bolt/SiteMapParserBolt.java:
##########
@@ -103,22 +121,25 @@ public void execute(Tuple tuple) {
LOG.debug("Processing {}", url);
- boolean looksLikeSitemap = sniff(content);
- // can force the mimetype as we know it is XML
- if (looksLikeSitemap) {
+ String isSitemap = metadata.getFirstValue(isSitemapKey);
+
+ // only promote an unmarked document when the operator asked for it: a
+ // page deciding how the pipeline treats it must not depend on a string
+ // in its body, and a promoted document also needs a sitemap compatible
+ // content type
+ if (isSitemap == null && sniffContent && sniffsAsSitemap(ct, content))
{
Review Comment:
Addressed in b7565ea6 — `FetcherBolt` now carries `isSitemap` across
redirects, so a `/sitemap.xml` → `/sitemap_index.xml` hop arrives with the key
set and is parsed as a sitemap without needing sniffing.
##########
core/src/main/resources/crawler-default.yaml:
##########
@@ -288,6 +288,23 @@ config:
# filters URLs in sitemaps based on their modified Date (if any)
sitemap.filter.hours.since.modified: -1
+ # whether a document without the isSitemap key is classified as a sitemap
+ # by searching the first bytes of its content for the sitemaps.org
+ # namespace. Off by default: any page carrying the namespace string early
+ # enough would be reclassified as a sitemap and never reach the parser
+ # bolt. When enabled, a content type which rules a sitemap out (a page
+ # served as HTML) stops the sniffing.
+ sitemap.sniffContent: false
+
+ # whether the sitemap parser applies strict URL checking: a sitemap then
+ # only yields URLs below its own host and path (strict URL checking of
+ # crawler-commons, not its namespace check), so a sitemap cannot enrol URLs
+ # on hosts it has nothing to do with. Off by default: a sitemap living at
Review Comment:
Right — addressed in b7565ea6: with strict on, index `<loc>` entries are
checked via `SiteMapParser.urlIsValid` against the index's own base (the same
directory derivation crawler-commons uses for a urlset) and skipped with a log
otherwise. `SiteMapParserBoltCrossHostTest` covers both sides: a sub-sitemap on
another host under an index is discarded, one under the same base is still
discovered.
--
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]
