Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

formField fails if custom unmarshaller is in scope #541

Open
marekzebrowski opened this issue Nov 17, 2016 · 7 comments
Open

formField fails if custom unmarshaller is in scope #541

marekzebrowski opened this issue Nov 17, 2016 · 7 comments
Labels
1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted help wanted Identifies issues that the core team will likely not have time to work on t:docs Issues related to the documentation
Milestone

Comments

@marekzebrowski
Copy link

formField directive fails if there is too generic unmarshaller in scope, that prevents FieldMagnet machinery to work.
It can happen quite a lot - for example when using akka-http-json4s or other generic unmarshaller.

Reproducer https://github.com/marekzebrowski/akka-http-form-field-bug

If it is not solvable, maybe a word of warning in documentation, or a guide how to write own unmarshaller would be helpful

@ktoso ktoso added help wanted Identifies issues that the core team will likely not have time to work on t:docs Issues related to the documentation 1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted labels Nov 17, 2016
@ktoso
Copy link
Member

ktoso commented Nov 17, 2016

Yeah sounds like a docs thing, thanks for reporting.
Up for grabs if someone has the time! :)

@ktoso ktoso added this to the backlog milestone Nov 17, 2016
@marekzebrowski
Copy link
Author

I wonder if it is a bug. Defined marshaller should work only for application/json content-type. Request in test-case is application/x-www-form-urlencoded. If it is not a bug, it is at least confusing

@silvaren
Copy link

Not sure sure if this is the same issue, but it looks very similar. When using circe + akka-http-json support for JSON (which I believe provides generic unmarshallers to the scope) as in:

  private def route(implicit mat: Materializer) = {
    import Directives._
    import de.heikoseeberger.akkahttpcirce.FailFastCirceSupport._
    import io.circe.generic.auto._

    pathSingleSlash {
      post {
        entity(as[Foo]) { foo =>
          complete {
            foo
          }
        }
      }
    } ~
      path("formurlencoded") {
        post {
          formFields('coolKey) { coolKey =>
            complete(s"The cool value is '${coolKey}'")
          }
        }
      }
  }

If I run and test the second route:
curl -d "coolKey=coolValue" -X POST http://localhost:8000/formurlencoded

I get the following error:
The request's Content-Type is not supported. Expected: application/json or multipart/form-data

But if I remove the JSON support it works correctly. Full issue on akka-http-json, but it might be actually an Akka HTTP issue.

@jrudolph
Copy link
Member

Yes, it's an anti-pattern to bring generic implicits into scope that are able to marshal/unmarshal everything. When you think about it, it is somewhat obvious that this cannot work. So, the recommendation is to never provide something like

def unmarshaller[T]: FromEntityUnmarshaller[T]

where T is basically unconstrained.

@jrudolph jrudolph added bug and removed bug labels Jun 12, 2017
@jrudolph
Copy link
Member

I think we can still improve the situation for formField directives. Right now the FieldMagnet machinery expects implicit FromEntityUnmarshaller[StrictForm] everywhere. Keeping it implicit allows you to override what kind of content the formField directive is able to interpret as a form field and how to parse it. In reality no one will actually override it because the formField directive is thought to be used with multipart/formdata and application/x-www-form-urlencoded payloads. I think we should deprecate the existing implicits and move them into a superclass and provide implicits that don't require that particular implicit (but use a concrete unmarshaller instead).

jrudolph added a commit to jrudolph/akka-http that referenced this issue Jun 12, 2017
…rm]` in FieldMagnets

The reasoning is that probably all cases that implicit will be the same. Previous
code allowed that implicit to be customized by users to use `formField` with other
payloads than `multipart/formdata` and `application/x-www-form-urlencoded`. This
isn't really useful and makes `formField` proner to implicit resolution problems.

The old implicit are kept for now to keep binary compatibility but can be
removed later.
jrudolph added a commit to jrudolph/akka-http that referenced this issue Jun 12, 2017
jrudolph added a commit to jrudolph/akka-http that referenced this issue Oct 24, 2017
…rm]` in FieldMagnets

The reasoning is that probably all cases that implicit will be the same. Previous
code allowed that implicit to be customized by users to use `formField` with other
payloads than `multipart/formdata` and `application/x-www-form-urlencoded`. This
isn't really useful and makes `formField` proner to implicit resolution problems.

The old implicit are kept for now to keep binary compatibility but can be
removed later.
jrudolph added a commit to jrudolph/akka-http that referenced this issue Oct 24, 2017
jrudolph added a commit that referenced this issue Oct 24, 2017
…rshalling

Simplify formField implicits a bit #541
@koertkuipers
Copy link

Yes, it's an anti-pattern to bring generic implicits into scope that are able to marshal/unmarshal everything. When you think about it, it is somewhat obvious that this cannot work. So, the recommendation is to never provide something like

def unmarshaller[T]: FromEntityUnmarshaller[T]

where T is basically unconstrained.

is is not clear to me how we can add an unmarshaller for type T then? to add support for another content type without disabling existing content types?
if we want to support multiple unmarshallers for a type T, which to me is a reasonable thing to want to do, is the problem that this cannot be captured in the current design using a typeclass for T?

@jrudolph
Copy link
Member

The original ticket was about a generic unmarshaller. You cannot bring an unmarshaller into scope that claims to unmarshal everything, this will conflict with other unmarshallers.

And yes, to some degree the typeclass pattern cannot solve the case where you have several different unmarshallers in scope for a single concrete type. The implicit / typeclass pattern must use static information to figure out a single instance.

That said, if you want to support multiple content types for a single concrete target type, you can create a composite Unmarshaller that supports all those content types using Unmarshaller.firstOf and make only that one implicit (or bring only that one into scope). Is that what you are after?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted help wanted Identifies issues that the core team will likely not have time to work on t:docs Issues related to the documentation
Projects
None yet
Development

No branches or pull requests

5 participants