#315 Adaptative URL filter to normalize URLs based on canonical tag

[akash-manna-sky]

Adaptative URL filter to normalize URLs based on canonical tag
Fixes #315
Thank you for contributing to Apache StormCrawler.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes

• Is there a issue associated with this PR? Is it referenced in the commit message?

• Does your PR title start with #XXXX where XXXX is the issue number you are trying to resolve?

• Has your PR been rebased against the latest commit within the target branch (typically main)?

• Is your initial contribution a single, squashed commit?

• Is the code properly formatted with mvn git-code-format:format-code -Dgcf.globPattern="**/*" -Dskip.format.code=false?

For code changes

• Have you ensured that the full suite of tests is executed via mvn clean verify?
• Have you written or updated unit tests to verify your changes?
• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
• If applicable, have you updated the LICENSE file, including the main LICENSE file?
• If applicable, have you updated the NOTICE file, including the main NOTICE file?

Note

Please ensure that once the PR is submitted, you check GitHub Actions for build issues and submit an update to your PR as soon as possible.

[akash-manna-sky]

Hi @jnioche , please review the changes.

[jnioche]

thanks @akash-manna-sky

a quick AI assisted review

Thanks for tackling #315 — the PR hygiene here is good (formatting, unit tests, docs, issue linkage all present). Most of my comments are about how the filter interacts with the topol
ogy, which is hard to see without tracing through several bolts.

Blocker: the filter can't learn anything in a standard topology

learn() reads the canonical from sourceMetadata, but in both parsing bolts the URL filters are applied to the outlinks before the parse filters that produce the canonical:

• JSoupParserBolt: toOutlinks(url, metadata, slinks) (line 452) → filterOutlink → urlFilters.filter (line 604), whereas jsoupFilters.filter is line 464 and parseFilters.fil ter is line 478.
• external/tika ParserBolt: outlink filtering at line 406 vs parseFilters.filter at line 282.

canonical is extracted by XPathFilter, a parse filter. So at filtering time sourceMetadata.getFirstValue("canonical") is always null and learn() returns early. The unit tests
pass because they inject the metadata by hand.

The feature only does anything if the user adds canonical to metadata.persist (so it is learnt from the previous fetch), or places the filtering inside LinkParseFilter ordered
after the XPath filter — neither of which is documented. The docs hunk says the filter "must therefore be placed in a parsing bolt", which isn't sufficient.

This needs a design decision rather than a patch: either learn from somewhere that runs after the parse filters, or explicitly require canonical in metadata.persist and document t
hat rules are learnt one fetch cycle late.

Also structural

Thread safety. stats.get(k, ...) returns a plain HashMap that is mutated outside any lock, plus non-volatile int counters and lastLearnedSource. URLFilter instances *are

• used concurrently: StatusEmitterBolt.urlFilters is a single instance shared by all FetcherThreads, and FetcherBolt calls emitOutlink from those threads (lines 635 and 857).
  Every other built-in filter is stateless, so this contract has never been exercised. Two fetcher threads handling redirects for the same host can corrupt the scope map or lose counter
  increments. ConcurrentHashMap + AtomicInteger/LongAdder would cover it.

Non-deterministic normalisation. The learned evidence is per-instance, and URLFilters.fromConf builds a fresh instance per bolt (URLFilters.java:86) — one per parser task, one
per fetcher, one in URLFilterBolt (which calls filter(null, tempMed, url) and so never learns), plus a second one inside LinkParseFilter. urlfilters.config.file is global, so
there's no way to load the filter only in the parsing bolt as the docs suggest. The same outlink then gets emitted as ?id=1&sid=x by one task and ?id=1 by another, so both forms
land in the status store and both keep being fetched — the opposite of the intended dedup. URLs discovered before a rule was learnt are never reconciled either.

Content-bearing parameters can be learnt. Self-referencing canonicals are a common misconfiguration: plenty of sites serve /list?page=2..N with <link rel="canonical" href="/lis t">. sameResource() accepts that (same host/port/path), so after minObservations such pages page hits a 1.0 drop ratio and becomes removable. Since the rule is applied to outli
nks before they're emitted as DISCOVERED, ?page=2..N are all rewritten to /list, deduplicated away and never fetched — the crawl silently loses all paginated content, with no evid
ence decay and no recovery path. A denylist of protected parameter names (page, p, offset, start, q, …), or requiring the canonical to drop the parameter across several dist
inct paths, would limit the blast radius.

