softwaremill / tapir

Rapid development of self-documenting APIs
https://tapir.softwaremill.com
Apache License 2.0
1.34k stars 405 forks source link

[BUG] File in multipart request is saved on disk and not deleted #2468

Open desavitsky opened 1 year ago

desavitsky commented 1 year ago

Tapir version: zio-vertx-1.1.1

Scala version: 2.13.8

Files regardless of their size are saved on disk and not deleted after request is completed. Had to delete them manually They are saved in folder in project root ./file-uploads

code to reproduce:

import io.vertx.core.Vertx
import io.vertx.ext.web.Router
import sttp.model.Part
import sttp.tapir.generic.auto._
import sttp.tapir.plainBody
import sttp.tapir.server.vertx.zio.VertxZioServerInterpreter
import sttp.tapir.server.vertx.zio.VertxZioServerInterpreter._
import sttp.tapir.ztapir._
import zio._

object Short extends zio.App {
  implicit val runtime = zio.Runtime.default

  case class TestRequest(test: String, file: Part[File])
  val responseEndpoint =
    endpoint
      .in("test")
      .in(multipartBody[TestRequest])
      .out(plainBody[String])

  val attach = VertxZioServerInterpreter[ZEnv]().route(responseEndpoint.zServerLogic(req => ZIO.succeed(req.test)))

  def run(args: List[String]): URIO[ZEnv, ExitCode] =
    ZManaged
      .make(ZIO.effect {
        val vertx  = Vertx.vertx()
        val server = vertx.createHttpServer()
        val router = Router.router(vertx)
        attach(router)
        server.requestHandler(router).listen(9099)
      } flatMap (_.asRIO)) { server =>
        ZIO.effect(server.close()).flatMap(_.asRIO).orDie
      }
      .useForever
      .exitCode

}
adamw commented 1 year ago

Currently multipart upload files are deleted when there's a decode failure on the body: https://github.com/softwaremill/tapir/blob/c3f45cbbd38830eb41c41cddde0ca8f283c6efb5/server/core/src/main/scala/sttp/tapir/server/interpreter/ServerInterpreter.scala#L173-L176

Arguably this could be extended so that the files are deleted if there is a decode failure (e.g. an exception during a .map) at a later stage as well.

However, so far the assumption was that when we invoke the server logic, it's up to the user to decide what to do with those files - whether to delete them, keep them, move, etc. I'm not sure if deleting is a safe default - I guess people might rely on these file staying in their original locations after the request completes?

desavitsky commented 1 year ago

Well, that's possible that people might rely on these files, but kinda strange From my point of view, the thing that file is written to disc is the internal part of backend processing the multipart request, and it should not be the part of the logic Maybe it is possible to allow users to customize it? For example, add extra field to VertxZioServerOptions vertxHandlerOptions and let users change it if they need it?

desavitsky commented 1 year ago

Also, I think it is needed to be added to docs, because this behaviour was a surprise for me:)