Use `Clock` from `CE` instead of one from `java.time` in `CSRF` Middleware #7166

danicheg · 2023-06-17T06:42:24Z

This could be controversial, so I'd be glad to hear your opinions.

danicheg · 2023-06-17T06:48:54Z

server/shared/src/main/scala/org/http4s/server/middleware/CSRF.scala

+    @deprecated(
+      "This method doesn't change anything since the `java.time.Clock` has been purged from CSRFBuilder",
+      "v1.0.0-M40",
+    )
+    def withClock(clock: Clock): CSRFBuilder[F, G] = this


I'm worrying about this breaking behaviour, but I didn't come up with anything better than that. At least, users will be get notified by the compiler. 🤷🏻

I think it's a fine heads up.

danicheg · 2023-06-17T07:13:09Z

Flatmapping is challenging in Scala 3 😓

[error] -- [E008] Not Found Error: /home/runner/work/http4s/http4s/server/shared/src/test/scala/org/http4s/server/middleware/CSRFSuite.scala:302:10 
[error] 302 |          control.results.flatMap(_.get match {
[error]     |          ^
[error]     |value withFilter is not a member of cats.effect.IO[(
[error]     |  org.http4s.server.middleware.CSRF[cats.effect.IO, cats.effect.IO]
[error]     |, org.http4s.server.middleware.CSRF.CSRFToken, String)], but could be made available as an extension method.
[error]     |
[error]     |The following import might fix the problem:
[error]     |
[error]     |  import org.scalacheck.Gen.const
[error]     |
[error] 303 |            case Outcome.Canceled() =>
[error] 304 |              IO.raiseError(new IllegalStateException("Canceled"))
[error] 305 |            case Outcome.Errored(t) =>
[error] 306 |              IO.raiseError(t)
[error] 307 |            case Outcome.Succeeded(res) =>
[error] 308 |              IO(res)
[error] 309 |          })

danicheg · 2023-06-17T07:50:21Z

server/shared/src/test/scala/org/http4s/server/middleware/CSRFSuite.scala

