Add Scala.js support to sttp client

[sbrunk]

This builds on top of #815.

• Compile sttp-client to Scala.js.
• Make client tests async so we can run them on JS.
• Move client testserver into its own sbt subproject so we can use it from the ScalaTest JS runner.

This is still a draft because it relies on a published artifact of com.softwaremill.common:tagging for Scala.js 1 (right now it's only published for Scala.js 0.6). I could build this locally from the repo without any changes, so it really only needs publishing.

There are also still some failing tests. SttpClientMultipartTests are failing and a bunch of tests from SttpClientBasicTests are also failing with a weird stacktrace (if I comment these out, 32 tests pass). The first test causing this is

testClient(in_json_out_json, FruitAmount("orange", 11), Right(FruitAmount("orange", 11)))

Stacktrace:

[error] Caused by: java.io.IOException: Got bad status ordinal: 758655357
[error]         at org.scalajs.testing.common.Serializer$StatusSerializer$.deserialize(Serializer.scala:272)
[error]         at org.scalajs.testing.common.Serializer$StatusSerializer$.deserialize(Serializer.scala:265)
[error]         at org.scalajs.testing.common.Serializer$DeserializeState$.read$extension(Serializer.scala:37)
[error]         at org.scalajs.testing.common.Serializer$EventSerializer$$anon$6.<init>(Serializer.scala:304)
[error]         at org.scalajs.testing.common.Serializer$EventSerializer$.deserialize(Serializer.scala:300)
[error]         at org.scalajs.testing.common.Serializer$EventSerializer$.deserialize(Serializer.scala:290)
[error]         at org.scalajs.testing.common.Serializer$DeserializeState$.read$extension(Serializer.scala:37)
[error]         at org.scalajs.testing.common.RunMux$$anon$1.deserialize(RunMux.scala:28)
[error]         at org.scalajs.testing.common.RunMux$$anon$1.deserialize(RunMux.scala:21)
[error]         at org.scalajs.testing.common.Serializer$.deserialize(Serializer.scala:47)
[error]         at org.scalajs.testing.common.RPCCore.$anonfun$handleMessage$1(RPCCore.scala:115)
[error]         at org.scalajs.testing.common.RPCCore.$anonfun$handleMessage$1$adapted(RPCCore.scala:56)
[error]         at org.scalajs.testing.common.Serializer$.withInputStream(Serializer.scala:60)
[error]         at org.scalajs.testing.common.RPCCore.handleMessage(RPCCore.scala:56)
[error]         at org.scalajs.testing.adapter.JSEnvRPC.$anonfun$run$1(JSEnvRPC.scala:25)
[error]         at org.scalajs.testing.adapter.JSEnvRPC.$anonfun$run$1$adapted(JSEnvRPC.scala:25)
[error]         at org.scalajs.jsenv.selenium.SeleniumComRun.receivedMessage(SeleniumRun.scala:91)
[error]         at org.scalajs.jsenv.selenium.SeleniumRun.$anonfun$fetchAndProcess$3(SeleniumRun.scala:59)
[error]         at org.scalajs.jsenv.selenium.SeleniumRun.$anonfun$fetchAndProcess$3$adapted(SeleniumRun.scala:59)
[error]         at org.scalajs.jsenv.selenium.SeleniumRun$$anon$1.accept(SeleniumRun.scala:191)
[error]         at java.base/java.util.ArrayList.forEach(ArrayList.java:1541)
[error]         at org.scalajs.jsenv.selenium.SeleniumRun.fetchAndProcess(SeleniumRun.scala:59)
[error]         at org.scalajs.jsenv.selenium.SeleniumRun.$anonfun$handler$1(SeleniumRun.scala:37)
[error]         at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23)
[error]         at scala.concurrent.Future$.$anonfun$apply$1(Future.scala:659)
[error]         at scala.util.Success.$anonfun$map$1(Try.scala:255)
[error]         at scala.util.Success.map(Try.scala:213)
[error]         at scala.concurrent.Future.$anonfun$map$1(Future.scala:292)
[error]         at scala.concurrent.impl.Promise.liftedTree1$1(Promise.scala:33)
[error]         at scala.concurrent.impl.Promise.$anonfun$transform$1(Promise.scala:33)
[error]         at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:64)
[error]         at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
[error]         at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
[error]         at java.base/java.lang.Thread.run(Thread.java:834)

