Fix bug causing headers to be omitted when connecting to websocket using K/JS (nodejs) (KTOR-1629) #2307

DRSchlaubi · 2021-01-16T14:52:09Z

Subsystem
Websocket client on Kotlin/JS (nodejs)

Motivation
Because the browser websocket client does not support custom headers on the WS handshake the Ktor JS engine just ignores them completely however
the used NodeJS package (ws) supports setting custom headers

Issue: KTOR-1629

Solution
Change the JsClientEngine.createWebsocketFunction() to also acknowledge the headers on nodejs

- private fun createWebSocket(urlString_capturingHack: String): WebSocket {
+ private fun createWebSocket(urlString_capturingHack: String, headers: Headers): WebSocket {
        return if (PlatformUtils.IS_NODE) {
            val ws_capturingHack = js("require('ws')")
+            val headers_capturingHack: dynamic = object {}
+           headers.forEach { name, values ->
+                val value = values.first()
+                headers_capturingHack[name] = value
+           }
-            js("new ws_capturingHack(urlString_capturingHack)")
+            js("new ws_capturingHack(urlString_capturingHack, { headers: headers_capturingHack })")
        } else {
            js("new WebSocket(urlString_capturingHack)")
        }
    }

…ing K/JS (nodejs) (KTOR-1629)

e5l

Hey @DRSchlaubi, thanks for the PR.
Could you add a test that fails before and pass after the PR?

DRSchlaubi · 2021-01-19T09:24:31Z

Hey @DRSchlaubi, thanks for the PR.
Could you add a test that fails before and pass after the PR?

I don't really know-how. I thought I just copy the echo test, but echo.websocket.org does not send back the headers
So I thought to check whether the headers get added to DefaultWebsocketSession.call.request but it does that either way

e5l · 2021-01-19T14:33:10Z

Sure, you can add the test here: https://github.com/ktorio/ktor/blob/master/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/WebSocketTest.kt similar to other tests. And here is the test server: https://github.com/ktorio/ktor/blob/master/ktor-client/ktor-client-tests/jvm/src/io/ktor/client/tests/utils/tests/WebSockets.kt. It will start automatically on the test run.

For instance, you can set up a server handler to collect header fields from a request and send them back in a text frame(and verify them on the client-side for sure :) )

DRSchlaubi · 2021-01-19T14:34:30Z

this would work for the ws tests but what about the wss tests since the test server can't use https

e5l · 2021-01-19T14:50:21Z

Could you tell me why do we need wss to test headers content?

DRSchlaubi · 2021-01-19T14:53:13Z

It's just that we test the connection to ws and wss. I don't really think this matters though

e5l · 2021-01-19T15:07:53Z

Yep, let's just have a regular ws test

DRSchlaubi · 2021-01-19T17:40:49Z

I can't actually read the code style check so don't know what to change

EDIT: I finally found the login as a guest button to teamcity and could read the linter checks, but there seem to be a lot in files i didn't even change

ktor-client/ktor-client-core/js/src/io/ktor/client/engine/js/JsClientEngine.kt

ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/WebSocketTest.kt

ahardewig · 2021-02-12T17:05:28Z

Hey there, this issue seems to be affecting me too. A host exposes a WSS endpoint that needs a custom header to be sent in the request for validation. Despite being specified in the HttpHeaders, it's not propagated up to the actual request. Glad to see a fix is already being worked on! Is there anything I can do to help out?

DRSchlaubi · 2021-02-12T17:52:23Z

Actually this should be done already not sure if the lot team wants me to fix anything

hfhbd · 2021-02-17T11:40:42Z

@DRSchlaubi could you rebase your PR with the latest master, which refactored the JS engine?

ktor/ktor-client/ktor-client-core/js/src/io/ktor/client/engine/js/JsClientEngine.kt

Line 67 in e3b5f60

NodeWebsocket(urlString)

Eg:

// JsClientEngine.kt
val properties: dynamic = object { }
headers.forEach { name, values ->
    val value = values.joinToString(",")
    properties.headers[name] = value
}
NodeWebsocket(urlString, properties)

DRSchlaubi · 2021-02-17T11:47:44Z

Oh you now separated between nodeWebsocket and JS WebSocket, right? I*ll try to update the PR

# Conflicts: # ktor-client/ktor-client-core/js/src/io/ktor/client/engine/js/JsClientEngine.kt # ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/WebSocketTest.kt

DRSchlaubi · 2021-02-18T06:45:22Z

I couldn't run the tests after merging in master. I let them run for about an hour and Intellij still displayed "instanceiating tests"

hfhbd · 2021-02-18T09:04:54Z

@DRSchlaubi Did you use ./gradlew allTests using the terminal or IntelliJ?

