jnioche commented on PR #2231: URL: https://github.com/apache/stormcrawler/pull/2231#issuecomment-6105583644
Thanks @sean0407, this fixes what #2198 reports. I checked the three URLs from the issue and they now come back unchanged, including with `queryElementsToRemove`. The existing repairs (`http://google.com&d=4&good=true` → `http://google.com?d=4&good=true`, `http://dev.com/s&utax/NEWSRLSEfy18.pdf` left alone) work as before. A few things to look at before merging. ### 1. A `?` in the fragment still blocks the repair The new guard ignores everything after `#`, so that a `?` in the fragment doesn't prevent a real repair. But the rest of the method still works on the whole URL, fragment included, and the old `?` check on the last element still sees it. You only see this with `removeAnchorPart: false`; with the default config the fragment is already gone when `unmangleQueryString` is called. | input (`removeAnchorPart: false`) | output with this PR | expected | |---|---|---| | `http://a.example/p&a=b#frag` | `http://a.example/p?a=b#frag` | same ✔ | | `http://a.example/p&a=b#frag?x` | `http://a.example/p&a=b#frag?x` | `http://a.example/p?a=b#frag?x` | Splitting off the fragment once at the start, working on the part before it and putting it back at the end fixes this. It also makes the `?` check on the last element unnecessary. Something along these lines: ```java private String unmangleQueryString(String urlToFilter) { // the fragment is not part of the query: leave it out of the checks // and put it back as is final int hash = urlToFilter.indexOf('#'); final String url = hash == -1 ? urlToFilter : urlToFilter.substring(0, hash); if (url.indexOf('?') != -1) { // there is already a query return urlToFilter; } // only look at what follows the last slash, so that an & in an // earlier path segment is not mistaken for the start of a query final int lastSlash = url.lastIndexOf('/'); final String last = url.substring(lastSlash + 1); final int firstAmp = last.indexOf('&'); if (firstAmp == -1 || last.indexOf('=') <= 0) { return urlToFilter; } return url.substring(0, lastSlash + 1) + last.substring(0, firstAmp) + '?' + last.substring(firstAmp + 1) + (hash == -1 ? "" : urlToFilter.substring(hash)); } ``` ### 2. Tests The new test only uses an empty config. It would be worth adding: - **The third URL from the issue, with `queryElementsToRemove: ["utm_source"]`**, since that's where the bug did the most damage: | input | before this PR | with this PR | |---|---|---| | `http://a.example/login/?next=/a/b/&utm_source=ref&z=1` | `http://a.example/login/?next=%2Fa%2Fb%2F%3Futm_source%3Dref&z=1` (`utm_source` stuck inside `next`) | `http://a.example/login/?next=%2Fa%2Fb%2F&z=1` | - **The fragment case from point 1, with `removeAnchorPart: false`.** At the moment no test reaches the fragment branch of the new guard. ### Nits - The Javadoc of `unmangleQueryString` still only describes the `&` → `?` repair. It should say that a URL already containing a `?` before the fragment is left alone, and why only the last path segment is checked (so an `&` in an earlier segment isn't taken for the start of a query). -- 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]