So this needs some more investigation.

I also saw that you just changed the client testserver a bit @adamw but I haven't rebased yet because there are some conflicts due to moving the testserver into its own module.

[sbrunk]

@adamw I looked briefly at your changes in the http4s server for client tests. If I understand it correctly, it listens on a port dynamically provided by the OS now. The issue is that with the separation between client tests and the testserver as done in this PR, we need a hardcoded port (this approach is also used in sttp). Especially since we can't really configure the ScalaTest JS runner from the build.

[adamw]

@sbrunk yeah you are right, we'll have to bring back the hard-coded port for the client tests (server tests are the most problematic here anyway, as they allocate a lot of ports). But I'm stuck on fixing some flaky tests to be able to release again since a couple of days, and these are some of my attempts to mitigate the problem. It's better, but not yet fixed ;)

[sbrunk]

With tagging available it compiles now. Thanks. :)
I'm starting to look into the failing multipart tests.

[info] - Endpoint(in: POST /api /echo /multipart {body as multipart/form-data}, errout: -, out: {body as text/plain (UTF-8)}) *** FAILED ***
[info]   Left(<(), the Unit value>) was not equal to Right("melon=10") (ClientTests.scala:44)
[info] - Endpoint(in: POST /api /echo {body as multipart/form-data}, errout: -, out: {body as text/plain (UTF-8)}) *** FAILED ***
[info]   "{"fruit":"apple","amount":10}
[info]   ------WebKitFormBoundary3BG7JFANPhegOl2n
[info]   Content-Disposition: form-data; name="notes"; filename="blob"
[info]   Content-Type: text/plain
[info]   
[info]   Now!
[info]   ------WebKitFormBoundary3BG7JFANPhegOl2n--
[info]   " did not include substring "Content-Type: text/plain; charset=UTF-8" (ClientMultipartTests.scala:24)

Do you know if it's possible to log the full raw request/response in sttp or in http4s testserver when running the tests somehow? I think it could be helpful to see how the first request is different when using a JVM backend.

In the second test it seems the JS version is missing the charset. Meaning it sets the Content-Type header only to text/plain instead of the expected text/plain; charset=UTF-8.

[sbrunk]

So I was playing a bit with cURL it seems in the first test the issue is that Content-Header is missing the boundary=...part, causing a 422.

I haven't looked into it yet, but my guess is that perhaps the header is overwritten in sttp's fetch backend? Apparently when using FormData, it will set Content-Header including boundary for multipart requests. See https://muffinman.io/uploading-files-using-fetch-multipart-form-data/

[adamw]

So that's the header of the part that is being changed?

Look at the AbstractFetchBackend implementation (in sttp-client): https://github.com/softwaremill/sttp/blob/561003b1fce6244b4c56517fdee6c73ac796e932/core/src/main/scalajs/sttp/client3/AbstractFetchBackend.scala#L183

it seems that custom content-type can only be set for blobs, and otherwise headers are not added. This might be a limitation of the backend - which would mean that we cannot do much. Or it might be that headers can be somehow added in the fetch implementation as well, but we're simply not doing it (yet :) )

[sbrunk]

Ah sorry I meant boundary=... is missing in the global content-type header, the part headers inside the body seem to be fine. This is how the full request looks like:

