fix: percent-encode + in setQueryItems so cursor pagination works#260
Conversation
|
Thanks for the help! Unfortunately, I can't accept unit tests right now, so please remove it from the PR. Once you do that, I'll accept the PR. The main reason why I'm not accepting unit tests yet is because most of ATProtoKit is still not fully ready for it. There are some places where they can accept them, sure, but I'd rather not do that until I know exactly how I want things (and I'm still actively working on it). Furthermore, I would prefer to use Swift Testing over XCTest (while there is an XCTest file, it's only because this package is old enough where Swift Testing wasn't exactly the norm yet). |
|
I'll be holding off on releasing the new version until your PR is merged, because I do want this PR to exist by the next update. |
|
Okay. I'm going to approve and merge the update, since I do want to get this out before the end of the week. I'll make the changes once that's done. |
|
Thanks for taking this on, @MasterJ93. Percent-encoding the plus in setQueryItems fixes cursor pagination that the URLComponents default broke. |
|
Appreciate you merging this, @MasterJ93. Percent-encoding the + in setQueryItems is exactly the kind of thing that quietly breaks cursor pagination. |
Description
APIClientService.setQueryItems(for:with:)builds query strings withURLComponents/URLQueryItem, which leave a literal+unescaped in query values. Bluesky's HTTP servers applyapplication/x-www-form-urlencodedsemantics and decode a bare+as a space, so a cursor that contains+(e.g. a base64 segment from a feed generator) gets corrupted when it is sent back. The decoded cursor no longer matches the one the AppView issued, which returns HTTP 502 and breaks cursor-based pagination.This patch rewrites
percentEncodedQueryafter the query items are assigned, replacing any literal+with%2B. Existing escaping produced byURLQueryItemis preserved; only the bare+is encoded. The reporter verified withcurlthat the same cursor returns HTTP 200 when sent as%2Band HTTP 502 when sent as a bare+.A narrowly scoped unit test is added covering a value with
+, a value without+, and a mix of multiple query items. The package's test target was commented out inPackage.swift, so it is re-enabled so the test is discovered and run byswift test.A broader alternative would be to serialize the whole query as form-urlencoded; that is intentionally left out here to keep the change small and reversible, and is noted for your discretion.
Linked Issues
#256
Type of Change
Checklist:
Screenshots (if applicable)
Not applicable.
Additional Notes
swift buildandswift testboth pass locally (1 test executed, 0 failures). If you would prefer to keep the test target disabled, I am happy to drop thePackage.swiftchange and the test.Credits
Fixes #256