From cd6041c108063d1b98c9a7cbb10f9efbcb48dab3 Mon Sep 17 00:00:00 2001 From: Jay Ohms Date: Thu, 2 Jan 2025 12:26:00 -0500 Subject: [PATCH 1/3] Allow cross-origin redirects from link clicks to propose a new visit --- core/src/main/assets/js/turbo.js | 53 ++++++++++++++----- .../dev/hotwire/core/turbo/session/Session.kt | 46 +++++++++++++--- .../core/turbo/session/SessionCallback.kt | 1 + .../hotwire/core/turbo/session/SessionTest.kt | 13 ++++- .../fragments/HotwireWebFragmentDelegate.kt | 7 +++ 5 files changed, 98 insertions(+), 22 deletions(-) diff --git a/core/src/main/assets/js/turbo.js b/core/src/main/assets/js/turbo.js index e41ae8b..035a5bf 100644 --- a/core/src/main/assets/js/turbo.js +++ b/core/src/main/assets/js/turbo.js @@ -98,18 +98,18 @@ // Adapter interface visitProposedToLocation(location, options) { - if (window.Turbo && Turbo.navigator.locationWithActionIsSamePage(location, options.action)) { - // Scroll to the anchor on the page - TurboSession.visitProposalScrollingToAnchor(location.toString(), JSON.stringify(options)) - Turbo.navigator.view.scrollToAnchorFromLocation(location) - } else if (window.Turbo && Turbo.navigator.location?.href === location.href) { - // Refresh the page without native proposal - TurboSession.visitProposalRefreshingPage(location.toString(), JSON.stringify(options)) - this.visitLocationWithOptionsAndRestorationIdentifier(location, JSON.stringify(options), Turbo.navigator.restorationIdentifier) - } else { - // Propose the visit - TurboSession.visitProposedToLocation(location.toString(), JSON.stringify(options)) - } + if (window.Turbo && Turbo.navigator.locationWithActionIsSamePage(location, options.action)) { + // Scroll to the anchor on the page + TurboSession.visitProposalScrollingToAnchor(location.toString(), JSON.stringify(options)) + Turbo.navigator.view.scrollToAnchorFromLocation(location) + } else if (window.Turbo && Turbo.navigator.location?.href === location.href) { + // Refresh the page without native proposal + TurboSession.visitProposalRefreshingPage(location.toString(), JSON.stringify(options)) + this.visitLocationWithOptionsAndRestorationIdentifier(location, JSON.stringify(options), Turbo.navigator.restorationIdentifier) + } else { + // Propose the visit + TurboSession.visitProposedToLocation(location.toString(), JSON.stringify(options)) + } } // Turbolinks 5 @@ -134,8 +134,18 @@ this.loadResponseForVisitWithIdentifier(visit.identifier) } - visitRequestFailedWithStatusCode(visit, statusCode) { - TurboSession.visitRequestFailedWithStatusCode(visit.identifier, visit.hasCachedSnapshot(), statusCode) + async visitRequestFailedWithStatusCode(visit, statusCode) { + // Turbo does not permit cross-origin fetch redirect attempts and + // they'll lead to a visit request failure. Attempt to see if the + // visit request failure was due to a cross-origin redirect. + const redirect = await this.fetchFailedRequestCrossOriginRedirect(visit, statusCode) + const location = visit.location.toString() + + if (redirect != null) { + TurboSession.visitProposedToCrossOriginRedirect(location, redirect.toString(), visit.identifier) + } else { + TurboSession.visitRequestFailedWithStatusCode(location, visit.identifier, visit.hasCachedSnapshot(), statusCode) + } } visitRequestFinished(visit) { @@ -174,6 +184,21 @@ // Private + async fetchFailedRequestCrossOriginRedirect(visit, statusCode) { + // Non-HTTP status codes are sent by Turbo for network + // failures, including cross-origin fetch redirect attempts. + if (statusCode <= 0) { + try { + const response = await fetch(visit.location, { redirect: "follow" }) + if (response.url != null && response.url.origin != visit.location.origin) { + return response.url + } + } catch {} + } + + return null + } + afterNextRepaint(callback) { if (document.hidden) { callback() diff --git a/core/src/main/kotlin/dev/hotwire/core/turbo/session/Session.kt b/core/src/main/kotlin/dev/hotwire/core/turbo/session/Session.kt index 449e361..6e17079 100644 --- a/core/src/main/kotlin/dev/hotwire/core/turbo/session/Session.kt +++ b/core/src/main/kotlin/dev/hotwire/core/turbo/session/Session.kt @@ -14,9 +14,9 @@ import androidx.webkit.WebViewCompat import androidx.webkit.WebViewFeature.VISUAL_STATE_CALLBACK import androidx.webkit.WebViewFeature.isFeatureSupported import dev.hotwire.core.config.Hotwire -import dev.hotwire.core.logging.logEvent import dev.hotwire.core.files.delegates.FileChooserDelegate import dev.hotwire.core.files.delegates.GeolocationPermissionDelegate +import dev.hotwire.core.logging.logEvent import dev.hotwire.core.turbo.errors.HttpError import dev.hotwire.core.turbo.errors.LoadError import dev.hotwire.core.turbo.errors.WebError @@ -26,10 +26,10 @@ import dev.hotwire.core.turbo.offline.* import dev.hotwire.core.turbo.util.isHttpGetRequest import dev.hotwire.core.turbo.util.runOnUiThread import dev.hotwire.core.turbo.util.toJson -import dev.hotwire.core.turbo.webview.HotwireWebView import dev.hotwire.core.turbo.visit.Visit import dev.hotwire.core.turbo.visit.VisitAction import dev.hotwire.core.turbo.visit.VisitOptions +import dev.hotwire.core.turbo.webview.HotwireWebView import java.util.Date /** @@ -195,6 +195,33 @@ class Session( callback { it.visitProposedToLocation(location, options) } } + /** + * Called by Turbo bridge when a cross-origin redirect visit is proposed. + * + * Warning: This method is public so it can be used as a Javascript Interface. + * You should never call this directly as it could lead to unintended behavior. + * + * @param location The original visit location requested. + * @param redirectLocation The cross-origin redirect location. + * @param visitIdentifier A unique identifier for the visit. + */ + @JavascriptInterface + fun visitProposedToCrossOriginRedirect( + location: String, + redirectLocation: String, + visitIdentifier: String + ) { + logEvent("visitProposedToCrossOriginRedirect", + "location" to location, + "redirectLocation" to redirectLocation, + "visitIdentifier" to visitIdentifier + ) + + if (visitIdentifier == currentVisit?.identifier) { + callback { it.visitProposedToCrossOriginRedirect(redirectLocation) } + } + } + /** * Called by Turbo bridge when a new visit proposal will refresh the * current page. @@ -277,25 +304,30 @@ class Session( * Warning: This method is public so it can be used as a Javascript Interface. * You should never call this directly as it could lead to unintended behavior. * + * @param location The location of the failed visit. * @param visitIdentifier A unique identifier for the visit. * @param visitHasCachedSnapshot Whether the visit has a cached snapshot available. * @param statusCode The HTTP status code that caused the failure. */ @JavascriptInterface - fun visitRequestFailedWithStatusCode(visitIdentifier: String, visitHasCachedSnapshot: Boolean, statusCode: Int) { + fun visitRequestFailedWithStatusCode( + location: String, + visitIdentifier: String, + visitHasCachedSnapshot: Boolean, + statusCode: Int + ) { val visitError = HttpError.from(statusCode) logEvent( "visitRequestFailedWithStatusCode", + "location" to location, "visitIdentifier" to visitIdentifier, "visitHasCachedSnapshot" to visitHasCachedSnapshot, "error" to visitError ) - currentVisit?.let { visit -> - if (visitIdentifier == visit.identifier) { - callback { it.requestFailedWithError(visitHasCachedSnapshot, visitError) } - } + if (visitIdentifier == currentVisit?.identifier) { + callback { it.requestFailedWithError(visitHasCachedSnapshot, visitError) } } } diff --git a/core/src/main/kotlin/dev/hotwire/core/turbo/session/SessionCallback.kt b/core/src/main/kotlin/dev/hotwire/core/turbo/session/SessionCallback.kt index 2fd56fa..0b7b918 100644 --- a/core/src/main/kotlin/dev/hotwire/core/turbo/session/SessionCallback.kt +++ b/core/src/main/kotlin/dev/hotwire/core/turbo/session/SessionCallback.kt @@ -19,6 +19,7 @@ interface SessionCallback { fun visitCompleted(completedOffline: Boolean) fun visitLocationStarted(location: String) fun visitProposedToLocation(location: String, options: VisitOptions) + fun visitProposedToCrossOriginRedirect(location: String) fun visitDestination(): VisitDestination fun formSubmissionStarted(location: String) fun formSubmissionFinished(location: String) diff --git a/core/src/test/kotlin/dev/hotwire/core/turbo/session/SessionTest.kt b/core/src/test/kotlin/dev/hotwire/core/turbo/session/SessionTest.kt index 07e79d9..0a8819d 100644 --- a/core/src/test/kotlin/dev/hotwire/core/turbo/session/SessionTest.kt +++ b/core/src/test/kotlin/dev/hotwire/core/turbo/session/SessionTest.kt @@ -80,6 +80,17 @@ class SessionTest { verify(callback).visitProposedToLocation(newLocation, options) } + @Test + fun visitProposedToCrossOriginRedirectFiresCallback() { + val location = "${visit.location}/page" + val redirectLocation = "https://example.com/page" + + session.currentVisit = visit + session.visitProposedToCrossOriginRedirect(location, redirectLocation, visit.identifier) + + verify(callback).visitProposedToCrossOriginRedirect(redirectLocation) + } + @Test fun visitStartedSavesCurrentVisitIdentifier() { val visitIdentifier = "12345" @@ -110,7 +121,7 @@ class SessionTest { val visitIdentifier = "12345" session.currentVisit = visit.copy(identifier = visitIdentifier) - session.visitRequestFailedWithStatusCode(visitIdentifier, true, 500) + session.visitRequestFailedWithStatusCode(visit.location, visitIdentifier, true, 500) verify(callback).requestFailedWithError( visitHasCachedSnapshot = true, diff --git a/navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireWebFragmentDelegate.kt b/navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireWebFragmentDelegate.kt index b7e4101..d9fe87a 100644 --- a/navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireWebFragmentDelegate.kt +++ b/navigation-fragments/src/main/java/dev/hotwire/navigation/fragments/HotwireWebFragmentDelegate.kt @@ -243,6 +243,13 @@ internal class HotwireWebFragmentDelegate( navigator.route(location, options) } + override fun visitProposedToCrossOriginRedirect(location: String) { + // Pop the current destination from the backstack since it + // resulted in a visit failure due to a cross-origin redirect. + navigator.pop() + navigator.route(location) + } + override fun visitDestination(): VisitDestination { return this } From 9e17f8cf47dd86a7e63e4dcb5dc13ed9a40e2a02 Mon Sep 17 00:00:00 2001 From: Jay Ohms Date: Mon, 6 Jan 2025 17:36:11 -0500 Subject: [PATCH 2/3] Detect cross-origin visit redirects natively via OkHttp, instead of relying on javascript Fetch API, which has CORS limitations. --- core/src/main/assets/js/turbo.js | 29 +--- .../hotwire/core/turbo/http/HttpRepository.kt | 60 +++++++++ .../OfflineWebViewRequestInterceptor.kt | 2 +- .../dev/hotwire/core/turbo/session/Session.kt | 71 +++++++--- .../hotwire/core/turbo/BaseRepositoryTest.kt | 21 ++- .../config/PathConfigurationRepositoryTest.kt | 1 - .../hotwire/core/turbo/session/SessionTest.kt | 125 +++++++++++++----- .../resources/http-responses/empty-body.json | 1 + 8 files changed, 228 insertions(+), 82 deletions(-) create mode 100644 core/src/main/kotlin/dev/hotwire/core/turbo/http/HttpRepository.kt create mode 100644 core/src/test/resources/http-responses/empty-body.json diff --git a/core/src/main/assets/js/turbo.js b/core/src/main/assets/js/turbo.js index 035a5bf..44b7099 100644 --- a/core/src/main/assets/js/turbo.js +++ b/core/src/main/assets/js/turbo.js @@ -134,15 +134,15 @@ this.loadResponseForVisitWithIdentifier(visit.identifier) } - async visitRequestFailedWithStatusCode(visit, statusCode) { - // Turbo does not permit cross-origin fetch redirect attempts and - // they'll lead to a visit request failure. Attempt to see if the - // visit request failure was due to a cross-origin redirect. - const redirect = await this.fetchFailedRequestCrossOriginRedirect(visit, statusCode) + visitRequestFailedWithStatusCode(visit, statusCode) { const location = visit.location.toString() - if (redirect != null) { - TurboSession.visitProposedToCrossOriginRedirect(location, redirect.toString(), visit.identifier) + // Non-HTTP status codes are sent by Turbo for network failures, including + // cross-origin fetch redirect attempts. For non-HTTP status codes, pass to + // the native side to determine whether a cross-origin redirect visit should + // be proposed. + if (statusCode <= 0) { + TurboSession.visitRequestFailedWithNonHttpStatusCode(location, visit.identifier, visit.hasCachedSnapshot()) } else { TurboSession.visitRequestFailedWithStatusCode(location, visit.identifier, visit.hasCachedSnapshot(), statusCode) } @@ -184,21 +184,6 @@ // Private - async fetchFailedRequestCrossOriginRedirect(visit, statusCode) { - // Non-HTTP status codes are sent by Turbo for network - // failures, including cross-origin fetch redirect attempts. - if (statusCode <= 0) { - try { - const response = await fetch(visit.location, { redirect: "follow" }) - if (response.url != null && response.url.origin != visit.location.origin) { - return response.url - } - } catch {} - } - - return null - } - afterNextRepaint(callback) { if (document.hidden) { callback() diff --git a/core/src/main/kotlin/dev/hotwire/core/turbo/http/HttpRepository.kt b/core/src/main/kotlin/dev/hotwire/core/turbo/http/HttpRepository.kt new file mode 100644 index 0000000..239fa14 --- /dev/null +++ b/core/src/main/kotlin/dev/hotwire/core/turbo/http/HttpRepository.kt @@ -0,0 +1,60 @@ +package dev.hotwire.core.turbo.http + +import android.webkit.CookieManager +import dev.hotwire.core.logging.logError +import dev.hotwire.core.turbo.util.dispatcherProvider +import kotlinx.coroutines.withContext +import okhttp3.HttpUrl.Companion.toHttpUrl +import okhttp3.Request +import okhttp3.Response + +internal class HttpRepository { + private val cookieManager = CookieManager.getInstance() + + data class Result( + val response: Response, + val redirectToLocation: String?, + val redirectIsCrossOrigin: Boolean + ) + + suspend fun fetch(location: String): Result? { + return withContext(dispatcherProvider.io) { + val response = issueRequest(location) + + if (response != null) { + // Determine if there was a redirect, based on the final response's request url + val responseUrl = response.request.url + val isRedirect = location != responseUrl.toString() + val redirectIsCrossOrigin = isRedirect && location.toHttpUrl().host != responseUrl.host + + Result( + response = response, + redirectToLocation = if (isRedirect) responseUrl.toString() else null, + redirectIsCrossOrigin = redirectIsCrossOrigin + ) + } else { + null + } + } + } + + private fun issueRequest(location: String): Response? { + return try { + val request = buildRequest(location) + HotwireHttpClient.instance.newCall(request).execute() + } catch (e: Exception) { + logError("httpRequestError", e) + null + } + } + + private fun buildRequest(location: String): Request { + val builder = Request.Builder().url(location) + + cookieManager.getCookie(location)?.let { + builder.header("Cookie", it) + } + + return builder.build() + } +} diff --git a/core/src/main/kotlin/dev/hotwire/core/turbo/offline/OfflineWebViewRequestInterceptor.kt b/core/src/main/kotlin/dev/hotwire/core/turbo/offline/OfflineWebViewRequestInterceptor.kt index 3d159ea..fb27df9 100644 --- a/core/src/main/kotlin/dev/hotwire/core/turbo/offline/OfflineWebViewRequestInterceptor.kt +++ b/core/src/main/kotlin/dev/hotwire/core/turbo/offline/OfflineWebViewRequestInterceptor.kt @@ -9,7 +9,7 @@ import dev.hotwire.core.turbo.util.isHttpGetRequest internal class OfflineWebViewRequestInterceptor(val session: Session) { private val offlineRequestHandler get() = Hotwire.config.offlineRequestHandler - private val httpRepository get() = session.httpRepository + private val httpRepository get() = session.offlineHttpRepository private val currentVisit get() = session.currentVisit fun interceptRequest(request: WebResourceRequest): WebResourceResponse? { diff --git a/core/src/main/kotlin/dev/hotwire/core/turbo/session/Session.kt b/core/src/main/kotlin/dev/hotwire/core/turbo/session/Session.kt index 6e17079..afbcafa 100644 --- a/core/src/main/kotlin/dev/hotwire/core/turbo/session/Session.kt +++ b/core/src/main/kotlin/dev/hotwire/core/turbo/session/Session.kt @@ -22,6 +22,7 @@ import dev.hotwire.core.turbo.errors.LoadError import dev.hotwire.core.turbo.errors.WebError import dev.hotwire.core.turbo.errors.WebSslError import dev.hotwire.core.turbo.http.HotwireHttpClient +import dev.hotwire.core.turbo.http.HttpRepository import dev.hotwire.core.turbo.offline.* import dev.hotwire.core.turbo.util.isHttpGetRequest import dev.hotwire.core.turbo.util.runOnUiThread @@ -30,6 +31,7 @@ import dev.hotwire.core.turbo.visit.Visit import dev.hotwire.core.turbo.visit.VisitAction import dev.hotwire.core.turbo.visit.VisitOptions import dev.hotwire.core.turbo.webview.HotwireWebView +import kotlinx.coroutines.launch import java.util.Date /** @@ -52,8 +54,9 @@ class Session( internal var visitPending = false internal var restorationIdentifiers = SparseArray() internal val context: Context = activity.applicationContext - internal val httpRepository = OfflineHttpRepository(activity.lifecycleScope) - internal val requestInterceptor = OfflineWebViewRequestInterceptor(this) + internal val httpRepository = HttpRepository() + internal val offlineHttpRepository = OfflineHttpRepository(activity.lifecycleScope) + internal val offlineRequestInterceptor = OfflineWebViewRequestInterceptor(this) // User accessible @@ -106,7 +109,7 @@ class Session( "An offline request handler must be provided to pre-cache $location" } - httpRepository.preCache( + offlineHttpRepository.preCache( requestHandler, OfflinePreCacheRequest( url = location, userAgent = webView.settings.userAgentString ) @@ -195,18 +198,7 @@ class Session( callback { it.visitProposedToLocation(location, options) } } - /** - * Called by Turbo bridge when a cross-origin redirect visit is proposed. - * - * Warning: This method is public so it can be used as a Javascript Interface. - * You should never call this directly as it could lead to unintended behavior. - * - * @param location The original visit location requested. - * @param redirectLocation The cross-origin redirect location. - * @param visitIdentifier A unique identifier for the visit. - */ - @JavascriptInterface - fun visitProposedToCrossOriginRedirect( + private fun visitProposedToCrossOriginRedirect( location: String, redirectLocation: String, visitIdentifier: String @@ -331,6 +323,53 @@ class Session( } } + /** + * Called by Turbo bridge when a visit request fails with a non-HTTP status code, suggesting + * it may be the result of a cross-origin redirect visit. Determining a cross-origin redirect + * is not possible in javascript with the Fetch API due to CORS restrictions, so verify on + * the native side. Propose a cross-origin redirect visit if a redirect is found, otherwise + * fail the visit. + * + * Warning: This method is public so it can be used as a Javascript Interface. + * You should never call this directly as it could lead to unintended behavior. + * + * @param location The original visit location requested. + * @param visitIdentifier A unique identifier for the visit. + * @param visitHasCachedSnapshot Whether the visit has a cached snapshot available. + */ + @JavascriptInterface + fun visitRequestFailedWithNonHttpStatusCode( + location: String, + visitIdentifier: String, + visitHasCachedSnapshot: Boolean + ) { + logEvent("visitRequestFailedWithNonHttpStatusCode", + "location" to location, + "visitIdentifier" to visitIdentifier, + "visitHasCachedSnapshot" to visitHasCachedSnapshot + ) + + activity.lifecycleScope.launch { + val result = httpRepository.fetch(location) + + if (result != null && result.response.isSuccessful && + result.redirectToLocation != null && result.redirectIsCrossOrigin) { + visitProposedToCrossOriginRedirect( + location = location, + redirectLocation = result.redirectToLocation, + visitIdentifier = visitIdentifier + ) + } else { + visitRequestFailedWithStatusCode( + location = location, + visitIdentifier = visitIdentifier, + visitHasCachedSnapshot = visitHasCachedSnapshot, + statusCode = WebError.Unknown.errorCode + ) + } + } + } + /** * Called by Turbo bridge when the HTTP request has been completed. * @@ -781,7 +820,7 @@ class Session( } override fun shouldInterceptRequest(view: WebView, request: WebResourceRequest): WebResourceResponse? { - return requestInterceptor.interceptRequest(request) + return offlineRequestInterceptor.interceptRequest(request) } override fun onReceivedError(view: WebView, request: WebResourceRequest, error: WebResourceErrorCompat) { diff --git a/core/src/test/kotlin/dev/hotwire/core/turbo/BaseRepositoryTest.kt b/core/src/test/kotlin/dev/hotwire/core/turbo/BaseRepositoryTest.kt index 194fe7d..2a85de5 100644 --- a/core/src/test/kotlin/dev/hotwire/core/turbo/BaseRepositoryTest.kt +++ b/core/src/test/kotlin/dev/hotwire/core/turbo/BaseRepositoryTest.kt @@ -1,5 +1,6 @@ package dev.hotwire.core.turbo +import dev.hotwire.core.turbo.http.HotwireHttpClient import dev.hotwire.core.turbo.util.dispatcherProvider import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -26,6 +27,7 @@ open class BaseRepositoryTest : BaseUnitTest() { override fun setup() { super.setup() + HotwireHttpClient.instance = client() Dispatchers.setMain(testDispatcher) dispatcherProvider.io = Dispatchers.Main server.start() @@ -38,20 +40,19 @@ open class BaseRepositoryTest : BaseUnitTest() { server.shutdown() } - protected fun client(): OkHttpClient { - return OkHttpClient.Builder() - .dispatcher(Dispatcher(SynchronousExecutorService())) - .build() - } - protected fun baseUrl(): String { return server.url("/").toString() } - protected fun enqueueResponse(fileName: String, headers: Map = emptyMap()) { + protected fun enqueueResponse( + fileName: String, + responseCode: Int = 200, + headers: Map = emptyMap() + ) { val inputStream = loadAsset(fileName) val source = inputStream.source().buffer() val mockResponse = MockResponse().apply { + setResponseCode(responseCode) headers.forEach { addHeader(it.key, it.value) } setBody(source.readString(StandardCharsets.UTF_8)) } @@ -59,6 +60,12 @@ open class BaseRepositoryTest : BaseUnitTest() { server.enqueue(mockResponse) } + private fun client(): OkHttpClient { + return OkHttpClient.Builder() + .dispatcher(Dispatcher(SynchronousExecutorService())) + .build() + } + private fun loadAsset(fileName: String): InputStream { return javaClass.classLoader?.getResourceAsStream("http-responses/$fileName") ?: throw IllegalStateException("Couldn't load api response file") diff --git a/core/src/test/kotlin/dev/hotwire/core/turbo/config/PathConfigurationRepositoryTest.kt b/core/src/test/kotlin/dev/hotwire/core/turbo/config/PathConfigurationRepositoryTest.kt index 5e1b48d..dc3a8a1 100644 --- a/core/src/test/kotlin/dev/hotwire/core/turbo/config/PathConfigurationRepositoryTest.kt +++ b/core/src/test/kotlin/dev/hotwire/core/turbo/config/PathConfigurationRepositoryTest.kt @@ -27,7 +27,6 @@ class PathConfigurationRepositoryTest : BaseRepositoryTest() { override fun setup() { super.setup() context = ApplicationProvider.getApplicationContext() - HotwireHttpClient.instance = client() } @Test diff --git a/core/src/test/kotlin/dev/hotwire/core/turbo/session/SessionTest.kt b/core/src/test/kotlin/dev/hotwire/core/turbo/session/SessionTest.kt index 0a8819d..d9adc5e 100644 --- a/core/src/test/kotlin/dev/hotwire/core/turbo/session/SessionTest.kt +++ b/core/src/test/kotlin/dev/hotwire/core/turbo/session/SessionTest.kt @@ -1,18 +1,21 @@ package dev.hotwire.core.turbo.session import android.os.Build -import androidx.activity.result.ActivityResultLauncher import androidx.appcompat.app.AppCompatActivity import com.nhaarman.mockito_kotlin.never import com.nhaarman.mockito_kotlin.times import com.nhaarman.mockito_kotlin.whenever +import dev.hotwire.core.turbo.BaseRepositoryTest +import dev.hotwire.core.turbo.errors.HttpError import dev.hotwire.core.turbo.errors.HttpError.ServerError import dev.hotwire.core.turbo.errors.LoadError +import dev.hotwire.core.turbo.errors.WebError import dev.hotwire.core.turbo.util.toJson -import dev.hotwire.core.turbo.webview.HotwireWebView import dev.hotwire.core.turbo.visit.Visit import dev.hotwire.core.turbo.visit.VisitDestination import dev.hotwire.core.turbo.visit.VisitOptions +import dev.hotwire.core.turbo.webview.HotwireWebView +import kotlinx.coroutines.ExperimentalCoroutinesApi import org.assertj.core.api.Assertions.assertThat import org.junit.Before import org.junit.Test @@ -24,26 +27,31 @@ import org.robolectric.Robolectric.buildActivity import org.robolectric.RobolectricTestRunner import org.robolectric.annotation.Config +@ExperimentalCoroutinesApi @RunWith(RobolectricTestRunner::class) @Config(sdk = [Build.VERSION_CODES.R]) -class SessionTest { +class SessionTest : BaseRepositoryTest() { @Mock private lateinit var callback: SessionCallback + @Mock private lateinit var webView: HotwireWebView + @Mock private lateinit var activity: AppCompatActivity private lateinit var session: Session private lateinit var visit: Visit @Before - fun setup() { + override fun setup() { + super.setup() + MockitoAnnotations.openMocks(this) activity = buildActivity(TurboTestActivity::class.java).get() session = Session("test", activity, webView) visit = Visit( - location = "https://turbo.hotwired.dev", + location = baseUrl(), destinationIdentifier = 1, restoreWithCachedSnapshot = false, reload = false, @@ -62,7 +70,7 @@ class SessionTest { } @Test - fun getNewIsAlwaysNewInstance() { + fun `session is always new instance`() { val session = Session("test", activity, webView) val newSession = Session("test", activity, webView) @@ -70,7 +78,7 @@ class SessionTest { } @Test - fun visitProposedToLocationFiresCallback() { + fun `visit proposed to location fires callback`() { val options = VisitOptions() val newLocation = "${visit.location}/page" @@ -81,18 +89,7 @@ class SessionTest { } @Test - fun visitProposedToCrossOriginRedirectFiresCallback() { - val location = "${visit.location}/page" - val redirectLocation = "https://example.com/page" - - session.currentVisit = visit - session.visitProposedToCrossOriginRedirect(location, redirectLocation, visit.identifier) - - verify(callback).visitProposedToCrossOriginRedirect(redirectLocation) - } - - @Test - fun visitStartedSavesCurrentVisitIdentifier() { + fun `visit started saves current visit identifier`() { val visitIdentifier = "12345" session.currentVisit = visit.copy(identifier = visitIdentifier) @@ -100,14 +97,14 @@ class SessionTest { visitIdentifier = visitIdentifier, visitHasCachedSnapshot = true, visitIsPageRefresh = false, - location = "https://turbo.hotwired.dev" + location = visit.location ) assertThat(session.currentVisit?.identifier).isEqualTo(visitIdentifier) } @Test - fun visitFailedToLoadCallsAdapter() { + fun `visit failed to load calls adapter`() { val visitIdentifier = "12345" session.currentVisit = visit.copy(identifier = visitIdentifier) @@ -117,20 +114,78 @@ class SessionTest { } @Test - fun visitRequestFailedWithStatusCodeCallsAdapter() { + fun `visit request failed with status code calls adapter`() { val visitIdentifier = "12345" session.currentVisit = visit.copy(identifier = visitIdentifier) session.visitRequestFailedWithStatusCode(visit.location, visitIdentifier, true, 500) verify(callback).requestFailedWithError( - visitHasCachedSnapshot = true, + visitHasCachedSnapshot = true, error = ServerError.InternalServerError ) } @Test - fun visitCompletedCallsAdapter() { + fun `visit request failed with non http status code calls adapter for cross origin redirect`() { + val redirectLocation = "https://example.com/" + val visitIdentifier = "12345" + + enqueueResponse( + fileName = "empty-body.json", + responseCode = 301, + headers = mapOf("Location" to redirectLocation) + ) + + enqueueResponse( + fileName = "empty-body.json", + responseCode = 200 + ) + + session.currentVisit = visit.copy(identifier = visitIdentifier) + session.visitRequestFailedWithNonHttpStatusCode(visit.location, visitIdentifier, true) + + verify(callback).visitProposedToCrossOriginRedirect(redirectLocation) + } + + @Test + fun `visit request failed with non http status code calls adapter without redirect`() { + enqueueResponse( + fileName = "empty-body.json", + responseCode = 404 + ) + + val visitIdentifier = "12345" + + session.currentVisit = visit.copy(identifier = visitIdentifier) + session.visitRequestFailedWithNonHttpStatusCode(visit.location, visitIdentifier, true) + + verify(callback).requestFailedWithError( + visitHasCachedSnapshot = true, + error = HttpError.from(WebError.Unknown.errorCode) + ) + } + + @Test + fun `visit request failed with non http status code calls adapter without redirect fails visit`() { + enqueueResponse( + fileName = "empty-body.json", + responseCode = 200 + ) + + val visitIdentifier = "12345" + + session.currentVisit = visit.copy(identifier = visitIdentifier) + session.visitRequestFailedWithNonHttpStatusCode(visit.location, visitIdentifier, true) + + verify(callback).requestFailedWithError( + visitHasCachedSnapshot = true, + error = HttpError.from(WebError.Unknown.errorCode) + ) + } + + @Test + fun `visit completed calls adapter`() { val visitIdentifier = "12345" val restorationIdentifier = "67890" @@ -141,7 +196,7 @@ class SessionTest { } @Test - fun visitCompletedSavesRestorationIdentifier() { + fun `visit completed saves restoration identifier`() { val visitIdentifier = "12345" val restorationIdentifier = "67890" assertThat(session.restorationIdentifiers.size()).isEqualTo(0) @@ -153,7 +208,7 @@ class SessionTest { } @Test - fun visitFormSubmissionStartedFiresCallback() { + fun `visit form submission started fires callback`() { session.currentVisit = visit session.formSubmissionStarted(visit.location) @@ -161,7 +216,7 @@ class SessionTest { } @Test - fun visitFormSubmissionFinishedFiresCallback() { + fun `visit form submission finished fires callback`() { session.currentVisit = visit session.formSubmissionFinished(visit.location) @@ -169,7 +224,7 @@ class SessionTest { } @Test - fun pageLoadedSavesRestorationIdentifier() { + fun `page loaded saves restoration identifier`() { val restorationIdentifier = "67890" assertThat(session.restorationIdentifiers.size()).isEqualTo(0) @@ -180,7 +235,7 @@ class SessionTest { } @Test - fun pendingVisitIsVisitedWhenReady() { + fun `pending visit is visited when ready`() { session.currentVisit = visit session.visitPending = true @@ -189,7 +244,7 @@ class SessionTest { } @Test - fun resetToColdBoot() { + fun `reset to cold boot`() { session.currentVisit = visit session.isReady = true session.isColdBooting = false @@ -200,7 +255,7 @@ class SessionTest { } @Test - fun resetToColdBootClearsIdentifiers() { + fun `reset to cold boot clears identifiers`() { val visitIdentifier = "12345" session.currentVisit = visit.copy(identifier = visitIdentifier) session.coldBootVisitIdentifier = "0" @@ -211,7 +266,7 @@ class SessionTest { } @Test - fun restoreCurrentVisit() { + fun `restore current visit`() { val visitIdentifier = "12345" val restorationIdentifier = "67890" @@ -224,7 +279,7 @@ class SessionTest { } @Test - fun restoreCurrentVisitFailsWithNoRestorationIdentifier() { + fun `restore current visit fails with no restoration identifier`() { val visitIdentifier = "12345" session.currentVisit = visit.copy(identifier = visitIdentifier) @@ -235,7 +290,7 @@ class SessionTest { } @Test - fun restoreCurrentVisitFailsWithSessionNotReady() { + fun `restore current visit fails with session not ready`() { val visitIdentifier = "12345" val restorationIdentifier = "67890" @@ -249,7 +304,7 @@ class SessionTest { } @Test - fun webViewIsNotNull() { + fun `webView is not null`() { assertThat(session.webView).isNotNull } } diff --git a/core/src/test/resources/http-responses/empty-body.json b/core/src/test/resources/http-responses/empty-body.json new file mode 100644 index 0000000..0967ef4 --- /dev/null +++ b/core/src/test/resources/http-responses/empty-body.json @@ -0,0 +1 @@ +{} From 49a299c2297113e14a7052c36295a73a7f3760fe Mon Sep 17 00:00:00 2001 From: Jay Ohms Date: Wed, 8 Jan 2025 14:07:58 -0500 Subject: [PATCH 3/3] Rename the request result class and move the redirect properties to its own data class --- .../hotwire/core/turbo/http/HttpRepository.kt | 21 ++++++++++++------- .../dev/hotwire/core/turbo/session/Session.kt | 4 ++-- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/core/src/main/kotlin/dev/hotwire/core/turbo/http/HttpRepository.kt b/core/src/main/kotlin/dev/hotwire/core/turbo/http/HttpRepository.kt index 239fa14..c1310c0 100644 --- a/core/src/main/kotlin/dev/hotwire/core/turbo/http/HttpRepository.kt +++ b/core/src/main/kotlin/dev/hotwire/core/turbo/http/HttpRepository.kt @@ -11,13 +11,17 @@ import okhttp3.Response internal class HttpRepository { private val cookieManager = CookieManager.getInstance() - data class Result( + data class HttpRequestResult( val response: Response, - val redirectToLocation: String?, - val redirectIsCrossOrigin: Boolean + val redirect: HttpRedirect? ) - suspend fun fetch(location: String): Result? { + data class HttpRedirect( + val location: String, + val isCrossOrigin: Boolean + ) + + suspend fun fetch(location: String): HttpRequestResult? { return withContext(dispatcherProvider.io) { val response = issueRequest(location) @@ -25,12 +29,13 @@ internal class HttpRepository { // Determine if there was a redirect, based on the final response's request url val responseUrl = response.request.url val isRedirect = location != responseUrl.toString() - val redirectIsCrossOrigin = isRedirect && location.toHttpUrl().host != responseUrl.host - Result( + HttpRequestResult( response = response, - redirectToLocation = if (isRedirect) responseUrl.toString() else null, - redirectIsCrossOrigin = redirectIsCrossOrigin + redirect = if (!isRedirect) null else HttpRedirect( + location = responseUrl.toString(), + isCrossOrigin = location.toHttpUrl().host != responseUrl.host + ) ) } else { null diff --git a/core/src/main/kotlin/dev/hotwire/core/turbo/session/Session.kt b/core/src/main/kotlin/dev/hotwire/core/turbo/session/Session.kt index afbcafa..6cf8bd1 100644 --- a/core/src/main/kotlin/dev/hotwire/core/turbo/session/Session.kt +++ b/core/src/main/kotlin/dev/hotwire/core/turbo/session/Session.kt @@ -353,10 +353,10 @@ class Session( val result = httpRepository.fetch(location) if (result != null && result.response.isSuccessful && - result.redirectToLocation != null && result.redirectIsCrossOrigin) { + result.redirect?.isCrossOrigin == true) { visitProposedToCrossOriginRedirect( location = location, - redirectLocation = result.redirectToLocation, + redirectLocation = result.redirect.location, visitIdentifier = visitIdentifier ) } else {