POST /api/echo/multipart HTTP/1.1
Host: localhost:51823
Connection: keep-alive
Content-Length: 322
content-type: multipart/form-data
Accept: */*
Origin: file://
Sec-Fetch-Site: cross-site
Sec-Fetch-Mode: cors
Sec-Fetch-Dest: empty
Accept-Encoding: gzip, deflate, br
Accept-Language: de-DE,de;q=0.9,en-US;q=0.8,en;q=0.7

------WebKitFormBoundaryRtwriztJD5FD0kh5
Content-Disposition: form-data; name="fruit"; filename="blob"
Content-Type: text/plain

melon
------WebKitFormBoundaryRtwriztJD5FD0kh5
Content-Disposition: form-data; name="amount"; filename="blob"
Content-Type: text/plain

10
------WebKitFormBoundaryRtwriztJD5FD0kh5--

To actually work, the content-type header needs the boundary set correctly, like so:

...
content-type: multipart/form-data; boundary=----WebKitFormBoundaryRtwriztJD5FD0kh5
...

Using the JavaScript FormData API with fetch (which the sttp fetch backend does) should set that header automatically with the correct boundary value. So my suspicion is that the header is manually overwritten somewhere.

[sbrunk]

I did a quick test by changing sttp's AbstractFetchBackend so it does not set the content-type header for multipart requests and it seems to work. :)

PR coming soon.

[sbrunk]

The multipart tests pass now by using 3.0.0-RC10 of sttp and by relaxing the charset check when running on JS. This is because, as you've said above, we can't control the content-type of parts either.

The remaining blocker is now this exception in the test runner when running SttpClientBasicTests.

...
[error] Caused by: java.io.IOException: Got bad status ordinal: 758655357
[error]         at org.scalajs.testing.common.Serializer$StatusSerializer$.deserialize(Serializer.scala:320)
...

I have to admit I'm a bit clueless where to start looking for the cause here. I've already asked on the Scala.js gitter channel but without an answer so far. I've also started to write down which tests cause this by commenting them out one by one, because it only happens in ~8 tests or so, so perhaps we can see a pattern there. I'll post the list here when I'm done finding all tests causing this error.

[adamw]

Hm never saw that error as well - do you have a single test that's failing like that? Could be easier to investigate. Are these only multipart tests?

[sbrunk]

Here's the list. The failing ones all come from ClientBasicTests.

tapir/client/tests/src/main/scala/sttp/tapir/client/tests/ClientBasicTests.scala

Line 27 in 6826ffc

testClient(in_json_out_json, FruitAmount("orange", 11), Right(FruitAmount("orange", 11)))

tapir/client/tests/src/main/scala/sttp/tapir/client/tests/ClientBasicTests.scala

Line 35 in 6826ffc

testClient(in_file_out_file, testFile, Right(testFile))

tapir/client/tests/src/main/scala/sttp/tapir/client/tests/ClientBasicTests.scala

Lines 67 to 68 in 6826ffc

	testClient(in_string_out_status_from_string.name("status one of 1"), "apple", Right(Right("fruit: apple")))
	testClient(in_string_out_status_from_string.name("status one of 2"), "papaya", Right(Left(29)))

[sbrunk]

Besides these four tests that cause the runner failure, we now have one remaining failing test:

[info] - Endpoint(in: GET /api /echo /param-to-header ?qq, errout: -, out: {header hh}) *** FAILED ***
[info]   List("apple, watermelon, plum") did not contain the same elements as List("apple", "watermelon", "plum") (ClientBasicTests.scala:50)

In this test:

tapir/client/tests/src/main/scala/sttp/tapir/client/tests/ClientBasicTests.scala

Lines 44 to 51 in 4852803

	test(in_query_list_out_header_list.showDetail) {
	// Note: some clients do not preserve the order in header values
	send(
	in_query_list_out_header_list,
	port,
	List("plum", "watermelon", "apple")
	).unsafeToFuture().map(_.right.get should contain theSameElementsAs List("apple", "watermelon", "plum"))
	}

Given this response from the testserver:

HTTP/1.1 200 OK
hh: apple
hh: watermelon
hh: plum
...

It seems like there's something wrong with how the response headers are converted to a list here.

[sbrunk]

Turns out this is actually a restriction of how the the Fetch API represents headers. The Headers class merges header values with the same name into a single, comma separated string. See https://developer.mozilla.org/en-US/docs/Web/API/Headers/append for an example and https://fetch.spec.whatwg.org/#headers-class for an explanation.

According to https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html that's actually allowed by HTTP.

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded.

So the question is what we should do about it.

[adamw]

@sbrunk multi-header support is something that differs from client to client and server to server. If fetch behaves this way, we have to embrace that and adjust the test - I think checking that there's either three headers or one combined would be sufficient

[sbrunk]

@adamw I've adjusted the test accordingly and rebased against master.

I've also created https://github.com/scala-js/scala-js/issues/4310 due to the test runner failures, perhaps the Scala.js devs have an idea what could cause that issue.

[sbrunk]

@adamw I added support for using Firefox with geckodriver (including autmatic download) as a replacement for Chrome/chromedriver and it is working locally now. Unfortunately it still doesn't work in GH actions due to memory issues even with a larger heap. The build often seems to hang at some point without any output, and when it fails, there are no logs.

[sbrunk]

I've done some experiments splitting linking and test into different steps (in a new branch). Giving the JVM in the linking step 6G heap and during the test 1G gets us a little bit further. With this setup, the tests seem to finish but then the build fails with a timeout trying to connect to the browser:
https://github.com/sbrunk/tapir/runs/1495365778?check_suite_focus=true

My suspicion is that the tests start to many Firefox instances and only clean them up at the end so the runner runs out of resources.

[adamw]

@sbrunk this size of memory consumption seems suspicious, in sttp-client we've got more JS modules and the tests pass fine. But maybe it's a chrome vs firefox problem?

Can we somehow start a single firefox instance or is that not possible?

[sbrunk]

@adamw looks like this is due to a current limitation of the sbt test framework integration. See scala-js/scala-js#4317. It only shows up now because Firefox needs a little bit more memory than Chrome which makes a difference with ~30 processes running at the same time...

The suggested workaround is to call test on each subproject explicitly instead of on the aggregate. I've done that in CI and now the tests are green! :)

tapir/.github/workflows/ci.yml

Lines 34 to 38 in 0b00138

	# Temporarily call JS tests for each subproject explicitly as a workaround until
	# https://github.com/scala-js/scala-js/issues/4317 has a solution
	- name: Test Scala.js
	if: matrix.target-platform == 'JS'
	run: sbt -v mimaReportBinaryIssues coreJS/test catsJS/test enumeratumJS/test refinedJS/test circeJsonJS/test playJsonJS/test uPickleJsonJS/test jsoniterScalaJS/test sttpClientJS/test

So if you're ok with that workaround for now this PR should be ready for review now.

[adamw]

Actually now it occurred to me that maybe a simplification is possible. We only really need to start firefox for sttpClientJS, no? The others should run fine without selenium?

Another question - shouldn't we enumerate (in gh actions ci.yml) both e.g. sttpClientJS and sttpClientJS2_12?

[sbrunk]

Actually now it occurred to me that maybe a simplification is possible. We only really need to start firefox for sttpClientJS, no? The others should run fine without selenium?

@adamw yes that's what I had done initially but you suggested self-contained build without having to install node so I went with the full browser approach (see our discussion in #815 (comment)).

Another question - shouldn't we enumerate (in gh actions ci.yml) both e.g. sttpClientJS and sttpClientJS2_12?

Ah yes good catch. Totally missed 2.12 in the workaround.

[adamw]

@sbrunk yes maybe it's time to revisit this assumption ;) see #880 for some of my experiments

[adamw]

Released in https://github.com/softwaremill/tapir/releases/tag/v0.17.0-M11 - thanks!