DRSchlaubi · 2021-02-18T10:40:19Z

I used Intellij. Will try cli later. Wouldn't be the first time that K/JS test don't work in Intellij

DRSchlaubi · 2021-02-18T10:41:22Z

@DRSchlaubi Did you use ./gradlew allTests using the terminal or IntelliJ?

That is not even one of my tests or related to code I changed

hfhbd · 2021-02-18T10:43:10Z

@DRSchlaubi Did you use ./gradlew allTests using the terminal or IntelliJ?

That is not even one of my tests or related to code I changed

I know, the screenshot shows not your test. It is only an example.

ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/WebSocketTest.kt

DRSchlaubi · 2021-02-18T13:20:25Z

Also Intellij doesn't like to import ktor properly

DRSchlaubi · 2021-02-18T13:40:12Z

Yeah now i am stuck with this again

[TestServer] start
<========-----> 64% EXECUTING [15m 47s]
> IDLE
> IDLE
> IDLE
> IDLE
> IDLE
> :kotlinNpmInstall > [4/4] Building fresh packages...
> IDLE
> IDLE
> IDLE

DRSchlaubi · 2021-02-18T14:23:53Z

Tests are stuck at downloading yarn deps

hfhbd · 2021-02-19T10:50:50Z

ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/WebSocketTest.kt

+                assertEquals(CUSTOM_HEADER_VALUE, header)
+            }
+        }
+    }


Compile error, this bracket is false

hfhbd · 2021-02-19T10:50:58Z

ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/WebSocketTest.kt

+                assertEquals(listOf(CUSTOM_HEADER_VALUE, CUSTOM_HEADER_VALUE), header)
+            }
+        }
+    }


Compile error, this bracket is false

hfhbd · 2021-02-19T10:52:26Z

ktor-client/ktor-client-core/js/src/io/ktor/client/engine/js/node/NodeDefinitions.kt

@@ -7,7 +7,7 @@ package io.ktor.client.engine.js.node
 import io.ktor.client.engine.js.compatibility.*
 import org.w3c.dom.*
 import org.w3c.fetch.*
-import kotlin.js.*
+import kotlin.js.Promise


code style, use star import

ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/WebSocketTest.kt

cy6erGn0m · 2021-03-04T07:46:22Z

@DRSchlaubi could you please rebase the branch? Is it ready for review at the moment?

DRSchlaubi · 2021-03-04T15:55:03Z

I can't import ktor into intellij due to the error above

# Conflicts: # ktor-client/ktor-client-core/js/src/io/ktor/client/engine/js/JsClientEngine.kt # ktor-client/ktor-client-core/js/src/io/ktor/client/engine/js/node/NodeDefinitions.kt

DRSchlaubi · 2021-03-23T19:01:55Z

I rebased, but i couldn't run the tests as all tests give me an iOS error and core test complains about no module being specified

DRSchlaubi mentioned this pull request Jan 16, 2021

Status on multiplatform kordlib/Lavalink.kt#2

Open

Fix bug causing headers to be omitted when connecting to websocket us…

6bdf821

…ing K/JS (nodejs) (KTOR-1629)

DRSchlaubi force-pushed the bugfix/ws-headers-js branch from 63035b0 to 6bdf821 Compare January 16, 2021 14:57

Stexxe requested a review from e5l January 18, 2021 16:51

e5l requested changes Jan 19, 2021

View reviewed changes

Add unit test for websocket headers

cc258d9

hfhbd reviewed Jan 20, 2021

View reviewed changes

ktor-client/ktor-client-core/js/src/io/ktor/client/engine/js/JsClientEngine.kt Outdated Show resolved Hide resolved

hfhbd reviewed Jan 20, 2021

View reviewed changes

ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/WebSocketTest.kt Outdated Show resolved Hide resolved

DRSchlaubi added 2 commits January 20, 2021 16:56

Fix syntax error

d623400

Add support for multiple header values

975bb27

DRSchlaubi requested a review from e5l February 12, 2021 18:34

DRSchlaubi added 2 commits February 17, 2021 13:06

Merge branch 'master' into bugfix/ws-headers-js

82132fa

# Conflicts: # ktor-client/ktor-client-core/js/src/io/ktor/client/engine/js/JsClientEngine.kt # ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/WebSocketTest.kt

Update to new engine

5fe0516

Stexxe requested review from cy6erGn0m and removed request for e5l February 17, 2021 13:08

hfhbd reviewed Feb 18, 2021

View reviewed changes

ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/WebSocketTest.kt Show resolved Hide resolved

hfhbd reviewed Feb 18, 2021

View reviewed changes

ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/WebSocketTest.kt Show resolved Hide resolved

Return clientTests

8d3786a

