Add Flash, Flash.Message and Flash.Backend #2481

TomTriple · 2023-10-13T12:30:31Z

Same idea as in #2466. Summary:

Add typed and composable access to the flash-scope with a new type Flash. For that there exists various Flash#get as well as Flash#set operators.
Add a new type Flash.Message that adds an explicit type for typical use cases of flash messages representing a notice or alert (or both!). Inspired by https://guides.rubyonrails.org/action_controller_overview.html#the-flash.
Add cookie-based flash-scope with the typical size limitation that cookies have. The entire payload will be transported in the content of a cookie. Using this flash-scope involves no effects, only Response#addFlash / Request#flash.
Add backend-based (Flash.Backend) flash-scope that has no limitation in size as the payload is not transported in a cookie but kept in an internal structure. Still, it uses a cookie to transport an identifier for the lookup into the internal structure. Using this flash-scope involves effects as we need to access an internal Ref for the payload.

codecov-commenter · 2023-10-13T12:41:28Z

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (5906d30) 64.75% compared to head (380e07a) 65.14%.
Report is 11 commits behind head on main.

❗ Current head 380e07a differs from pull request most recent head 996364b. Consider uploading reports for the commit 996364b to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2481      +/-   ##
==========================================
+ Coverage   64.75%   65.14%   +0.39%     
==========================================
  Files         135      137       +2     
  Lines        7135     7291     +156     
  Branches     1200     1285      +85     
==========================================
+ Hits         4620     4750     +130     
- Misses       2515     2541      +26

Files	Coverage Δ
zio-http/src/main/scala/zio/http/Middleware.scala	`57.64% <ø> (-15.49%)`	⬇️
zio-http/src/main/scala/zio/http/Request.scala	`44.68% <100.00%> (+6.38%)`	⬆️
zio-http/src/main/scala/zio/http/Response.scala	`42.85% <100.00%> (+2.38%)`	⬆️
zio-http/src/main/scala/zio/http/Flash.scala	`77.65% <77.65%> (ø)`

... and 18 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

guizmaii · 2023-10-18T06:43:32Z

@TomTriple To overcome the cookie content size limitation we could split the content of the flash into several cookies. WDYT?

guizmaii · 2023-10-18T06:52:02Z

zio-http/src/main/scala/zio/http/Flash.scala

+  def flatMap[B](f: A => Flash[B]): Flash[B] = Flash.FlatMap(self, f)
+
+  def map[B](f: A => B): Flash[B] = self.flatMap(a => Flash.succeed(f(a)))
+
+  def orElse[B >: A](that: => Flash[B]): Flash[B] = Flash.OrElse(self, that)
+
+  /**
+   * Operator alias for `orElse`.
+   */
+  def <>[B >: A](that: => Flash[B]): Flash[B] = self.orElse(that)
+
+  def zip[B](that: => Flash[B]): Flash[(A, B)] = self.zipWith(that)((a, b) => a -> b)
+
+  /**
+   * Operator alias for `zip`.
+   */
+  def <*>[B](that: => Flash[B]): Flash[(A, B)] = self.zip(that)
+
+  def zipWith[B, C](that: => Flash[B])(f: (A, B) => C): Flash[C] =
+    self.flatMap(a => that.map(b => f(a, b)))
+
+  def optional: Flash[Option[A]] = self.map(Option(_)) <> Flash.succeed(None)
+
+  def foldHtml[A1 >: A, B](f: Html => B, g: Html => B)(h: (B, B) => B)(implicit
+    ev: A1 =:= Flash.Message[Html, Html],
+  ): Flash[B] =
+    self.map(a => a.asInstanceOf[A1].fold(f, g)(h))
+
+  def toHtml[A1 >: A](implicit ev: A1 =:= String): Flash[Html] =
+    self.map(Html.fromString(_))


Don't we need these methods to be final? Is there a need for them to be overridable?

I can't hurt to make them final!

guizmaii · 2023-10-18T07:10:27Z

zio-http/src/main/scala/zio/http/Flash.scala

+      }
+    }
+
+    val layer: ULayer[Backend] = ZLayer(Ref.make(Map.empty[UUID, Map[String, String]]).map(Impl.apply))


I do not see the point of providing a flash backend based on a Map as most probably users' apps will be horizontally scaled. For dev purposes? 🤔

What if we rename this layer to inMemory then?

Yes, you're right - when you're scaling the app that does not work anymore. In this case one needs to use an external data service. I think it should be possible to add another Backend with the given interface.

Yes, calling it inMemory is a good idea!

guizmaii · 2023-10-18T07:13:26Z

zio-http/src/main/scala/zio/http/Flash.scala

