@RouteParam is not parsed on get requests

[ashald]

@RouteParam is not parsed on get requests.

Expected behavior

If a case class with @RouteParam field used for request, the value is extracted from the URI into the request case class instance.

Actual behavior

Field is not populated

Steps to reproduce the behavior

Finatra 18.2.0

case class AccessCursorRequest(@RouteParam id: String)

class ProbeController @Inject()(
  facade: ProbeFacade
) extends Controller
  with Logging {

get("/cursor/:id/read") { request: AccessCursorRequest =>
    logger.info(s"Value: <${request.id}>")
  }

}

And then:

$ curl "http://localhost:8888/cursor/foobar/read"

In logs:

finagle/netty4-1 INFO  n.a.c.ProbeController - Value: <>

[cacoco]

@Ashald I think you are missing a $ in your code. E.g, your log statement should be:

logger.info(s"Value: '${request.id}'")

See: https://docs.scala-lang.org/overviews/core/string-interpolation.html

[ashald]

@cacoco that's just a typo when I was transferring code from my project into the issue. I simplified it a bit. My code does access the field properly. Also, if that was the case we would see

finagle/netty4-1 INFO  n.a.c.ProbeController - Value: '{request.id}'

rather than

finagle/netty4-1 INFO  n.a.c.ProbeController - Value: ''

in logs.

Please re-open the issue.

[cacoco]

@Ashald thanks for more info. Can you update the code in the steps to reproduce? We obviously have a ton of tests in this area which are working properly so I am inclined to believe it is something simple.

Thanks!

[cacoco]

Ah, I know the problem. See: https://twitter.github.io/finatra/user-guide/http/requests.html#custom-request-case-class

Specifically:

Custom “request” case classes can be used for declarative parsing of requests with a content-type of application/json with support for type conversions, default values, and validations.

You need to pass an "application/json" content-type header in your curl command. Using a case class is generally meant to be when you are sending a body that you would like automatically parsed into an object. Using it solely for a @RouteParam is fine but you still need to send the "application/json" content-type header.

In generally we really recommend you test your code through Feature Tests where small things like this are less likely to happen when using the tooling provided.

[ashald]

Supplying content-type on GET requests is cumbersome, especially if you expect that users will try API in browsers. So this ticket probably should be treated more like a question: do you think we can make an option re lift the requirement for specific content-type for this to work?

[cacoco]

@Ashald I think this is difference of intent. For this case, you are somewhat misusing the functionality and I would not suggest using a case class and an @RouteParam. You can simply parse the value from the incoming request param map. Using a case class here does nothing but add the burden of the needing to specify a content-type header.

Eg.,

import com.twitter.finagle.http.Request

class ProbeController @Inject()(
  facade: ProbeFacade
) extends Controller
  with Logging {

get("/cursor/:id/read") { request: Request =>
    logger.info(s"Value: `${request.getParam("id")}`")
  }
}

would be the expected way to do this. See here for more examples.

As I mentioned the intent of using a case class is generally to parse an incoming request body that is assumed to be JSON into the case class object. The @RouteParam is a convenience be able to also pull params from the path when doing so (same with @QueryParam for query parameters and @FormParam for form parameters). Using them alone in a case class buys you almost nothing and incurs the setting of the content-type cost.

Thanks!

[ashald]

Well, it feels like the fact that there is a way to mix route params with "body"/"query" params is some sort of abstraction leak according to the intended usage you mentioned. My expectation was that using case classes would help with type coercion and validation.

Anyway, from my personal point of view it's a huge disappointment that I cannot use pattern matching with Request case classes in order to extract parameters from the request. But if it's not how it was indented to use then there is nothing we can do within this issue so I'm going to close it.