Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KTOR-326 Allow to set followRedirect property for js client engine #3053

Merged
merged 1 commit into from
Jun 17, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import kotlinx.coroutines.*
import kotlin.coroutines.*

internal val CALL_COROUTINE = CoroutineName("call-context")
internal val CLIENT_CONFIG = AttributeKey<HttpClientConfig<*>>("client-config")

/**
* Base interface use to define engines for [HttpClient].
Expand Down Expand Up @@ -59,7 +60,10 @@ public interface HttpClientEngine : CoroutineScope, Closeable {

client.monitor.raise(HttpRequestIsReadyForSending, builder)

val requestData = builder.build()
val requestData = builder.build().apply {
attributes.put(CLIENT_CONFIG, client.config)
}

validateHeaders(requestData)
checkExtensions(requestData)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ import org.w3c.dom.*
import org.w3c.dom.events.*
import kotlin.coroutines.*

internal class JsClientEngine(override val config: HttpClientEngineConfig) : HttpClientEngineBase("ktor-js") {
internal class JsClientEngine(
override val config: HttpClientEngineConfig
) : HttpClientEngineBase("ktor-js") {

override val dispatcher = Dispatchers.Default

Expand All @@ -31,13 +33,14 @@ internal class JsClientEngine(override val config: HttpClientEngineConfig) : Htt
@OptIn(InternalAPI::class)
override suspend fun execute(data: HttpRequestData): HttpResponseData {
val callContext = callContext()
val clientConfig = data.attributes[CLIENT_CONFIG]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why pass it through data? there is a property config in class

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cause we need client config, not engine

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Let's create a ticket to pass client config in client factory HttpClientEngineFactory.create method in 3.0.0 for a proper fix.


if (data.isUpgradeRequest()) {
return executeWebSocketRequest(data, callContext)
}

val requestTime = GMTDate()
val rawRequest = data.toRaw(callContext)
val rawRequest = data.toRaw(clientConfig, callContext)
val rawResponse = commonFetch(data.url.toString(), rawRequest)

val status = HttpStatusCode(rawResponse.status.toInt(), rawResponse.statusText)
Expand All @@ -59,17 +62,19 @@ internal class JsClientEngine(override val config: HttpClientEngineConfig) : Htt
// Adding "_capturingHack" to reduce chances of JS IR backend to rename variable,
// so it can be accessed inside js("") function
@Suppress("UNUSED_PARAMETER", "UnsafeCastFromDynamic", "UNUSED_VARIABLE", "LocalVariableName")
private fun createWebSocket(urlString_capturingHack: String, headers: Headers): WebSocket =
if (PlatformUtils.IS_NODE) {
val ws_capturingHack = js("eval('require')('ws')")
val headers_capturingHack: dynamic = object {}
headers.forEach { name, values ->
headers_capturingHack[name] = values.joinToString(",")
}
js("new ws_capturingHack(urlString_capturingHack, { headers: headers_capturingHack })")
} else {
js("new WebSocket(urlString_capturingHack)")
private fun createWebSocket(
urlString_capturingHack: String,
headers: Headers
): WebSocket = if (PlatformUtils.IS_NODE) {
val ws_capturingHack = js("eval('require')('ws')")
val headers_capturingHack: dynamic = object {}
headers.forEach { name, values ->
headers_capturingHack[name] = values.joinToString(",")
}
js("new ws_capturingHack(urlString_capturingHack, { headers: headers_capturingHack })")
} else {
js("new WebSocket(urlString_capturingHack)")
}

private suspend fun executeWebSocketRequest(
request: HttpRequestData,
Expand Down Expand Up @@ -129,7 +134,7 @@ private fun Event.asString(): String = buildString {
append(JSON.stringify(this@asString, arrayOf("message", "target", "type", "isTrusted")))
}

private fun io.ktor.client.fetch.Headers.mapToKtor(): Headers = buildHeaders {
private fun org.w3c.fetch.Headers.mapToKtor(): Headers = buildHeaders {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this works on nodeJS? See also #2592

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, we replace some types, and the actual implementation stays the same.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are also fine

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit tests? AFAIK they does not work with nodeJS production, at least 1 year ago, which results into the same problem at runtime: #2385 so you needed to create a test project and use the artifact.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to set up a sample

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a sample for node: it works

this@mapToKtor.asDynamic().forEach { value: String, key: String ->
append(key, value)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

package io.ktor.client.engine.js

import io.ktor.client.*
import io.ktor.client.engine.*
import io.ktor.client.fetch.RequestInit
import io.ktor.client.request.*
Expand All @@ -17,7 +18,10 @@ import org.w3c.fetch.*
import kotlin.coroutines.*

@OptIn(InternalAPI::class, DelicateCoroutinesApi::class)
internal suspend fun HttpRequestData.toRaw(callContext: CoroutineContext): RequestInit {
internal suspend fun HttpRequestData.toRaw(
clientConfig: HttpClientConfig<*>,
callContext: CoroutineContext
): RequestInit {
val jsHeaders = js("({})")
mergeHeaders(this@toRaw.headers, this@toRaw.body) { key, value ->
jsHeaders[key] = value
Expand All @@ -37,7 +41,7 @@ internal suspend fun HttpRequestData.toRaw(callContext: CoroutineContext): Reque
return buildObject {
method = this@toRaw.method.value
headers = jsHeaders
redirect = RequestRedirect.FOLLOW
redirect = if (clientConfig.followRedirects) RequestRedirect.FOLLOW else RequestRedirect.MANUAL

bodyBytes?.let { body = Uint8Array(it.toTypedArray()) }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,7 @@

package io.ktor.client.engine.js

import kotlinx.coroutines.*
import org.khronos.webgl.*
import kotlin.coroutines.*
import kotlin.js.*

internal external interface ReadableStream {
public fun getReader(): ReadableStreamReader
}

internal external interface ReadResult {
val done: Boolean
val value: Uint8Array?
}

internal external interface ReadableStreamReader {
public fun cancel(reason: dynamic): Promise<dynamic>
public fun read(): Promise<ReadResult>
}

internal suspend fun ReadableStreamReader.readChunk(): Uint8Array? = suspendCancellableCoroutine { continuation ->
read().then {
val chunk = it.value
val result = if (it.done || chunk == null) null else chunk
continuation.resumeWith(Result.success(result))
}.catch { cause ->
continuation.resumeWithException(cause)
}
}

@Suppress("UnsafeCastFromDynamic")
internal fun Uint8Array.asByteArray(): ByteArray {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,23 @@
package io.ktor.client.engine.js.browser

import io.ktor.client.engine.js.*
import io.ktor.client.engine.js.ReadableStream
import io.ktor.client.fetch.*
import io.ktor.utils.io.*
import kotlinx.coroutines.*
import org.khronos.webgl.Uint8Array
import org.w3c.fetch.Response
import kotlin.coroutines.*

internal fun CoroutineScope.readBodyBrowser(response: Response): ByteReadChannel {
@Suppress("UNCHECKED_CAST_TO_EXTERNAL_INTERFACE")
val stream = response.body as? ReadableStream ?: error("Fail to obtain native stream: ${response.asDynamic()}")
val stream: ReadableStream<Uint8Array> = response.body ?: return ByteReadChannel.Empty
return channelFromStream(stream)
}

internal fun CoroutineScope.channelFromStream(
stream: ReadableStream
stream: ReadableStream<Uint8Array>
): ByteReadChannel = writer {
val reader = stream.getReader()
val reader: ReadableStreamDefaultReader<Uint8Array> = stream.getReader()
while (true) {
try {
val chunk = reader.readChunk() ?: break
Expand All @@ -30,3 +32,14 @@ internal fun CoroutineScope.channelFromStream(
}
}
}.channel

internal suspend fun ReadableStreamDefaultReader<Uint8Array>.readChunk(): Uint8Array? =
suspendCancellableCoroutine { continuation ->
read().then {
val chunk = it.value
val result = if (it.done || chunk == null) null else chunk
continuation.resumeWith(Result.success(result))
}.catch { cause ->
continuation.resumeWithException(cause)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@ import kotlin.js.Promise
internal suspend fun commonFetch(
input: String,
init: RequestInit
): Response = suspendCancellableCoroutine { continuation ->
): org.w3c.fetch.Response = suspendCancellableCoroutine { continuation ->
val controller = AbortController()
init.signal = controller.signal

continuation.invokeOnCancellation {
controller.abort()
}

val promise: Promise<Response> = if (PlatformUtils.IS_BROWSER) {
val promise: Promise<org.w3c.fetch.Response> = if (PlatformUtils.IS_BROWSER) {
fetch(input, init)
} else {
jsRequireNodeFetch()(input, init)
Expand All @@ -50,7 +50,7 @@ internal fun AbortController(): AbortController {
}

internal fun CoroutineScope.readBody(
response: Response
response: org.w3c.fetch.Response
): ByteReadChannel = if (PlatformUtils.IS_BROWSER) {
readBodyBrowser(response)
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,11 @@
package io.ktor.client.engine.js.node

import io.ktor.client.engine.js.*
import io.ktor.client.fetch.*
import io.ktor.utils.io.*
import kotlinx.coroutines.*
import kotlinx.coroutines.channels.*
import org.khronos.webgl.ArrayBuffer
import org.khronos.webgl.Uint8Array
import org.khronos.webgl.*
import org.w3c.fetch.*

internal fun CoroutineScope.readBodyNode(response: Response): ByteReadChannel = writer {
val body: dynamic = response.body ?: error("Fail to get body")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import kotlin.js.Promise

// external fun fetch(input: Request, init: RequestInit? = definedExternally): Promise<Response>

public external fun fetch(input: String, init: RequestInit? = definedExternally): Promise<Response>
public external fun fetch(input: String, init: RequestInit? = definedExternally): Promise<org.w3c.fetch.Response>

public external interface Request : Body {
/* "default" | "no-store" | "reload" | "no-cache" | "force-cache" | "only-if-cached" */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import io.ktor.client.request.*
import io.ktor.client.statement.*
import io.ktor.client.tests.utils.*
import io.ktor.http.*
import io.ktor.util.*
import kotlin.test.*

@Suppress("PublicApiImplicitType")
Expand All @@ -23,7 +24,7 @@ class HttpRedirectTest : ClientLoader() {
}

test { client ->
client.prepareGet("$TEST_URL_BASE").execute {
client.prepareGet(TEST_URL_BASE).execute {
assertEquals(HttpStatusCode.OK, it.status)
assertEquals("OK", it.bodyAsText())
}
Expand Down Expand Up @@ -132,4 +133,18 @@ class HttpRedirectTest : ClientLoader() {
}
}
}

@Test
fun testRedirectDisabled() = clientTests {
config {
followRedirects = false
}

test { client ->
if (PlatformUtils.IS_BROWSER) return@test

val response = client.get(TEST_URL_BASE)
assertEquals(HttpStatusCode.Found, response.status)
}
}
}