Added getAs for QueryParams #2505

BanyMaciej · 2023-11-01T13:11:06Z

Add getAs[A] for QueryParams

Closes #2423

CLAassistant · 2023-11-01T13:11:13Z

All committers have signed the CLA.

codecov-commenter · 2023-11-01T13:26:18Z

Codecov Report

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

Comparison is base (1ee9701) 64.95% compared to head (dbaaac9) 64.95%.
Report is 1 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2505      +/-   ##
==========================================
- Coverage   64.95%   64.95%   -0.01%     
==========================================
  Files         136      137       +1     
  Lines        7197     7211      +14     
  Branches     1296     1312      +16     
==========================================
+ Hits         4675     4684       +9     
- Misses       2522     2527       +5

Files	Coverage Δ
zio-http/src/main/scala/zio/http/QueryParams.scala	`84.78% <83.33%> (-0.52%)`	⬇️
...ttp/src/main/scala/zio/http/QueryParamsError.scala	`0.00% <0.00%> (ø)`

... and 1 file with indirect coverage changes

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

guersam · 2023-11-02T05:18:20Z

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

+  /**
+   * Retrieves the first typed query parameter value having the specified name.
+   */
+  def getAs[A](key: String)(implicit codec: TextCodec[A]): Option[A] = get(key).flatMap(codec.decode)


I think there could be needs to distinguish between the absence of the parameter and decoding failure. What do you think?

It's good point, but I'm afraid that if we have output type like Either[String, Option[A]] or Option[Either[String, A]], it may be even harder to use than separating it to get and decode on the call site. Maybe I should define separate typed error for this, something like DecodingError and NotFoundError and then result would be Either[QueryParamError, A]?

I agree with your concern. Personally I'd prefer the latter.

guersam · 2023-11-02T12:45:56Z

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

+   * Retrieves the first typed query parameter value having the specified name.
+   */
+  def getAs[A](key: String)(implicit codec: TextCodec[A]): Either[QueryParamsError, A] = for {
+    param      <- get(key).toRight(QueryParamsError.MissingQueryParam(key))


Great! QueryParamsError.Missing seems more readable to me. What do you think?

Seems logical :)

987Nabil · 2023-11-02T21:14:22Z

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

@@ -18,9 +18,10 @@ package zio.http

 import java.nio.charset.Charset

-import zio.Chunk
+import zio.{Chunk, NonEmptyChunk}


NonEmptyChunk is unused or?

You are right - I left some unused imports, I would fix it

987Nabil · 2023-11-02T21:17:14Z

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

+  /**
+   * Retrieves the first typed query parameter value having the specified name.
+   */
+  def getAs[A](key: String)(implicit codec: TextCodec[A]): Either[QueryParamsError, A] = for {


I am wondering, if a def getAsZIO[A](key: String)(implicit codec: TextCodec[A]): IO[QueryParamsError, A] makes sense too.

Isn't wrapping it in ZIO to heavy machinery for this task? I believe calling it like ZIO.fromEither(queryParams.getAs ...) is not much trouble, and current version could be possibly used in some lower level implementations :)

I believe there could be a significant gain in readability at the cost of a minor performance overhead. Keeping getAs as is and adding getAsZIO as a variant seems consistent with existing ZIO functions like ZIO.serviceWith and ZIO.serviceWithZIO.

Thanks for explaining :) You are right, I will add additional functions

guersam

Sorry for late, added my two cents over the error ADT.

guersam · 2023-11-07T09:16:43Z

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

+  def message: String
+}
+object QueryParamsError {
+  final case class Missing(queryParamName: String) extends QueryParamsError {


Missing#queryParamName seems inconsistent with Malformed#name.

guersam · 2023-11-07T09:19:53Z

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

+    def message = s"Unable to decode query parameter with name $name and value $value using $codec"
+  }
+
+  final case class MultiMalformed(chunk: Chunk[Malformed]) extends QueryParamsError {


Aren't name and codec the same across the entire chunk in a MultiMalformed? If so, I'd suggest an another encoding:

final case class MultiMalformed(name: String, codec: `TextCodec[_]`, values: Chunk[String]) extends QueryParamsError

Actually, then we could get rid of the "single' Malformed - it could be represented as MultiMalformed (then I will rename it to Malformed) with one element chunk. What do you think?

👍 NonEmptyChunk could give more guarantee here.

guersam · 2023-11-07T09:22:21Z

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

+  }
+
+  final case class Malformed(name: String, value: String, codec: TextCodec[_]) extends QueryParamsError {
+    def message = s"Unable to decode query parameter with name $name and value $value using $codec"


I doubt that TextCodec always provides stable toString. codec.describe might be more reliable choice.

Yes, that is true.

BanyMaciej requested review from amitksingh1490, jdegoes, vigoo and adamgfraser as code owners November 1, 2023 13:11

added getAs[A] for QueryParams

17a45fa

BanyMaciej force-pushed the zio-http-issue-2423 branch from 844871a to 17a45fa Compare November 1, 2023 13:17

guersam reviewed Nov 2, 2023

View reviewed changes

Added getAs for QueryParams - with typed errors

36f75b9

guersam reviewed Nov 2, 2023

View reviewed changes

Added getAs for QueryParams - Errors naming fix

9ae1bd0

BanyMaciej requested a review from guersam November 2, 2023 13:20

987Nabil reviewed Nov 2, 2023

View reviewed changes

BanyMaciej added 2 commits November 2, 2023 23:07

Added getAs for QueryParams - imports fix

68be226

Added getAs for QueryParams - added getAsZIO

dbaaac9

987Nabil approved these changes Nov 4, 2023

View reviewed changes

987Nabil merged commit 40c2961 into zio:main Nov 6, 2023
14 checks passed

guersam reviewed Nov 7, 2023

View reviewed changes

BanyMaciej mentioned this pull request Nov 7, 2023

Added getAs for QueryParams - errors enhancement #2510

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added getAs for QueryParams #2505

Added getAs for QueryParams #2505

BanyMaciej commented Nov 1, 2023 •

edited

CLAassistant commented Nov 1, 2023 •

edited

codecov-commenter commented Nov 1, 2023 •

edited

guersam Nov 2, 2023

BanyMaciej Nov 2, 2023

guersam Nov 2, 2023

guersam Nov 2, 2023

BanyMaciej Nov 2, 2023

987Nabil Nov 2, 2023

BanyMaciej Nov 2, 2023

987Nabil Nov 2, 2023

BanyMaciej Nov 2, 2023

guersam Nov 3, 2023

BanyMaciej Nov 3, 2023

guersam left a comment

guersam Nov 7, 2023

guersam Nov 7, 2023

BanyMaciej Nov 7, 2023

987Nabil Nov 7, 2023

guersam Nov 7, 2023

guersam Nov 7, 2023

987Nabil Nov 7, 2023

Added getAs for QueryParams #2505

Added getAs for QueryParams #2505

Conversation

BanyMaciej commented Nov 1, 2023 • edited

CLAassistant commented Nov 1, 2023 • edited

codecov-commenter commented Nov 1, 2023 • edited

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guersam left a 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

Choose a reason for hiding this comment

BanyMaciej commented Nov 1, 2023 •

edited

CLAassistant commented Nov 1, 2023 •

edited

codecov-commenter commented Nov 1, 2023 •

edited