Special support for byte stream on endpoint api in/out #2145
Conversation
vigoo
requested review from
amitksingh1490,
jdegoes and
adamgfraser
as code owners
| @@ -24,6 +24,6 @@ private[codec] trait ContentCodecs { | |||
| def content[A](implicit schema: Schema[A]): ContentCodec[A] = | |||
| HttpCodec.Content(schema) | |||
|
|
|||
| def contentStream[A](implicit schema: Schema[A]): ContentCodec[ZStream[Any, Nothing, A]] = | |||
| def contentStream[A](implicit schema: Schema[A]): ContentCodec[ZStream[Any, Throwable, A]] = | |||
There was a problem hiding this comment.
Maybe this should not be changed and the input stream's errors should be handled somehow before passing the stream to the implementation, but I did not see an obvious place to do that.
There was a problem hiding this comment.
This should definitely not be changed.
The reason is that an endpoint handler cannot deal with Throwable errors. It only knows about typed errors, which are part of the protocol itself. Throwable errors indicate something fatal happened: for example, that an element of the stream was improperly encoded. No recovery is possible at the level of the endpoint handler. So it's a defect and must be handled as a fallback higher up (request handler, HTTP).
| Headers(Header.ContentType(MediaType.application.json)) | ||
| } else throw new IllegalStateException("A request on a REST endpoint should have at most one body") | ||
| if (isByteStream(flattened.content)) { | ||
| Headers(Header.ContentType(MediaType.application.`octet-stream`)) // TODO: customizable content type |
There was a problem hiding this comment.
We should be able to attach a custom media type to the body codec somehow, not sure if it should be part of this PR or not
There was a problem hiding this comment.
I think it should be part of this PR. Basically, store an Option[MediaType] inside the codec, and add variants that allow it to be specified (overridden) when the content codec is specified.
| @@ -517,13 +517,13 @@ object HttpCodec | |||
| def withIndex(index: Int): Content[A] = copy(index = index) | |||
| } | |||
| private[http] final case class ContentStream[A](schema: Schema[A], index: Int = 0) | |||
| extends Atom[HttpCodecType.Content, ZStream[Any, Nothing, A]] { | |||
| extends Atom[HttpCodecType.Content, ZStream[Any, Throwable, A]] { | |||
There was a problem hiding this comment.
Same here, this should be Nothing and we should orDie on throwables.
| if (jsonEncoders.length == 0) Headers.empty | ||
| else if (jsonEncoders.length == 1) { | ||
| Headers(Header.ContentType(MediaType.application.json)) | ||
| } else throw new IllegalStateException("A request on a REST endpoint should have at most one body") |
There was a problem hiding this comment.
This, btw, is where the multi-part stuff would go.
|
|
||
| private def isByteStream(codecs: Chunk[BodyCodec[_]]): Boolean = | ||
| if (codecs.length == 1) { | ||
| codecs.headOption match { |
There was a problem hiding this comment.
headOption boxes. You already know there is one, so do codecs(0).
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #2145 +/- ##
==========================================
- Coverage 66.87% 66.73% -0.14%
==========================================
Files 141 141
Lines 6040 6268 +228
Branches 256 259 +3
==========================================
+ Hits 4039 4183 +144
- Misses 2001 2085 +84
☔ View full report in Codecov by Sentry. |
Resolves #2141