+        _ <- control.advanceAndTick(1.millis)
+        res <-
+          control.results.flatMap(_.get match {


It seems like TestControl behaves differently for JS or maybe I'm doing something wrong 🤔 cc @armanbilge

No, TestControl should behave the same. Does the code make some side-effecting call somewhere? e.g. something that relies on delay(...), blocking(...) or async(...)?

Aha, yes it does. It needs HMAC cryptography. This is async on JS (i.e. it takes real time to execute) so it cannot be used on the test runtime with mock time.

Maybe we can just disable this test on JS, like this.

http4s/tests/shared/src/test/scala/org/http4s/UriSuite.scala

Lines 998 to 999 in 88f6688

if (Platform.isJvm || Platform.isNative)

test("Uri parameter convenience methods should set a parameter with a float values") {

This is async on JS (i.e. it takes real time to execute) so it cannot be used on the test runtime with mock time.

This is also worth noting somewhere on CE docs. It's a good-to-know thing!

I see, thanks. However, it's more about deadlock, rather than just getting different behaviour for JS. I mean, we didn't get any of the tests hanged forever. They were executed but without mocking the time.

They were executed but without mocking the time.

No, they did mock the time :) it just deadlock

I mean, we didn't get any of the tests hanged forever.

It's because in TestControl deadlock does not mean the tests hang forever. Deadlock is detected as no result even though there are no fibers to run.

I realize this is all very confusing :) if you use executeEmbed as I recommended you would get a more informative error.

https://github.com/typelevel/cats-effect/blob/8ba8916407eb8f35fbffae76f72dac48d2dc7357/testkit/shared/src/main/scala/cats/effect/testkit/TestControl.scala#L413

It's baffling why we don't get the same errors when ticking then (if an IO is deadlocked) 🤔

Because when using the advanced API, it's not an error to have no result. The advanced API simply lets check if a result is present or not, which is why it returns an Option. If you use .get on the Option, then you may get a surprising error.

http4s/server/shared/src/test/scala/org/http4s/server/middleware/CSRFSuite.scala

Line 166 in 8bc0fee

control.results.flatMap(_.get match {

armanbilge · 2023-06-17T14:43:37Z

server/shared/src/test/scala/org/http4s/server/middleware/CSRFSuite.scala

+      TestControl.execute(program).flatMap { control =>
+        for {
+          _ <- control.advanceAndTick(1.millis)


Btw, this is a bit of an anti-pattern. Can we write this test case using executeEmbed(...) and IO.sleep(...) instead of advanceAndTick?

Oh, really? Wasn't aware that this usage of TestControl is considered an anti-pattern 😓 Is this piece of knowledge presented on some CE resources?

Yes, see here. Unless this is an advanced situation, executeEmbed is the preferred way.

https://typelevel.org/cats-effect/docs/core/test-runtime#full-execution

The first sort of test we will attempt to write comes in the form of a complete execution. This is generally the most common scenario, in which most of the power of TestControl is unnecessary
...
For more advanced cases, executeEmbed may not be enough to properly measure the functionality under test.

TestControl#execute + advanceAndTick seems less tricky than TestControl#executeEmbed + IO.sleep. Or maybe I'm just biased about any usage of IO.sleep anywhere. But I really wonder in which ways this is precisely anti-pattern...

But I really wonder in which ways this is precisely anti-pattern...

You can ask Fabio about it more on the Discord. For example see comment here.
https://discord.com/channels/632277896739946517/632310980449402880/947533678546939935

Maybe "anti-pattern" was too strong a word. I meant it in the sense: using advanced, complex APIs when simple APIs will work is an anti-pattern.

I'm not 100% sure I understand the objection to sleep, it seems something along the lines of:
"having time pass in tests (albeit simulated) is weird, I want to inject mocked time"
but I think the opposite is true, having simulated time pass in tests is better than mocking time as a general pattern, because if the implementation of the code uses sleep, the mocked time has to be consistent with that.
It also has the conceptual advantage that it's behaving like the real program would, and the convenience advantage that using TestControl then reduces to calling a single function: executeEmbed.
In fact, you can often just write a normal program with sleep, concurrency , Refs to collect results etc, try it out in the repl, and then convert it to a deterministic case by adding executeEmbed.

Finally, on the trickiness aspect, manual advance is definitely trickier, there are some cases where the abstraction is a bit leaky

danicheg · 2023-06-24T14:36:12Z

@SystemFw @armanbilge thank you, folks, for bearing with me and explaining the difference between TestControl#executeEmbed and TestControl#execute. My objections against IO.sleep is no more than just biased opinion. In short, I mean that users will likely use more high-level functions such as delay(timeunit), throttle, retry and so on in their code directly (even if they are built powered with IO.sleep). IO.sleep feels like too low-level code.

armanbilge

Dare I say ... shall we run this through 0.23 😛 nahh, although I think it's do-able.

Thanks for cleaning this one up :)

server/shared/src/main/scala/org/http4s/server/middleware/CSRF.scala

server/shared/src/test/scala/org/http4s/server/middleware/CSRFSuite.scala

Co-authored-by: Arman Bilge <armanbilge@gmail.com>

danicheg added 2 commits June 17, 2023 10:38

Use Clock from CE instead of one from java.time in CSRF Middleware

6d6ac10

Refactor CSRF tests

974adf7

danicheg added the breaking Change breaks compatibility (binary or source) label Jun 17, 2023

mergify bot added the module:server label Jun 17, 2023

danicheg commented Jun 17, 2023

View reviewed changes

Fix flatmapping in Scala 3

8bc0fee

danicheg commented Jun 17, 2023

View reviewed changes

armanbilge reviewed Jun 17, 2023

View reviewed changes

danicheg added 3 commits June 24, 2023 17:33

Merge branch 'main' into csrf-clock

3ed3517

Use TestControl#executeEmbed in CSRFSuite

945c7c2

Filter out a few tests from running on JS in CSRFSuite

33472df

armanbilge approved these changes Jun 26, 2023

View reviewed changes

Address review comments

9baf048

Co-authored-by: Arman Bilge <armanbilge@gmail.com>

danicheg merged commit 6108459 into http4s:main Jun 30, 2023
13 checks passed

danicheg deleted the csrf-clock branch June 30, 2023 05:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `Clock` from `CE` instead of one from `java.time` in `CSRF` Middleware #7166

Use `Clock` from `CE` instead of one from `java.time` in `CSRF` Middleware #7166

danicheg commented Jun 17, 2023

danicheg Jun 17, 2023

rossabaker Jun 21, 2023

danicheg commented Jun 17, 2023

danicheg Jun 17, 2023

armanbilge Jun 17, 2023

armanbilge Jun 17, 2023

armanbilge Jun 17, 2023

danicheg Jun 18, 2023

danicheg Jun 18, 2023

armanbilge Jun 18, 2023

danicheg Jun 18, 2023

armanbilge Jun 18, 2023 •

edited

Loading

danicheg Jun 18, 2023

armanbilge Jun 17, 2023

danicheg Jun 18, 2023

armanbilge Jun 18, 2023

danicheg Jun 18, 2023

armanbilge Jun 18, 2023

SystemFw Jun 19, 2023

danicheg commented Jun 24, 2023

armanbilge left a comment

	if (Platform.isJvm \|\| Platform.isNative)
	test("Uri parameter convenience methods should set a parameter with a float values") {

Use Clock from CE instead of one from java.time in CSRF Middleware #7166

Use Clock from CE instead of one from java.time in CSRF Middleware #7166

Conversation

danicheg commented Jun 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danicheg commented Jun 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge Jun 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danicheg commented Jun 24, 2023

armanbilge left a comment

Choose a reason for hiding this comment

Use `Clock` from `CE` instead of one from `java.time` in `CSRF` Middleware #7166

Use `Clock` from `CE` instead of one from `java.time` in `CSRF` Middleware #7166

armanbilge Jun 18, 2023 •

edited

Loading