Smaller things

• minObservations counts observations, not distinct pages. lastLearnedSource only suppresses consecutive re-observations of the same source. With fetchInterval.default at
  1440 min, a host whose only query-string URL is /x?sid=1 accumulates dropped=5, total=5 over five days and the rule is applied on the strength of one page. The docs say "number of
  pages a parameter must have been seen on" — nothing tracks distinct source URLs.
• URL reconstruction changes unrelated parts, and only when something was removed. url.getPath() is "" for http://example.com?sid=1&id=2, so the result is http://example.co m?id=2 — a different status-store key from the http://example.com/?id=2 that BasicURLNormaliser produces. Similarly, if URLUtil.toURL had to sanitize the input (space, |, \
  , {, }), the sanitized form is returned, so /a b?sid=1&id=2 → /a%20b?out a removable parameter is returned verbatim. Appending /` for an empty pat
  h and rebuilding only the query portion of the original string would avoid both.
• maxParams has no eviction. The size check counts every parameter evevant ones, and entries are never removed. A site with per-page tokens in the p
  arameter name (cache-busters, utm_term_<hash>, CMS facets like f[123]=) fills the 100 slots with single-observation entries early in the crawl, after which the genuinely removab
  le parameter can never be tracked for that host. A size-bounded cache (Caffe or dropping single-observation entries when full, would be self-healing.
• testNumberOfTrackedSitesIsBounded doesn't exercise eviction. It sets maxScopes: 1 but only ever observes example.com, so the bound is never reached. Observing two distinct
  hosts and asserting the first host's evidence was discarded would actually

Things that do look correct: null handling, MalformedURLException handling correctly leaves mailto:/file: URLs alone), default-port comparison, and
the raw-vs-decoded parameter-name handling.

[akash-manna-sky]

Hi @jnioche, I addressed your concerns, please have a look.

[jnioche]

Thanks @akash-manna-sky

Summary
Genuinely good work. The design is sound, the failure modes are anticipated (self-referencing canonicals, per-page token params, memory bounds, thread safety), the docs are written to the project's standard, and the 47 tests pass and cover the real edge cases (ports, encoding, fragments, relative canonicals, concurrency).

Two blockers, a few design points worth settling before merge.

Blockers

1. Code is not formatted — CI will fail.

mvn git-code-format:validate-code-format fails. Running the formatter touches all four Java files (line-width and javadoc reflow, plus DEFAULT_PROTECTED_PARAMS gets exploded one-per-line). The checklist item claims this was done; it wasn't:

2. Two commits, not squashed, and 6 commits behind main. The checklist asks for a single squashed commit rebased on main.

Design points

3. store is complexity with no payoff. CanonicalRules.java:100 keys a static INSTANCES map by name, and all configuration comes from stormConf — so two stores in the same topology are necessarily identically configured and differ only in which evidence lands where. Meanwhile "the configuration of the first caller wins" is a genuine footgun in local mode / integration tests where several topologies share a JVM, and nothing ever removes entries (cleanup() isn't overridden).

I'd either drop store entirely, or put the config in the learner's params and have the URL filter name the learner. As it stands the store knob only buys a hazard.

While it's there: "default" is hard-coded in CanonicalParamLearner.java:60 but lives as package-private DEFAULT_STORE in AdaptiveURLNormalizer.java:47 — two literals that must agree. Move it to CanonicalRules as a public constant.

4. Default memory bounds are loose for a broad crawl. max.scopes: 10000 × max.params: 100 allows up to a million ParamStats, each holding two AtomicIntegers and a ConcurrentHashMap-backed set of full path strings. That's a few hundred MB of worker heap in the worst case, and the Caffeine maximumSize on scopes doesn't see it because it counts scopes, not params. Either put a weigher on scopes reflecting the param count, drop max.params to something like 20, or at minimum document the heap implication next to the setting.

5. "Rules are only ever added, never withdrawn" isn't quite true. The comment on promoteIfEstablished says promotions are final, and both javadoc and internals.adoc repeat it — but scopes is a size-bounded Caffeine cache, so an established scope can be evicted and its rules lost. testNumberOfTrackedSitesIsBounded acknowledges this while testAnEstablishedRuleIsNeverWithdrawn asserts the opposite invariant. Not a bug, but the docs should say "unless the host is evicted".