Added getAs for QueryParams #2505
Conversation
BanyMaciej
requested review from
amitksingh1490,
jdegoes,
vigoo and
adamgfraser
as code owners
|
|
BanyMaciej
force-pushed
the
zio-http-issue-2423
branch
from
844871a to
17a45fa
Compare
Codecov ReportAttention:
❗ 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
☔ View full report in Codecov by Sentry. |
| /** | ||
| * 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) |
There was a problem hiding this comment.
I think there could be needs to distinguish between the absence of the parameter and decoding failure. What do you think?
There was a problem hiding this comment.
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]?
There was a problem hiding this comment.
I agree with your concern. Personally I'd prefer the latter.
| * 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)) |
There was a problem hiding this comment.
Great! QueryParamsError.Missing seems more readable to me. What do you think?
| @@ -18,9 +18,10 @@ package zio.http | |||
|
|
|||
| import java.nio.charset.Charset | |||
|
|
|||
| import zio.Chunk | |||
| import zio.{Chunk, NonEmptyChunk} | |||
There was a problem hiding this comment.
NonEmptyChunk is unused or?
There was a problem hiding this comment.
You are right - I left some unused imports, I would fix it
| /** | ||
| * Retrieves the first typed query parameter value having the specified name. | ||
| */ | ||
| def getAs[A](key: String)(implicit codec: TextCodec[A]): Either[QueryParamsError, A] = for { |
There was a problem hiding this comment.
I am wondering, if a def getAsZIO[A](key: String)(implicit codec: TextCodec[A]): IO[QueryParamsError, A] makes sense too.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for explaining :) You are right, I will add additional functions
| def message: String | ||
| } | ||
| object QueryParamsError { | ||
| final case class Missing(queryParamName: String) extends QueryParamsError { |
There was a problem hiding this comment.
Missing#queryParamName seems inconsistent with Malformed#name.
| 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 { |
There was a problem hiding this comment.
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 QueryParamsErrorThere was a problem hiding this comment.
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?
There was a problem hiding this comment.
👍 NonEmptyChunk could give more guarantee here.
| } | ||
|
|
||
| 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" |
There was a problem hiding this comment.
I doubt that TextCodec always provides stable toString. codec.describe might be more reliable choice.
Add getAs[A] for QueryParams
Closes #2423