+        Flash.COOKIE_NAME,
+        URLEncoder.encode(
+          JsonCodec.jsonEncoder(Schema[Map[String, String]]).encodeJson(run(setter, Map.empty)).toString,
+          java.nio.charset.Charset.defaultCharset.toString.toLowerCase,


Why not UTF8? 🤔

Can not hurt to make that explicit!

guizmaii · 2023-10-18T07:17:00Z

zio-http/src/main/scala/zio/http/Flash.scala

+        ),
+      )
+
+    def run[A](setter: Setter[A], map: Map[String, String]): Map[String, String] = {


We could inline this run function into the previous one, no?

def run[A](setter: Setter[A]): Cookie.Response = { def loop(setter: Setter[A], map: mutable.Map[String, String]): mutable.Map[String, String] = setter match { case SetValue(schema, key, a) => map += (key -> JsonCodec.jsonEncoder(schema).encodeJson(a).toString) case Concat(left, right) => loop(right, loop(left, map)) case Empty => map } Cookie.Response( Flash.COOKIE_NAME, URLEncoder.encode( JsonCodec.jsonEncoder(Schema[Map[String, String]]).encodeJson(loop(setter, mutable.Map.empty).toMap).toString, java.nio.charset.Charset.defaultCharset.toString.toLowerCase, ) ) }

Or you want to expose both function maybe?

Internally there both run operations are used

guizmaii · 2023-10-18T07:22:03Z

zio-http/src/main/scala/zio/http/Flash.scala

+    /**
+     * Gets an `A` from the backend-based flash-scope and provides a fallback.
+     */
+    def flashOrElse[A](request: Request, flash: Flash[A])(orElse: => A): UIO[A] =


guizmaii · 2023-10-18T07:22:09Z

zio-http/src/main/scala/zio/http/Flash.scala

+     * Optionally adds flash values to the backend-based flash-scope and returns
+     * a workflow with an updated `Response`.
+     */
+    def addFlash[A](response: Response, setterOpt: Option[Flash.Setter[A]]): UIO[Response] =


guizmaii · 2023-10-18T07:22:59Z

zio-http/src/main/scala/zio/http/Flash.scala

+    /**
+     * Combines setting this flash value with another setter `that`.
+     */
+    def ++[B](that: => Setter[B]): Setter[(A, B)] = Setter.Concat(self, that)


guizmaii · 2023-10-18T07:35:36Z

zio-http/src/main/scala/zio/http/Flash.scala

+    map.keys.map(a => Flash.get(a)(Schema[A])).reduce(_ <> _)
+  }
+
+  private def loop[A](flash: Flash[A], map: Map[String, String]): Either[Throwable, A] =


Why using Throwable here in the Left part and not just String? 🤔
Using String would simplify the code a bit, no?

🟢 Also, as this function is only used in the run function below (I'm maybe wrong here), I'd put it as a inner function of the run function:

private[http] def run[A](flash: Flash[A], sourceMap: Map[String, String]): Either[Throwable, A] = { def loop[A](flash: Flash[A], map: Map[String, String]): Either[Throwable, A] = ... loop(flash, sourceMap) }

zio-http/src/main/scala/zio/http/Flash.scala

guizmaii · 2023-10-18T07:44:17Z

zio-http/src/main/scala/zio/http/Flash.scala

+      .flatMap { cookie =>
+        try Right(URLDecoder.decode(cookie.content, java.nio.charset.Charset.defaultCharset.toString.toLowerCase))
+        catch {
+          case e: Exception => Left(e)
+        }
+      }
+      .flatMap { cookieContent =>
+        JsonCodec.jsonDecoder(Schema.map[String, String]).decodeJson(cookieContent).left.map(e => new Throwable(e))
+      }


🟢 For performances, I'd inline these 2 .flatMap:

.flatMap { cookie => try { val content = URLDecoder.decode(cookie.content, java.nio.charset.Charset.defaultCharset.toString.toLowerCase)) JsonCodec.jsonDecoder(Schema.map[String, String]).decodeJson(content).left.map(e => new Throwable(e)) } catch { case e: Exception => Left(e) } }

TomTriple · 2023-10-18T07:49:23Z

@TomTriple To overcome the cookie content size limitation we could split the content of the flash into several cookies. WDYT?

I think that's a interesting proposal - even though the limit then is at about 300 cookies (which... should be enough for most / all? things regarding payload data in the flash). I like your idea but right now I´m not sure about any other side effects that come up with it.

guizmaii · 2023-10-18T07:51:23Z

@TomTriple We could keep this "split into several cookies" idea for another PR, I just wanted to mention it 🙂

Co-authored-by: Jules Ivanic <jules.ivanic@gmail.com>

TomTriple · 2023-10-18T08:16:03Z

@TomTriple We could keep this "split into several cookies" idea for another PR, I just wanted to mention it 🙂

Yes of course - we will see what other reviewers think about it. I'll send another PR with your changes from the review. Thanks a lot for reviewing 👍

guizmaii · 2023-10-18T15:20:05Z

zio-http/src/main/scala/zio/http/Flash.scala