hfhbd suggested changes Feb 19, 2021

View reviewed changes

hfhbd reviewed Feb 19, 2021

View reviewed changes

ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/WebSocketTest.kt Show resolved Hide resolved

Fix unit test

361b80f

hfhbd mentioned this pull request Mar 15, 2021

KTOR-2155 Split JS module to node and browser #2388

Closed

DRSchlaubi added 2 commits March 23, 2021 19:39

Merge remote-tracking branch 'upstream/master' into bugfix/ws-headers-js

26cc806

# Conflicts: # ktor-client/ktor-client-core/js/src/io/ktor/client/engine/js/JsClientEngine.kt # ktor-client/ktor-client-core/js/src/io/ktor/client/engine/js/node/NodeDefinitions.kt

Resolve merge conflicts

0614e44

Merge branch 'main' into bugfix/ws-headers-js

fe7d06e

e5l approved these changes Jun 15, 2021

View reviewed changes

e5l merged commit b40e900 into ktorio:main Jun 15, 2021

rsinukov mentioned this pull request Jun 15, 2021

KTOR-1629 Fix failing and not compiling tests #2517

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug causing headers to be omitted when connecting to websocket using K/JS (nodejs) (KTOR-1629) #2307

Fix bug causing headers to be omitted when connecting to websocket using K/JS (nodejs) (KTOR-1629) #2307

DRSchlaubi commented Jan 16, 2021 •

edited

Loading

e5l left a comment

DRSchlaubi commented Jan 19, 2021

e5l commented Jan 19, 2021

DRSchlaubi commented Jan 19, 2021

e5l commented Jan 19, 2021

DRSchlaubi commented Jan 19, 2021

e5l commented Jan 19, 2021

DRSchlaubi commented Jan 19, 2021 •

edited

Loading

ahardewig commented Feb 12, 2021 •

edited

Loading

DRSchlaubi commented Feb 12, 2021

hfhbd commented Feb 17, 2021 •

edited

Loading

DRSchlaubi commented Feb 17, 2021

DRSchlaubi commented Feb 18, 2021

hfhbd commented Feb 18, 2021 •

edited

Loading

DRSchlaubi commented Feb 18, 2021

DRSchlaubi commented Feb 18, 2021

hfhbd commented Feb 18, 2021

DRSchlaubi commented Feb 18, 2021

DRSchlaubi commented Feb 18, 2021

DRSchlaubi commented Feb 18, 2021

hfhbd Feb 19, 2021

hfhbd Feb 19, 2021

hfhbd Feb 19, 2021

cy6erGn0m commented Mar 4, 2021

DRSchlaubi commented Mar 4, 2021

DRSchlaubi commented Mar 23, 2021

Fix bug causing headers to be omitted when connecting to websocket using K/JS (nodejs) (KTOR-1629) #2307

Fix bug causing headers to be omitted when connecting to websocket using K/JS (nodejs) (KTOR-1629) #2307

Conversation

DRSchlaubi commented Jan 16, 2021 • edited Loading

e5l left a comment

Choose a reason for hiding this comment

DRSchlaubi commented Jan 19, 2021

e5l commented Jan 19, 2021

DRSchlaubi commented Jan 19, 2021

e5l commented Jan 19, 2021

DRSchlaubi commented Jan 19, 2021

e5l commented Jan 19, 2021

DRSchlaubi commented Jan 19, 2021 • edited Loading

ahardewig commented Feb 12, 2021 • edited Loading

DRSchlaubi commented Feb 12, 2021

hfhbd commented Feb 17, 2021 • edited Loading

DRSchlaubi commented Feb 17, 2021

DRSchlaubi commented Feb 18, 2021

hfhbd commented Feb 18, 2021 • edited Loading

DRSchlaubi commented Feb 18, 2021

DRSchlaubi commented Feb 18, 2021

hfhbd commented Feb 18, 2021

DRSchlaubi commented Feb 18, 2021

DRSchlaubi commented Feb 18, 2021

DRSchlaubi commented Feb 18, 2021

hfhbd Feb 19, 2021

Choose a reason for hiding this comment

hfhbd Feb 19, 2021

Choose a reason for hiding this comment

hfhbd Feb 19, 2021

Choose a reason for hiding this comment

cy6erGn0m commented Mar 4, 2021

DRSchlaubi commented Mar 4, 2021

DRSchlaubi commented Mar 23, 2021

DRSchlaubi commented Jan 16, 2021 •

edited

Loading

DRSchlaubi commented Jan 19, 2021 •

edited

Loading

ahardewig commented Feb 12, 2021 •

edited

Loading

hfhbd commented Feb 17, 2021 •

edited

Loading

hfhbd commented Feb 18, 2021 •

edited

Loading