dpol1 commented on code in PR #2231:
URL: https://github.com/apache/stormcrawler/pull/2231#discussion_r4345878736
##########
core/src/main/java/org/apache/stormcrawler/filtering/basic/BasicURLNormalizer.java:
##########
@@ -333,23 +333,30 @@ public int compare(NameValuePair p1, NameValuePair p2) {
};
/**
- * A common error to find is a query string that starts with an & instead
of a ? This will fix
- * that error. So http://foo.com&a=b will be changed to http://foo.com?a=b.
+ * Repairs a query string that starts with an & instead of a ?. The query
check ignores the
+ * fragment, and the repair only checks the last path segment so an & in
an earlier path segment
+ * is not mistaken for a query separator.
*
* @param urlToFilter the url to filter
* @return corrected url
*/
private String unmangleQueryString(String urlToFilter) {
- String[] pathElements = urlToFilter.split("/");
+ int fragmentStart = urlToFilter.indexOf('#');
+ String fragment = fragmentStart == -1 ? "" :
urlToFilter.substring(fragmentStart);
+ String url = fragmentStart == -1 ? urlToFilter :
urlToFilter.substring(0, fragmentStart);
+ if (url.indexOf('?') != -1) {
+ return urlToFilter;
+ }
+
+ String[] pathElements = url.split("/");
Review Comment:
my agent found: with `removeAnchorPart` false,
`http://a.example/x&a=b/#frag` now becomes `http://a.example/x?a=b#frag` and
drops the slash, while main leaves it as is.
`url.split("/", -1)` fixes it and also leaves `http://a.example/x&a=b/`
unchanged with the default settings, so could you cover both in the tests?
--
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]
