Do not collapse consecutive slashes when normalizing URLs

[anshuman83-40]

Fix #340

URL normalization rewrote any run of slashes in the path or querystring
as a single slash, so https://example.com/a///b became the ZIM path
example.com/a/b. a//b and a/b are different URLs which may serve
different content, so they are now kept as they are.

The same collapsing existed in the JS URL rewriting (wombatSetup.js),
which must produce the same ZIM paths as the Python normalization, so it
is removed there too.

Tests:

• Python: updated the three test_normalize cases that expected the
  collapsing, and added a///b and a//b?c=d//e cases.
• JS: added consecutiveSlashesAreKept in wombatUrlRewriting.js.

This only changes ZIM paths for URLs containing // after the host. As far
as I can tell existing ZIMs are not affected, since each ZIM ships the wombat
setup it was built with, but please correct me if that's wrong.

I could only run the Python tests locally; the JS test runs in CI.

[Sinkleberg]

There is still a mismatch in the Python rewrite path. From https://example.com/index.html, rewriting https://example.com/a///b now returns the ZIM key example.com/a///b but the link a/b: get_document_uri() passes the key through PurePosixPath, which collapses the slashes again. The link no longer points to the preserved key. I reproduced the same problem with repeated slashes in a query value; the base version's key and link matched.

Could you preserve empty path segments when generating the relative document URL and test the full rewrite result, not only normalize()? These were isolated Python checks, not an archive or browser test.

[Sinkleberg]

Thanks, the original cases and six added cases pass in my isolated checks. One edge remains: from https://example.com/index.html, the target https://example.com//b returns key example.com//b but link /b, which resolves at the reader's root. https://example.com///b produces //b, interpreted as another host.

Could you keep these links relative and add round-trip regressions for leading empty segments and an empty segment after the shared directory? Verified with the actual module and WHATWG URL resolution; no archive test.

[anshuman83-40]

Thanks for catching this, you're right! I only fixed the normalize() side and missed that get_document_uri() runs the path through PurePosixPath, which squashes the slashes again.

I pushed a fix: the relative link is now built segment by segment, so empty segments (the //) are kept. For paths without //, it gives exactly the same result as before. I also added tests that go through the full rewriter like you suggested, covering both the path and the query string cases, plus a page that itself has // in its URL.

One thing I noticed while testing: if a page's own URL contains //, a relative link inside it still gets collapsed, because Python's urljoin removes the empty segments when resolving it. That's outside this PR, but I can open a separate issue for it if you think it's worth fixing.

[Sinkleberg]

Yes, a separate issue makes sense for the urljoin behavior. I reproduced it on both the PR base and current head: other.html from https://example.com/x//y/page.html produces the key example.com/x/y/other.html, while WHATWG URL resolution preserves x//y.

The original fixes still pass. The leading-slash case from my latest review remains on the current head: the link /b resolves at the reader's root. That one still needs handling in this change. Checked with the actual modules and WHATWG URL resolution; no archive test.

[benoit74]

@anshuman83-40 please open follow-up issue indeed

Looks like @Sinkleberg edge case indeed still needs to be fixed. Please add it to the test cases

[anshuman83-40]

Fixed the leading-slash case: links that would start with / now get a ./ prefix, so example.com//b becomes .//b and ///b becomes .///b, both staying relative. I also added round-trip tests that resolve each rewritten link from the article the way a browser does (keeping empty segments, unlike urljoin) and check it gives back the ZIM path, including the cases @Sinkleberg mentioned.

Opened #346 for the urljoin case, thanks!

[Sinkleberg]

Thanks, the leading-slash cases now round-trip correctly in the isolated Python/WHATWG checks, and the original cases still pass. That resolves the link issue I raised here. Keeping the separate urljoin behavior in #346 makes sense. No archive test was run.

[anshuman83-40]

Thanks for checking it so carefully @Sinkleberg!

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (9b47195) to head (59ecb8f).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #344   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           42        42           
  Lines         2708      2720   +12     
  Branches       401       404    +3     
=========================================
+ Hits          2708      2720   +12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[benoit74]

Looks like JS side is not working: https://github.com/openzim/python-scraperlib/actions/runs/37477146077/job/115192717633?pr=344

[Sinkleberg]

The three failures are doubleSlash1, doubleSlash2 and doubleSlash3 in javascript/test/wombatUrlRewriting.js. Their expected URLs still collapse slashes: http%3A/example.com/some/path should now be http%3A//example.com//some/path. The actual JS results in the job log match Python normalization for all three inputs, and consecutiveSlashesAreKept passes. Updating those expectations should address these failures. Checked the CI log and Python outputs; I haven't rerun the JS suite locally.

[anshuman83-40]

Sorry about that, I missed three existing JS tests (doubleSlash1–3) that still expected the slashes to be collapsed, as @Sinkleberg also spotted. They're updated and CI is green now. I also merged main to resolve the CHANGELOG conflict with #347. Thanks for merging that one!

[Sinkleberg]

Confirmed, the three expectations are updated and all 140 JS tests pass in CI. That resolves the mismatch I flagged.

[benoit74]

Please squash your commits and rebase on main (we do not want merge commits which makes git history impossible to read)

[anshuman83-40]

Done, squashed into a single commit and rebased on main.

[benoit74]

LGTM