fix(android): scope BaseUrlInterceptor to placeholder host only
android / Build + lint + test (push) Failing after 1m41s
android / Build + lint + test (push) Failing after 1m41s
Discover suggestion artist images failed to load on Android while loading fine in the web client. Root cause: BaseUrlInterceptor unconditionally rewrote every outgoing request's scheme/host/port to AuthStore.baseUrl. That's correct for Minstrel-bound requests built with the http://placeholder.invalid sentinel (Retrofit's frozen baseUrl, every cover-URL builder, ServerImage's resolveServerUrl). But Lidarr surfaces artist artwork as absolute URLs to external hosts (artwork.musicbrainz.org, coverartarchive.org); rewriting those to the Minstrel host produced 404s that Coil silently fell back from to the User icon. Web works because the browser fetches the URL as authored. Coil on Android shares the OkHttp client (and so the interceptor chain) with Retrofit, which is why the bug surfaced here only. Add a PLACEHOLDER_HOST companion constant and short-circuit the rewrite for non-placeholder hosts. Test coverage: - placeholder host → rewritten to live baseUrl - absolute external URL → host/scheme/path preserved - unparseable baseUrl → falls through (no throw) AuthCookieInterceptor still attaches the Minstrel session cookie to external requests; external hosts ignore unrecognized cookies so that's not breaking anything, but it's worth a follow-up DRY pass to scope auth attachment the same way.
This commit is contained in:
@@ -9,16 +9,23 @@ import javax.inject.Inject
|
|||||||
import javax.inject.Singleton
|
import javax.inject.Singleton
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Rewrites every outgoing request's scheme/host/port to match the
|
* Rewrites Minstrel-server requests' scheme/host/port to match the
|
||||||
* current [AuthStore.baseUrl]. Retrofit's `.baseUrl(...)` is read
|
* current [AuthStore.baseUrl]. Retrofit's `.baseUrl(...)` is read
|
||||||
* once at Retrofit creation, but AuthStore.baseUrl loads from Room
|
* once at Retrofit creation, but AuthStore.baseUrl loads from Room
|
||||||
* asynchronously — so at injection time the Retrofit instance is
|
* asynchronously — so at injection time the Retrofit instance is
|
||||||
* frozen pointing at the [AuthStore.DEFAULT_BASE_URL] placeholder.
|
* frozen pointing at the [AuthStore.DEFAULT_BASE_URL] placeholder.
|
||||||
*
|
*
|
||||||
* This interceptor closes the gap: Retrofit can stay built with the
|
* This interceptor closes the gap: Retrofit can stay built with the
|
||||||
* placeholder forever, and every actual request gets retargeted at
|
* placeholder forever, and every actual Minstrel-bound request gets
|
||||||
* the live AuthStore value. Lets the user change server URL in
|
* retargeted at the live AuthStore value. Lets the user change
|
||||||
* Settings without an app relaunch (Phase 11 wiring).
|
* server URL in Settings without an app relaunch (Phase 11 wiring).
|
||||||
|
*
|
||||||
|
* Only requests whose host is the [PLACEHOLDER_HOST] sentinel are
|
||||||
|
* rewritten. Absolute external URLs — Lidarr-surfaced artwork from
|
||||||
|
* artwork.musicbrainz.org / coverartarchive.org, for example — must
|
||||||
|
* reach their authored host unchanged; rewriting them to the
|
||||||
|
* Minstrel host produced 404s the user saw as missing Discover
|
||||||
|
* suggestion covers.
|
||||||
*
|
*
|
||||||
* No-op when the stored base URL is unparseable (falls through to
|
* No-op when the stored base URL is unparseable (falls through to
|
||||||
* whatever Retrofit had) — that case shows up as a transport-level
|
* whatever Retrofit had) — that case shows up as a transport-level
|
||||||
@@ -31,6 +38,7 @@ class BaseUrlInterceptor @Inject constructor(
|
|||||||
|
|
||||||
override fun intercept(chain: Interceptor.Chain): Response {
|
override fun intercept(chain: Interceptor.Chain): Response {
|
||||||
val original = chain.request()
|
val original = chain.request()
|
||||||
|
if (original.url.host != PLACEHOLDER_HOST) return chain.proceed(original)
|
||||||
val baseUrl = authStore.baseUrl.value.toHttpUrlOrNull()
|
val baseUrl = authStore.baseUrl.value.toHttpUrlOrNull()
|
||||||
?: return chain.proceed(original)
|
?: return chain.proceed(original)
|
||||||
val rewritten: HttpUrl = original.url.newBuilder()
|
val rewritten: HttpUrl = original.url.newBuilder()
|
||||||
@@ -40,4 +48,12 @@ class BaseUrlInterceptor @Inject constructor(
|
|||||||
.build()
|
.build()
|
||||||
return chain.proceed(original.newBuilder().url(rewritten).build())
|
return chain.proceed(original.newBuilder().url(rewritten).build())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
companion object {
|
||||||
|
/** Sentinel host used by Retrofit + every code path that builds
|
||||||
|
* a Minstrel-server URL via the `http://placeholder.invalid/...`
|
||||||
|
* form (see [com.fabledsword.minstrel.shared.resolveServerUrl],
|
||||||
|
* CoverUrls.kt, AlbumRef/TrackRef cover getters). */
|
||||||
|
const val PLACEHOLDER_HOST = "placeholder.invalid"
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -0,0 +1,130 @@
|
|||||||
|
package com.fabledsword.minstrel.api
|
||||||
|
|
||||||
|
import com.fabledsword.minstrel.auth.AuthStore
|
||||||
|
import com.fabledsword.minstrel.cache.db.dao.AuthSessionDao
|
||||||
|
import io.mockk.coEvery
|
||||||
|
import io.mockk.every
|
||||||
|
import io.mockk.mockk
|
||||||
|
import kotlinx.coroutines.ExperimentalCoroutinesApi
|
||||||
|
import kotlinx.coroutines.flow.flowOf
|
||||||
|
import kotlinx.coroutines.test.TestScope
|
||||||
|
import kotlinx.coroutines.test.UnconfinedTestDispatcher
|
||||||
|
import okhttp3.HttpUrl
|
||||||
|
import okhttp3.OkHttpClient
|
||||||
|
import okhttp3.Protocol
|
||||||
|
import okhttp3.Request
|
||||||
|
import okhttp3.Response
|
||||||
|
import okhttp3.ResponseBody.Companion.toResponseBody
|
||||||
|
import okhttp3.mockwebserver.MockResponse
|
||||||
|
import okhttp3.mockwebserver.MockWebServer
|
||||||
|
import org.junit.jupiter.api.AfterEach
|
||||||
|
import org.junit.jupiter.api.BeforeEach
|
||||||
|
import org.junit.jupiter.api.Test
|
||||||
|
import kotlin.test.assertEquals
|
||||||
|
|
||||||
|
@OptIn(ExperimentalCoroutinesApi::class)
|
||||||
|
class BaseUrlInterceptorTest {
|
||||||
|
private lateinit var server: MockWebServer
|
||||||
|
private lateinit var authStore: AuthStore
|
||||||
|
|
||||||
|
@BeforeEach
|
||||||
|
fun setup() {
|
||||||
|
server = MockWebServer().apply { start() }
|
||||||
|
val dao =
|
||||||
|
mockk<AuthSessionDao> {
|
||||||
|
every { observe() } returns flowOf(null)
|
||||||
|
coEvery { get() } returns null
|
||||||
|
coEvery { upsert(any()) } returns Unit
|
||||||
|
coEvery { setSessionCookie(any()) } returns Unit
|
||||||
|
coEvery { setBaseUrl(any()) } returns Unit
|
||||||
|
}
|
||||||
|
authStore = AuthStore(dao, TestScope(UnconfinedTestDispatcher()))
|
||||||
|
}
|
||||||
|
|
||||||
|
@AfterEach
|
||||||
|
fun teardown() {
|
||||||
|
server.shutdown()
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `rewrites placeholder host to live baseUrl host port and scheme`() {
|
||||||
|
// baseUrl points at the mock server; outbound request targets the
|
||||||
|
// placeholder sentinel — interceptor should rewrite to the mock.
|
||||||
|
authStore.setBaseUrl(server.url("/").toString())
|
||||||
|
server.enqueue(MockResponse().setResponseCode(200))
|
||||||
|
val client = OkHttpClient.Builder().addInterceptor(BaseUrlInterceptor(authStore)).build()
|
||||||
|
|
||||||
|
client.newCall(
|
||||||
|
Request.Builder().url("http://placeholder.invalid/api/test").build(),
|
||||||
|
).execute()
|
||||||
|
|
||||||
|
val recorded = server.takeRequest()
|
||||||
|
assertEquals("/api/test", recorded.path)
|
||||||
|
// MockWebServer's recorded Host header is the rewritten host:port
|
||||||
|
assertEquals("${server.hostName}:${server.port}", recorded.getHeader("Host"))
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `passes external absolute URL through unchanged`() {
|
||||||
|
// Lidarr-surfaced artwork hosts (artwork.musicbrainz.org,
|
||||||
|
// coverartarchive.org) are absolute external URLs. The
|
||||||
|
// interceptor must NOT rewrite them to the Minstrel base —
|
||||||
|
// doing so produces 404s the user saw as missing Discover
|
||||||
|
// suggestion covers.
|
||||||
|
authStore.setBaseUrl(server.url("/").toString())
|
||||||
|
var seen: HttpUrl? = null
|
||||||
|
val client =
|
||||||
|
OkHttpClient.Builder()
|
||||||
|
.addInterceptor(BaseUrlInterceptor(authStore))
|
||||||
|
.addInterceptor { chain ->
|
||||||
|
seen = chain.request().url
|
||||||
|
Response.Builder()
|
||||||
|
.request(chain.request())
|
||||||
|
.protocol(Protocol.HTTP_1_1)
|
||||||
|
.code(200)
|
||||||
|
.message("OK")
|
||||||
|
.body("".toResponseBody())
|
||||||
|
.build()
|
||||||
|
}
|
||||||
|
.build()
|
||||||
|
|
||||||
|
client.newCall(
|
||||||
|
Request.Builder()
|
||||||
|
.url("https://artwork.musicbrainz.org/release-group/abc/front-250.jpg")
|
||||||
|
.build(),
|
||||||
|
).execute()
|
||||||
|
|
||||||
|
assertEquals("artwork.musicbrainz.org", seen?.host)
|
||||||
|
assertEquals("https", seen?.scheme)
|
||||||
|
assertEquals("/release-group/abc/front-250.jpg", seen?.encodedPath)
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `falls through when stored baseUrl is unparseable`() {
|
||||||
|
// Empty / malformed baseUrl can happen between cold start and
|
||||||
|
// first AuthStore load; the interceptor should leave the
|
||||||
|
// request alone rather than throw.
|
||||||
|
authStore.setBaseUrl("not a url")
|
||||||
|
var seen: HttpUrl? = null
|
||||||
|
val client =
|
||||||
|
OkHttpClient.Builder()
|
||||||
|
.addInterceptor(BaseUrlInterceptor(authStore))
|
||||||
|
.addInterceptor { chain ->
|
||||||
|
seen = chain.request().url
|
||||||
|
Response.Builder()
|
||||||
|
.request(chain.request())
|
||||||
|
.protocol(Protocol.HTTP_1_1)
|
||||||
|
.code(200)
|
||||||
|
.message("OK")
|
||||||
|
.body("".toResponseBody())
|
||||||
|
.build()
|
||||||
|
}
|
||||||
|
.build()
|
||||||
|
|
||||||
|
client.newCall(
|
||||||
|
Request.Builder().url("http://placeholder.invalid/api/test").build(),
|
||||||
|
).execute()
|
||||||
|
|
||||||
|
assertEquals("placeholder.invalid", seen?.host)
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user