finagle / finch

Scala combinator library for building Finagle HTTP services
https://finagle.github.io/finch
Apache License 2.0
1.6k stars 220 forks source link

Allow users override maxInMemoryFileSize in multipartFileUpload #1087

Open vkostyukov opened 5 years ago

vkostyukov commented 5 years ago

It was asked on Gitter. Here is the argument we need to wire: https://github.com/twitter/finagle/blob/develop/finagle-base-http/src/main/scala/com/twitter/finagle/http/exp/MultipartDecoder.scala#L20

hderms commented 5 years ago

@vkostyukov I started working on this one but I think some guidance on how you want the API to look would be useful. As of right now I see three possible high level ways to make this change:

  1. either add default values to API methods https://github.com/hderms/finch/blob/master/core/src/main/scala/io/finch/EndpointModule.scala#L367 or adding additional method signatures (i.e. clone each api method dealing with multipart uploads and add a StorageUnit argument). Imo this is ugly but simple to understand. Also simple to deprecate particular variants of the API methods if we went with the additional method signatures approach.

  2. alternatively I could add a method like withMaxInMemoryUploadSize to https://github.com/hderms/finch/blob/master/core/src/main/scala/io/finch/endpoint/multipart.scala#L103 which would make it a concern to the caller only if they specifically need to adjust the size. This seems undesirable based on the rest of the design of Finch

  3. Some kind of implicit parameter which you could provide your own instance of, which seems like a lot of machinery and doesn't seem correct from a design perspective

If you could push me in the right direction that would be appreciated. Thanks