+      .flatMap { cookie =>
+        try {
+          val content =
+            URLDecoder.decode(cookie.content, java.nio.charset.Charset.forName("UTF-8").toString.toLowerCase)


Can't you just use StandardCharsets.UTF_8? 🤔 Same for the .encode usage

Suggested change

URLDecoder.decode(cookie.content, java.nio.charset.Charset.forName("UTF-8").toString.toLowerCase)

URLDecoder.decode(cookie.content, StandardCharsets.UTF_8)

Thanks, somehow I did not know about StandardCharsets

TomTriple added 5 commits October 1, 2023 16:37

add new flash type and changed flash api

1511bf4

fmt

8e48c30

tiny stuff

b409633

add optional flash!

ba31c1c

add Flash.Message

10ea315

TomTriple requested review from amitksingh1490, jdegoes, vigoo and adamgfraser as code owners October 13, 2023 12:30

TomTriple force-pushed the flash-improve-with-env branch 2 times, most recently from 6d4a851 to 04661b7 Compare October 13, 2023 12:40

TomTriple force-pushed the flash-improve-with-env branch 2 times, most recently from 34e1365 to 65d0f40 Compare October 13, 2023 12:55

change flash message, add flash backend

cc778b1

TomTriple force-pushed the flash-improve-with-env branch from 65d0f40 to cc778b1 Compare October 13, 2023 13:13

bugfix for cookie path

ea70b8a

TomTriple force-pushed the flash-improve-with-env branch from 9b7fcff to ea70b8a Compare October 15, 2023 17:56

guizmaii reviewed Oct 18, 2023

View reviewed changes

zio-http/src/main/scala/zio/http/Flash.scala Outdated Show resolved Hide resolved

guizmaii reviewed Oct 18, 2023

View reviewed changes

zio-http/src/main/scala/zio/http/Flash.scala Outdated Show resolved Hide resolved

guizmaii reviewed Oct 18, 2023

View reviewed changes

zio-http/src/main/scala/zio/http/Flash.scala Outdated Show resolved Hide resolved

guizmaii reviewed Oct 18, 2023

View reviewed changes

TomTriple and others added 3 commits October 18, 2023 10:12

Update zio-http/src/main/scala/zio/http/Flash.scala

276d354

Co-authored-by: Jules Ivanic <jules.ivanic@gmail.com>

Update zio-http/src/main/scala/zio/http/Flash.scala

1f9fcf0

Co-authored-by: Jules Ivanic <jules.ivanic@gmail.com>

Update zio-http/src/main/scala/zio/http/Flash.scala

0f0434e

Co-authored-by: Jules Ivanic <jules.ivanic@gmail.com>

guizmaii reviewed Oct 18, 2023

View reviewed changes

TomTriple force-pushed the flash-improve-with-env branch 5 times, most recently from 03ef4d5 to 00873c6 Compare October 19, 2023 09:23

changes from review

47e8268

TomTriple force-pushed the flash-improve-with-env branch from 00873c6 to 47e8268 Compare October 19, 2023 09:26

Merge branch 'main' into flash-improve-with-env

dbe9bb7

TomTriple requested a review from 987Nabil as a code owner December 8, 2023 11:37

jdegoes approved these changes Dec 21, 2023

View reviewed changes

jdegoes merged commit f4b68d2 into zio:main Dec 21, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Flash, Flash.Message and Flash.Backend #2481

Add Flash, Flash.Message and Flash.Backend #2481

TomTriple commented Oct 13, 2023 •

edited

Loading

codecov-commenter commented Oct 13, 2023 •

edited

Loading

guizmaii commented Oct 18, 2023

guizmaii Oct 18, 2023

TomTriple Oct 18, 2023

guizmaii Oct 18, 2023 •

edited

Loading

TomTriple Oct 18, 2023

guizmaii Oct 18, 2023

TomTriple Oct 18, 2023

guizmaii Oct 18, 2023 •

edited

Loading

TomTriple Oct 18, 2023

guizmaii Oct 18, 2023

TomTriple Oct 18, 2023

guizmaii Oct 18, 2023

TomTriple Oct 18, 2023

guizmaii Oct 18, 2023

TomTriple Oct 18, 2023

guizmaii Oct 18, 2023 •

edited

Loading

guizmaii Oct 18, 2023 •

edited

Loading

TomTriple commented Oct 18, 2023

guizmaii commented Oct 18, 2023

TomTriple commented Oct 18, 2023

guizmaii Oct 18, 2023 •

edited

Loading

TomTriple Oct 18, 2023

	URLDecoder.decode(cookie.content, java.nio.charset.Charset.forName("UTF-8").toString.toLowerCase)
	URLDecoder.decode(cookie.content, StandardCharsets.UTF_8)

Add Flash, Flash.Message and Flash.Backend #2481

Add Flash, Flash.Message and Flash.Backend #2481

Conversation

TomTriple commented Oct 13, 2023 • edited Loading

codecov-commenter commented Oct 13, 2023 • edited Loading

Codecov Report

guizmaii commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guizmaii Oct 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

guizmaii Oct 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

guizmaii Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

guizmaii Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

TomTriple commented Oct 18, 2023

guizmaii commented Oct 18, 2023

TomTriple commented Oct 18, 2023

guizmaii Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomTriple commented Oct 13, 2023 •

edited

Loading

codecov-commenter commented Oct 13, 2023 •

edited

Loading

guizmaii Oct 18, 2023 •

edited

Loading

guizmaii Oct 18, 2023 •

edited

Loading

guizmaii Oct 18, 2023 •

edited

Loading

guizmaii Oct 18, 2023 •

edited

Loading

guizmaii Oct 18, 2023 •

edited

Loading