jnioche commented on PR #2231:
URL: https://github.com/apache/stormcrawler/pull/2231#issuecomment-6384941586
thanks @sean0407 , a few suggestions below
- **`BasicURLNormalizer.java:344-346`** — the fragment and the URL are both
cut out on every call, even when the URL has no `&`. This code runs on every
outlink. Return early with `if (urlToFilter.indexOf('&') == -1) return
urlToFilter;` before any `substring`.
- **`BasicURLNormalizer.java:349-359`** — the `split`/`String.join` round
trip isn't needed:
- Use `int lastSlash = url.lastIndexOf('/')` to find the last segment
instead of building an array.
- `firstAmp` is already computed, so build the result with `substring(0,
firstAmp) + '?' + substring(firstAmp + 1)` instead of the regex-based
`replaceFirst("&", "?")`.
- **`BasicURLNormalizerTest.java:216-230`** — this test puts two hard-coded
inputs in one method. Make it a `@ParameterizedTest` so a failure names the
input that failed. Add the issue's third row
(`http://a.example/login/?next=/a/b/&utm_source=ref&z=1` with empty params,
expected unchanged), which is only tested through `utm_source` removal right
now.
- **`BasicURLNormalizerTest.java:243-263`** — the two trailing-slash tests
are the same test with a different config. Merge them into one parameterized
test. The message "A trailing slash before a fragment must not be mistaken for
a query" is unclear; say what is actually checked (the URL is left unchanged).
- The repair still works: the issue's `http://a.example/p&a=b&c=d` →
`http://a.example/p?a=b&c=d` case has no test. The existing tests only cover
the host-level case and `&` in an earlier segment.
- An `&` in the fragment is no longer rewritten: `http://a.example/p#a&b=c`
with `removeAnchorPart: false` should stay unchanged. The old code gave
`p#a?b=c`, and the new code fixes this, but no test pins it.
--
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]
