Implmented multiparts support for netty #2560

Pask423 · 2022-11-10T11:07:41Z

No description provided.

Pask423 · 2022-11-10T11:49:45Z

Fourth task in #499

adamw · 2022-11-10T11:53:05Z

server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/SequenceSupport.scala

+import sttp.monad.syntax._
+
+// FIXME: Move to sttp-shared after zio 2.0.3 start working ?
+class SequenceSupport[F[_]](implicit me: MonadError[F]) {


let's maybe add this as an implicit value class in sttp.tapir.internal?

adamw · 2022-11-10T11:53:35Z

server/netty-server/cats/src/test/scala/sttp/tapir/server/netty/cats/NettyCatsServerTest.scala

@@ -20,7 +20,8 @@ class NettyCatsServerTest extends TestSuite with EitherValues {
          val interpreter = new NettyCatsTestServerInterpreter(eventLoopGroup, dispatcher)
          val createServerTest = new DefaultCreateServerTest(backend, interpreter)

-          val tests = new AllServerTests(createServerTest, interpreter, backend, staticContent = false, multipart = false).tests()
+          val tests = new AllServerTests(createServerTest, interpreter, backend, staticContent = false, multipart = false).tests() ++
+            new ServerMultipartTests(createServerTest, partOtherHeaderSupport = false).tests()


can we support the additional part headers? seems this might be a matter of adding the headers when serialising the part?

As far I as check there is no way to get those headers while parsing

hm it would be weird if the data just got lost

I will check it once more

After some more digging I cannot see any way to get parts headers from netty - they are not included in overall request headers nor in HttpDataType's used in getParts method.

yeah it seems so; so we'll have to leave this as is, thanks for checking

adamw · 2022-11-10T11:54:29Z

server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/NettyRequestBody.scala

+      case RawBodyType.FileBody =>
+        createFile(serverRequest)
+          .map(file => {
+            Files.write(file.toPath, ByteBufUtil.getBytes(upload.content()))


doesn't this read the whole file content into memory?

I think yes

well that's not good then. We need to transfer these bytes without reading the content into memory; and if there's any blocking code, surround this with monadError.blocking

As far as I understand we could extract the package-private method readBytes from netty (https://github.com/netty/netty/blob/4.1/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java#L1878) and use it here so, that bytes from ByteBuffer could be written to FileOutputStream, depending on the size it is done directly or chunk by chunk.

adamw · 2022-11-10T11:55:53Z

server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/NettyToResponseBody.scala

+        toPart(part.body, part.contentType, part.name, None)
+      case RawBodyType.FileBody =>
+        val fileRange = part.body.asInstanceOf[FileRange]
+        toPart(Files.readString(fileRange.file.toPath), part.contentType, part.name, Some(fileRange.file.getName))


again, this reads the whole file content into memory - can we provide the data to netty in another way? @rafalambrozewicz you did sth similar, maybe you'll be able to help :)

I am not sure if it is possible to do in other way but any help would be welcome.

We've checked whether netty has some support for multipart out of the box, but it looks like it doesn't. Not reading everything into memory forces usage of Chunked class i.e. io.netty.handler.streamChunkedStream, but turning all parts of multipart into such entity seems not trivial or simply not possible :-|

can't we generate a list of chunks, ChunkedInputs or a list of buffers, and call .write multiple times in NettyServerHandler?

adamw · 2022-11-10T11:56:46Z

server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/NettyToResponseBody.scala

+      s"""
+      $boundary
+          $contentTypeStr
+          Content-Disposition: form-data; $fileNameStr name="$name"


I think here we should be able to add more headers ... however, maybe the names/file names would need some escaping?

If i will find the way to read those headers from request then no problem.

rafalambrozewicz · 2022-11-10T12:19:21Z

server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/NettyRequestBody.scala

+    }
+  }
+
+  private def getParts(serverRequest: ServerRequest, m: RawBodyType.MultipartBody): List[F[Part[Any]]] = {


Consider placing nettyRequest function before getParts to match the order of appearance in toRaw and make it easier to read.

rafalambrozewicz · 2022-11-10T12:26:43Z

server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/NettyToResponseBody.scala

+    }
+  }
+
+  private def convertToBuffs(bodyType: RawBodyType[_], part: Part[Any]): ByteBuf = {


Consider placing convertToBuffs after wraps, so that this file is easier to read.

adamw · 2022-11-15T15:00:01Z

core/src/main/scala/sttp/tapir/internal/SequenceSupport.scala

+import sttp.monad.MonadError
+import sttp.monad.syntax._
+
+class SequenceSupport[F[_]](implicit me: MonadError[F]) {


just add this as an implicit class to the internal package object :)

adamw · 2022-11-15T15:07:05Z

server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/NettyRequestBody.scala

  private def nettyRequest(serverRequest: ServerRequest): FullHttpRequest = serverRequest.underlying.asInstanceOf[FullHttpRequest]
+
+  private def getParts(serverRequest: ServerRequest, m: RawBodyType.MultipartBody): List[F[Part[Any]]] = {
+    new HttpPostMultipartRequestDecoder(nettyRequest(serverRequest)).getBodyHttpDatas.asScala


javadocs for this class:

/** * This decoder will decode Body and can handle POST BODY. * * You <strong>MUST</strong> call {@link #destroy()} after completion to release all resources. * */

are we? :)

adamw · 2022-11-15T15:08:43Z

server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/NettyRequestBody.scala

+      .flatMap(httpData =>
+        httpData.getHttpDataType match {
+          case HttpDataType.Attribute =>
+            val part: F[Part[Any]] = monadError.unit(Part(name = httpData.getName, body = httpData.asInstanceOf[Attribute].getValue))


shouldn't we inspect the rawbodytype here as well, and convert the String attribute accordingly?

I am not sure, httpData.asInstanceOf[Attribute].getValue will return String no matter the body type and I do not know how losing information about the original body type will affect our flow.
Moreover it is a possibility that string body will be the only bodyType ever handle by Attribute as for example ByteArrayBody or FileBody are mark as FileUpload type

they are? I though it's up to netty to decide if a part is an attribute, or a file body?

adamw · 2022-11-15T15:14:59Z

server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/NettyToResponseBody.scala

+        toPart(part.body, part.contentType, part.name, None)
+      case RawBodyType.FileBody =>
+        val fileRange = part.body.asInstanceOf[FileRange]
+        toPart(Files.readString(fileRange.file.toPath), part.contentType, part.name, Some(fileRange.file.getName))


Here the same, we can't read the file into memory

As far as I understand we would need to introduce our own ChunkedMultipart data structure. It needs to be constructed by passing a list of ByteBufs with small content and multipart parts' like headers and footers, and chunked streams and chunked files. And it needs to behave like ChunkedFile or ChunkedStream i.e. to implement ChunkedInput<ByteBuf>, a way to treat all the parts as something homogeneous, that could be read as a one whole. At the first glance, it seems to be quite tricky.

adamw · 2022-11-29T12:32:07Z

server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/NettyRequestBody.scala

-      case _: RawBodyType.MultipartBody => ???
+      case m: RawBodyType.MultipartBody =>
+        monadError
+          .unit(new HttpPostMultipartRequestDecoder(nettyRequest(serverRequest)))


there's a .unit syntax; or, if this can throw an exception, we should use .eval

adamw · 2022-11-29T12:32:30Z

server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/NettyRequestBody.scala

+              .sequence()
+              .map(RawValue.fromParts)
+              .map(a => {
+                decoder.destroy()


I think this should be in the "finally" caluse using .ensure

adamw · 2022-11-29T12:32:51Z

server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/NettyRequestBody.scala

+          case HttpDataType.FileUpload =>
+            m.partType(httpData.getName)
+              .map(c => {
+                val upload = httpData.asInstanceOf[FileUpload]


why do we cast to FileUpload if we just need a HttpData?

adamw · 2022-11-29T12:34:00Z

server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/NettyRequestBody.scala

+    val mediaType = contentType.flatMap(c => MediaType.parse(c).toOption)
+    m match {
+      case RawBodyType.StringBody(charset) =>
+        monadError.unit(Part(name = upload.getName, body = upload.getString(charset), contentType = mediaType))


I think we need two variants, for parts which are in-memory and file-based (as reported by netty). If they are in memory, then this should be a .unit or monad.eval(...) (depending if exceptions are possible).

If these are in a file, then these should be .blocking, as reading from the file is done in a blocking way?

adamw · 2022-11-29T12:34:59Z

server/netty-server/src/main/scala/sttp/tapir/server/netty/internal/NettyRequestBody.scala

+      case RawBodyType.FileBody =>
+        createFile(serverRequest)
+          .map(file => {
+            upload.renameTo(file)


this definitely should be in a monad.blocking call

or maybe, there's a way to configure how Netty creates temporary files? We could use then whatever is provided in the options and bypass this step entirely

max-peroch · 2023-07-14T15:36:57Z

Thank you for your work on this <3

adamw · 2024-04-03T08:20:25Z

Closing old PRs

bartlomiej.zylinski added 5 commits November 9, 2022 12:33

Attempt to implement mulitpart support in netty

0fba0d7

Fix netty cats

3a89bcf

Implement monadError sequence

d60613b

Fix contentType handling

ce417ae

Fix compilation issues for 2.12

c9a4f68

Pask423 requested review from rafalambrozewicz and adamw November 10, 2022 11:51

adamw reviewed Nov 10, 2022

View reviewed changes

rafalambrozewicz reviewed Nov 10, 2022

View reviewed changes

Fix review issues

a017514

adamw reviewed Nov 15, 2022

View reviewed changes

bartlomiej.zylinski added 2 commits November 21, 2022 11:28

Fix support for HttpDataTypes

07e4642

Attempt to handle file without putting it in memory

f893481

adamw reviewed Nov 29, 2022

View reviewed changes

adamw closed this Apr 3, 2024

Pask423 deleted the feature/netty-multiparts branch May 7, 2024 07:14

Implmented multiparts support for netty #2560

Implmented multiparts support for netty #2560

Conversation

Pask423 commented Nov 10, 2022

Pask423 commented Nov 10, 2022

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

Pask423 Nov 14, 2022 • 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

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

Pask423 Nov 18, 2022 • 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

adamw Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

max-peroch commented Jul 14, 2023

adamw commented Apr 3, 2024

Pask423 Nov 14, 2022 •

edited

Loading

Pask423 Nov 18, 2022 •

edited

Loading

adamw Nov 29, 2022 •

edited

Loading