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

Improves error messages in path param serializer resolution #1093

Merged
merged 5 commits into from Nov 11, 2017

Conversation

Projects
None yet
5 participants
@silver-soule
Copy link
Contributor

commented Nov 7, 2017

  1. Added a wrapping exception for IllegalArgumentException with a detailed error message
  2. changed "ID" to "path parameter" in IllegalArgumentException error message

Pull Request Checklist

Fixes

Fixes #1084

Purpose

What does this PR do?
Provides more information for certain run time exceptions in a lagom based java service

Background Context

Why did you take this approach?

References

Are there any relevant issues / PRs / mailing lists discussions?

1. Added a wrapping exception for IllegalArgumentException with a det…
…ailed error message

2. changed "ID" to "path parameter" in IllegalArgumentException error message

@ignasi35 ignasi35 changed the title 1. Added a wrapping exception for IllegalArgumentException with a det… [WIP] Improves path param serializer resolving error messages Nov 7, 2017

@ignasi35

This comment has been minimized.

Copy link
Member

commented Nov 7, 2017

Hi @silver-soule I've renamed the PR and added a reference on the PR description to the original issue so that when we merge this PR, the issue will be closed.

} catch {
case ex: IllegalArgumentException =>
throw new RuntimeException(s"Error encountered while resolving the ${method.getDeclaringClass + "." + method.getName}" +
"service call: No path parameter serializer was found for the com.example.Foo path parameter. This can be fixed " +

This comment has been minimized.

Copy link
@ignasi35

ignasi35 Nov 7, 2017

Member

Is com.example.Foo an oversight?

This comment has been minimized.

Copy link
@silver-soule

silver-soule Nov 7, 2017

Author Contributor

yes, it seems I missed it. I'll fix it asap.

This comment has been minimized.

Copy link
@silver-soule

silver-soule Nov 7, 2017

Author Contributor

@ignasi35 I updated the error message with correct value

serviceCallResolver.resolvePathParamSerializer(new UnresolvedTypePathParamSerializer[AnyRef], arg)
} catch {
case ex: IllegalArgumentException =>
throw new RuntimeException(s"Error encountered while resolving the ${method.getDeclaringClass + "." + method.getName}" +

This comment has been minimized.

Copy link
@ignasi35

ignasi35 Nov 8, 2017

Member

I think this is changing the behaviour of the code: what used to throw an IllegalArgumentException now throws a RuntimeException. The fact that the the build in Travis passed makes me think we're missing tests for failure cases.

This comment has been minimized.

Copy link
@silver-soule

silver-soule Nov 8, 2017

Author Contributor

@ignasi35 Yes, it seems the test case was missing. I've added it.

This comment has been minimized.

Copy link
@renatocaval

renatocaval Nov 9, 2017

Member

IllegalArgumentException is a RuntimeException and we were throwing it before. We should keep it as it was.
We can still catch it to add a more meaning message, but we should keep it as IllegalArgumentException.

This comment has been minimized.

Copy link
@silver-soule

silver-soule Nov 9, 2017

Author Contributor

@renatocaval Ok I changed it to IllegalArgumentException as suggested.

Added test cases to check the error thrown if non-serializable path p…
…arameters are given in a lagom java ServiceCall
@ignasi35

This comment has been minimized.

Copy link
Member

commented Nov 9, 2017

The Travis build failed on an unrelated issue during scripted tests:


[info] 18:13:09.767 [error] play.core.server.NettyServer [] - Failed to listen for HTTP on /0.0.0.0:53240!
[info] play.core.server.ServerListenException: Failed to listen for HTTP on /0.0.0.0:53240!
[info] 	at play.core.server.NettyServer.play$core$server$NettyServer$$bindChannel(NettyServer.scala:219)
[info] 	at play.core.server.NettyServer$$anonfun$1.apply(NettyServer.scala:207)
[info] 	at play.core.server.NettyServer$$anonfun$1.apply(NettyServer.scala:207)
[info] 	at scala.Option.map(Option.scala:146)
[info] 	at play.core.server.NettyServer.<init>(NettyServer.scala:207)
[info] 	at play.core.server.NettyServerProvider.createServer(NettyServer.scala:270)
[info] 	at play.core.server.NettyServerProvider.createServer(NettyServer.scala:269)
[info] 	at play.core.server.LagomReloadableDevServerStart$$anonfun$mainDev$1.apply(LagomReloadableDevServerStart.scala:230)
[info] 	at play.core.server.LagomReloadableDevServerStart$$anonfun$mainDev$1.apply(LagomReloadableDevServerStart.scala:62)
[info] 	at play.utils.Threads$.withContextClassLoader(Threads.scala:21)
[info] 	at play.core.server.LagomReloadableDevServerStart$.mainDev(LagomReloadableDevServerStart.scala:61)
[info] 	at play.core.server.LagomReloadableDevServerStart$.mainDevHttpMode(LagomReloadableDevServerStart.scala:51)
[info] 	at play.core.server.LagomReloadableDevServerStart.mainDevHttpMode(LagomReloadableDevServerStart.scala)
[info] 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[info] 	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
[info] 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[info] 	at java.lang.reflect.Method.invoke(Method.java:498)
[info] 	at com.lightbend.lagom.dev.Reloader$.startDevMode(Reloader.scala:126)
[info] 	at com.lightbend.lagom.sbt.run.RunSupport$$anonfun$reloadRunTask$1.apply(RunSupport.scala:33)
[info] 	at com.lightbend.lagom.sbt.run.RunSupport$$anonfun$reloadRunTask$1.apply(RunSupport.scala:20)
[info] 	at scala.Function1$$anonfun$compose$1.apply(Function1.scala:47)
[info] 	at sbt.$tilde$greater$$anonfun$$u2219$1.apply(TypeFunctions.scala:40)
[info] 	at sbt.std.Transform$$anon$4.work(System.scala:63)
[info] 	at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:228)
[info] 	at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:228)
[info] 	at sbt.ErrorHandling$.wideConvert(ErrorHandling.scala:17)
[info] 	at sbt.Execute.work(Execute.scala:237)
[info] 	at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:228)
[info] 	at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:228)
[info] 	at sbt.ConcurrentRestrictions$$anon$4$$anonfun$1.apply(ConcurrentRestrictions.scala:159)
[info] 	at sbt.CompletionService$$anon$2.call(CompletionService.scala:28)
[info] 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[info] 	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
[info] 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[info] 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
[info] 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
[info] 	at java.lang.Thread.run(Thread.java:748)
[info] [error] (my-project/*:lagomRun) java.lang.reflect.InvocationTargetException
[info] [error] Total time: 6 s, completed Nov 8, 2017 6:13:09 PM
[info] [INFO] [11/08/2017 18:13:09.848] [my-project-internal-dev-mode-shutdown-hook-1] [CoordinatedShutdown(akka://my-project-internal-dev-mode)] Starting coordinated shutdown from JVM shutdown hook
[error] x sbt-plugin / injected-configs
[error]    {line 16}  Command failed: run failed

@ignasi35 ignasi35 requested review from TimMoore and renatocaval Nov 9, 2017

@ignasi35 ignasi35 changed the title [WIP] Improves path param serializer resolving error messages Improves error messages in path param serializer resolution Nov 9, 2017

@ignasi35 ignasi35 added this to the Lagom 1.4.0 milestone Nov 9, 2017

@ignasi35

This comment has been minimized.

Copy link
Member

commented Nov 9, 2017

Actually we could backport this to 1.3.x. @renatocaval @TimMoore WDYT?

@ignasi35 ignasi35 removed this from the Lagom 1.4.0 milestone Nov 9, 2017

Changed the wrapping exception type to IllegalArgumentException from …
…RuntimeException and accordingly updated test case
"com.lightbend.lagom.api.mock.InvalidPathParameterService service descriptor, or perhaps this parameter is meant to be the request message declared in the " +
"ServiceCall, and not extracted out of the path?")
}

This comment has been minimized.

Copy link
@renatocaval

renatocaval Nov 9, 2017

Member

@silver-soule sorry, but I haven't seen that before. My bad.

I find it fragile to test matching the text. We need to copy and paste and hack it to replace method.getDeclaringClass + "." + method.getName.

Maybe a better approach would be to create a new exception type that extends IllegalArgumentException. Later, when testing, we can simply check if we are getting the exception of the right type and we don't need to test the messaging.

This comment has been minimized.

Copy link
@silver-soule

silver-soule Nov 9, 2017

Author Contributor

@renatocaval
Yeah, that would be a better approach indeed , but I am not sure about two things -

  1. Where should I create this new exception class? In https://github.com/lagom/lagom/tree/master/service/javadsl/api/src/main/scala/com/lightbend/lagom/internal/javadsl/api does not seem ideal to me.
  2. Will calling this new subclass of IllegalArgumentException something like IllegalArgumentWrapperException be okay?

This comment has been minimized.

Copy link
@ignasi35

ignasi35 Nov 10, 2017

Member

Will calling this new subclass of IllegalArgumentException something like IllegalArgumentWrapperException be okay?

I think that name is not adding any information. IllegalArgumentWrapperException is a name that every possible java project could have. I think something like MissingPathParamSerializerException fits better the needs. WDYT?

This comment has been minimized.

Copy link
@renatocaval

renatocaval Nov 10, 2017

Member

We can call it IllegalPathParameterException.

We can add it under:
https://github.com/lagom/lagom/tree/master/service/javadsl/api/src/main/scala/com/lightbend/lagom/javadsl/api

Thus, not the internal package

This comment has been minimized.

Copy link
@silver-soule

silver-soule Nov 11, 2017

Author Contributor

@ignasi35 @renatocaval Thanks for your advice. I am going with IllegalPathParameterException, because it seems to align slightly better with the error message than MissingPathParamSerializerException.

@renatocaval

This comment has been minimized.

Copy link
Member

commented Nov 9, 2017

@TimMoore / @ignasi35, shall we backport this one once merged?

@renatocaval

This comment has been minimized.

Copy link
Member

commented Nov 9, 2017

@ignasi35 sorry, just now I saw that you have asked if we should backport it. Yes, ok for me. Will add the label

@renatocaval

This comment has been minimized.

Copy link
Member

commented Nov 11, 2017

Thanks for this @silver-soule!

@renatocaval renatocaval merged commit 907ac2d into lagom:master Nov 11, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

mdonkers added a commit to mdonkers/lagom that referenced this pull request Nov 11, 2017

Merge branch 'lagom-master'
* lagom-master: (194 commits)
  Improves error messages in path param serializer resolution (lagom#1093)
  Split test execution for Scala 2.11 and 2.12 (lagom#1095)
  Removed Scala style errors (lagom#1031)
  Setup MiMa for scaladsl projects (lagom#1092)
  Update to Play 2.6.7 (lagom#1088)
  Update Scala 2.12 version to 2.12.4 (lagom#1086)
  Akka Persistence JDBC update (lagom#1077)
  Fix unhandled event issue message in PersistentEntityTestDriver (lagom#1079)
  Delete PersistentEntityTestDriver.runOne (lagom#1081)
  Feature/update docs (lagom#1076)
  Hotfix/jsvalue serialization (lagom#1071)
  PathParamSerializer derived instances for traversables (lagom#1021)
  Ensures .gitignore file is copied to the Maven archetype (lagom#1070)
  Allow subscribers to access message metadata (lagom#1047)
  OffSet storage need the eventProcessorId (lagom#1056)
  Update and synchronize dependency versions (lagom#1061)
  Upgraded akka persistence cassandra version to 0.57 (lagom#1037)
  Added to the persistent read-sides documentation for CassandraReadSide, JdbcReadSide, etc the missing section about registering read-side processors (lagom#1052)
  fixes lagom#1018 - FF issue with websocket binary frames (lagom#1039)
  Service Router actions should use filtered request. (lagom#1030)
  ...
@ignasi35

This comment has been minimized.

Copy link
Member

commented Nov 16, 2017

Backported 402df4d 1.3.x

ignasi35 added a commit that referenced this pull request Nov 16, 2017

Improves error messages in path param serializer resolution (#1093)
* 1. Added a wrapping exception for IllegalArgumentException with a detailed error message
2. changed "ID" to "path parameter" in IllegalArgumentException error message

* updated error message

* Added test cases to check the error thrown if non-serializable path parameters are given in a lagom java ServiceCall

* Changed the wrapping exception type to IllegalArgumentException from RuntimeException and accordingly updated test case

* Added new exception type to be thrown if a serializer for path params is not found and updated test cases

@TimMoore TimMoore added this to the Lagom 1.3.11 milestone Nov 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.