Fix redirect handling in KtorImageFetcher (#859)#939
Conversation
LandscapistImage failed to load images whose URL performs a redirect, reporting `SendCountExceedException: Max send count 20 exceeded` even though only one redirect was involved. Two issues in KtorImageFetcher caused this: - No cookie store was installed. Some CDNs set a cookie on the first response and redirect to a target that requires it. Without retaining the cookie the target keeps redirecting, looping until the send-count limit is hit. Browsers retain cookies automatically, which is why the same URL loads in a browser. Install HttpCookies to match that. - maxSendCount was set to maxRedirects, conflating the total number of request dispatches (original + redirects + retries) with the redirect count. That both under-allowed redirects (off-by-one) and, before the setting existed, left Ktor's default of 20. Set it to maxRedirects + 1 so the original send plus maxRedirects hops are permitted. The shared client configuration is extracted into configureForImageLoading so create() and tests build an identical client. Adds MockEngine-based regression tests covering the cookie-gated redirect, the maxRedirects boundary, and failure beyond the limit.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThe Ktor client configuration in ChangesKtor redirect and cookie fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant KtorImageFetcher
participant HttpClient
participant Server
Client->>KtorImageFetcher: fetch(url)
KtorImageFetcher->>HttpClient: create() with configureForImageLoading
HttpClient->>Server: initial request
Server-->>HttpClient: redirect + Set-Cookie
HttpClient->>HttpClient: persist cookie (HttpCookies)
HttpClient->>Server: retry with cookie (maxSendCount = maxRedirects + 1)
Server-->>HttpClient: image bytes
HttpClient-->>KtorImageFetcher: FetchResult
KtorImageFetcher-->>Client: image bytes
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary
Fixes #859.
LandscapistImagefailed to load images whose URL performs a redirect, reportingSendCountExceedException: Max send count 20 exceededeven though only one redirect was involved. A browser loads the same URL fine.Two issues in
KtorImageFetchercaused this:install(HttpCookies)retains the cookie across redirect hops.maxSendCountsemantics. It was set tomaxRedirects, conflating the total number of request dispatches (original + redirects + retries) with the redirect count. That under-allowed redirects by one, and before that setting existed the client used Ktor's default of 20 (matching the reported error). It is nowmaxRedirects + 1, so the original send plusmaxRedirectshops are permitted.The shared client configuration is extracted into
configureForImageLoadingsocreate()and tests build an identical client regardless of engine.Tests
Adds
MockEngine-based regression tests inKtorImageFetcherTest:cookieGatedRedirectResolvesInsteadOfLooping— reproduces SendCountExceedException: Max send count 20 exceeded #859; a cookie-gated self-redirect now resolves to success.redirectChainWithinMaxRedirectsSucceeds— a chain withinmaxRedirectsis followed.redirectChainBeyondMaxRedirectsFails— a longer chain fails withSendCountExceedExceptioninstead of looping forever.All pass on
landscapist-core:desktopTest; native and wasm test targets compile.Notes
ktor-client-mockas acommonTestdependency.User-Agentis left unchanged; some servers gate on it, but it remains overridable viaNetworkConfig.userAgent.Summary by CodeRabbit
Bug Fixes
Tests