lagom / lagom

Reactive Microservices for the JVM
https://www.lagomframework.com
Apache License 2.0
2.63k stars 634 forks source link

uri param type incorrect matching/ parsing causing invalid error code in api response #2783

Open gurudatta11 opened 4 years ago

gurudatta11 commented 4 years ago

Lagom Version (1.2.x / 1.3.x / etc)

Current

API (Scala / Java / Neither / Both)

Checking in Scala

Operating System (Ubuntu 15.10 / MacOS 10.10 / Windows 10)

All

JDK (Oracle 1.8.0_112, OpenJDK 1.8.x, Azul Zing)

java version "1.8.0_181"

Library Dependencies

None

Expected Behavior

For any HTTP method type, in a url => http://{{ip}}/api/v1/hello/{id} Here {id} is of Long type.

On invoking of http://{{ip}}/api/v1/hello/12s

  1. It should return 404 error code, instead of 500 error code

Actual Behavior

For any HTTP method type, in a url => http://{{ip}}/api/v1/hello/{id} Here {id} is of Long type.

If I give string type here, http://{{ip}}/api/v1/hello/12s it gets 500 instead of 404. For Long it works fine.

Similarly for other type combinations

  1. Get below response from api, with 500 error code =>
    Execution exception
    [NumberFormatException: For input string: "12s"]
  2. Detailed logs from application logs =>
    Caused by: java.lang.NumberFormatException: For input string: "12s"
    at java.lang.NumberFormatException.forInputString(NumberFormatException.java:65)
    at java.lang.Long.parseLong(Long.java:589)
    at java.lang.Long.parseLong(Long.java:631)
    at scala.collection.immutable.StringLike.toLong(StringLike.scala:309)
    at scala.collection.immutable.StringLike.toLong$(StringLike.scala:309)
    at scala.collection.immutable.StringOps.toLong(StringOps.scala:33)
    at com.lightbend.lagom.scaladsl.api.deser.DefaultPathParamSerializers.$anonfun$LongPathParamSerializer$1(PathParamSerializer.scala:64)
    at com.lightbend.lagom.scaladsl.api.deser.DefaultPathParamSerializers.$anonfun$LongPathParamSerializer$1$adapted(PathParamSerializer.scala:64)
    at com.lightbend.lagom.scaladsl.api.deser.DefaultPathParamSerializers$$anon$1.deserialize(PathParamSerializer.scala:51)
    at com.lightbend.lagom.internal.scaladsl.server.ScaladslServiceRouter$ScaladslServiceRoute.$anonfun$createServiceCall$1(ScaladslServiceRouter.scala:61)
  3. Error points to https://github.com/lagom/lagom/blob/master/service/scaladsl/api/src/main/scala/com/lightbend/lagom/scaladsl/api/deser/PathParamSerializer.scala#L64

Reproducible Test Case

Easy to reproduce

ihostage commented 4 years ago

You need to change a type of path parameter to String and implement a validation code on the service implementation. Or write a custom PathParamSerializer.

gurudatta11 commented 4 years ago

that seems cumbersome, redundant, most of the path variables in REST will be numbers,

putting a wrapper while converting it to given types(when path is found) should suffice as fix

ihostage commented 4 years ago

most of the path variables in REST will be numbers,

very controversial statement. 🤔 In our projects, most variables are String.

putting a wrapper while converting it to given types(when path is found) should suffice as fix

You are right. You can create a LongWrapper and implement a PathParamSerializer for this type.

gurudatta11 commented 4 years ago

in akka Http, spring boot java , this gives a 404, instead of 500, because the path is invalid (since route is not defined in given project)

I didn't had a chance to check with play though,

But was checking if lagom itself can offer improvised route/path checks based on the types defined in the application.

It would reduce redundant wrapping in application code.

ihostage commented 4 years ago

If you really want a 404 response, you need to use a regexp for this route. Then Play router will not find action for nonmatch URL and return 404 code. For example in your case:

pathCall("/api/v1/hello/$id<[0-9]+>", getHello _)
ihostage commented 4 years ago

@gurudatta11 Can I close this issue?

ignasi35 commented 4 years ago

most of the path variables in REST will be numbers,

Lagom is not a REST framework. Lagom is a microservices framework with a Service API where you can describe a public API. Lagom provides an implementation of such API using HTTP and JSON as a default serialization format. Lagom provides client tooling to easily invoke a remote Lagom service with close to no coding involved. That client will be strongly typed so the case you are describing in which the server expects a Long but the client sends a String is not possible.

As a side effect from this design decision, some exception handling may be unexpected for certain use cases but, as @ihostage hinted, there usually is a way out. I agree that the alternative may not always be very user friendly or, as you put it, cumbersome.