jnioche commented on PR #2236:
URL: https://github.com/apache/stormcrawler/pull/2236#issuecomment-6143244095
Thanks for this, the migration looks solid. Here are some AI assisted
findings
I checked HttpClient 4 and HttpClient 5 against each other with the actual
jars:
- Query parsing and formatting with `'&', ';'` give identical output on a
set of tricky queries, so normalised URLs don't change.
- All 20 `HttpHeaders`/`HttpStatus` constants used in core, tika and warc
have the same string values.
- `CookieConverterTest` (61) and `BasicURLNormalizerTest` (21) pass.
- Dependency trees: compared with the base commit, the only changes are the
expected ones.
A few comments, most important first:
**1. `BasicURLNormalizer`: pin the format encoding with a test**
The whole `org.apache.hc.core5.net.URLEncodedUtils` class is deprecated in
httpcore5 5.4. The code depends on it for `parse` and also for `format`. The
comment only mentions that `WWWFormCodec` doesn't treat `;` as a separator, but
`WWWFormCodec.format` also encodes differently: it leaves `~` as is and encodes
`*` as `%2A`. For `a=~!'()*`, `URLEncodedUtils.format` gives
`a=%7E%21%27%28%29*` and `WWWFormCodec` gives `a=~%21%27%28%29%2A`. If someone
later "fixes" the deprecation warning, normalised URLs change silently and we
get duplicates in the status index. Could you add a test with `?a=~!'()*` and
extend the comment to say that `format` has to stay as well?
**2. `CookieConverter.getCookies`: public API still exposes an HttpClient
type**
We're breaking the API anyway, but the change swaps one HttpClient `Cookie`
for another. The only caller (okhttp `HttpProtocol`) uses just
`getName()`/`getValue()`. Returning a StormCrawler-owned type, or
`java.net.HttpCookie`, would decouple us from HttpComponents. Core could then
drop `httpclient5` and keep only `httpcore5` for the constants. Otherwise the
next HttpComponents major upgrade breaks this API again. This could be a
follow-up, but 4.0.0 is the natural time to do it.
**3. Root `dependencyManagement` pins override module versions**
Pinning httpclient5/httpcore5/httpcore5-h2 at the root forces those versions
on every module, including opensearch-java (via opensearch-java 3.10.0) and aws
(via awssdk `apache5-client`). The versions happen to match today. When
Dependabot bumps one of those clients to a release that needs a newer HC5, the
pin will silently downgrade it, which risks a `NoSuchMethodError` at runtime.
**4. `CookieConverter.parseCookieExpiry`: trying HttpClient 5 first changes
some results**
The legacy fallback alone reproduces HttpClient 4's `DateUtils.parseDate`
exactly: same patterns, quote trimming, 2000 pivot and leniency. Because
`parseStandardDate` runs first, a few inputs now give different results:
- `Wed, 09 Jun 2021 10:18:14 PST` gives 17:18:14Z (HttpClient 5 applies
daylight saving time) instead of 18:18:14Z.
- `Wed, 09 Jun 21 10:18:14 GMT` gives 0021-06-09 instead of 0021-06-07.
`parseStandardDate` also never parses the asctime format (it has no time
zone, so it returns null), and every miss throws and catches three exceptions.
I'd suggest using only the legacy path, or documenting the difference.
**5. `parseCookieExpiry`: per-call allocations**
Every fallback call creates the patterns array and three `SimpleDateFormat`
instances, and it re-parses `Instant.parse("2000-01-01T00:00:00Z")` on each
loop iteration. With `http.use.cookies=true` this runs for every request.
HttpClient 4 cached its formatters per thread. Moving the patterns and the
pivot `Date` into static final fields would help.
**6. `pom.xml`: use version properties**
Every other managed dependency uses a `${...version}` property. httpcore5
and httpcore5-h2 must stay in lockstep, but with two literal `5.4.3` values
Dependabot will open separate PRs, which can leave them mismatched. A shared
`httpcore5.version` property, plus `httpclient5.version`, would keep them
together.
**7. `core/pom.xml`: unused `httpcore5-h2`**
`httpclient5` brings in `httpcore5-h2`, the HTTP/2 implementation, which
core never uses, so it ends up in every topology uber-jar. Consider excluding
it, or see point 2.
**8. README**
The "Migrating to 4.0.0" section sits in the build instructions of the
top-level README. Its second paragraph describes things that did *not* change,
so users have nothing to do about them. These notes probably belong in the
release notes or docs instead.
Minor: the PR checklist says the issue is referenced in the commit message,
but the commit message doesn't mention #2211.
--
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]
