Fix ClassCastException when applying HandlerAspect to routes with path parameters#3920
Fix ClassCastException when applying HandlerAspect to routes with path parameters#3920Godzilla675 wants to merge 19 commits intozio:mainfrom
Conversation
…h parameters (zio#3141) Added @@ operators to Route trait that apply aspects after path parameter decoding, avoiding the type mismatch that caused the ClassCastException.
Godzilla675
requested review from
987Nabil,
jdegoes,
kyri-petrou and
vigoo
as code owners
✅ Deploy Preview for zio-http ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…4040294268 Fix Scalafmt formatting errors
There was a problem hiding this comment.
Pull request overview
This PR fixes a ClassCastException (issue #3141) that occurs when combining a HandlerAspect middleware with a route that has path parameters. The root cause was that Handler.@@ assumes the handler's input is Request, but when applied directly to a handler used in a parameterized route, the input is actually a tuple of (params, Request). The fix adds @@ operators directly on the Route trait, which applies the aspect via Route.transform — operating on the handler after path parameters have been decoded, when the handler's input type is guaranteed to be Request.
Changes:
- Added three
@@operator overloads to theRoutetrait forMiddleware,HandlerAspect[_, Unit], andHandlerAspect[_, Ctx], mirroring the existing pattern onRoutes - Added a regression test in
RouteSpecverifying that aHandlerAspectcan be applied to a route with path parameters without throwing aClassCastException
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
zio-http/shared/src/main/scala/zio/http/Route.scala |
Adds three @@ operator overloads to the Route sealed trait for applying Middleware and HandlerAspect at the route level |
zio-http/jvm/src/test/scala/zio/http/RouteSpec.scala |
Adds a regression test for issue #3141, verifying route + path params + HandlerAspect works without ClassCastException |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| suite("HandlerAspect with path parameters")( | ||
| test("HandlerAspect should work with routes containing path parameters (#3141)") { | ||
| val authAspect: HandlerAspect[Any, Int] = | ||
| HandlerAspect.interceptIncomingHandler(Handler.fromFunction[Request] { request => | ||
| (request, 42) | ||
| }) | ||
|
|
||
| // Fixed: Apply aspect to the Route (not the Handler) using the new Route.@@ operator | ||
| // This correctly applies the aspect after path parameters are decoded | ||
| val route = (Method.GET / "base" / string("id") -> | ||
| handler((id: String, req: Request) => { | ||
| ZIO.succeed(Response.text(s"id=$id")) | ||
| })) @@ authAspect | ||
|
|
||
| val routes = Routes(route) | ||
|
|
||
| for { | ||
| response <- routes.runZIO(Request.get(url"/base/test123")) | ||
| body <- response.body.asString | ||
| } yield assertTrue( | ||
| response.status == Status.Ok, | ||
| body == "id=test123", | ||
| ) | ||
| }, |
There was a problem hiding this comment.
This test verifies the ClassCastException from #3141 no longer occurs, but it doesn't verify that the context provided by the aspect (value 42) is actually accessible within the handler. Consider adding an assertion that the context is propagated correctly, e.g., by using withContext in the handler to access the Int context and including its value in the response body. This would more closely match the original issue's reproducer, which uses withContext.
| /** | ||
| * Applies a middleware aspect to this route. | ||
| */ | ||
| final def @@[Env1 <: Env](aspect: Middleware[Env1]): Route[Env1, Err] = | ||
| aspect(self.toRoutes).routes.head.asInstanceOf[Route[Env1, Err]] | ||
|
|
||
| /** | ||
| * Applies a handler aspect that does not provide context to this route. | ||
| */ | ||
| final def @@[Env0](aspect: HandlerAspect[Env0, Unit]): Route[Env with Env0, Err] = | ||
| self.transform[Env with Env0](handler => handler @@ aspect) | ||
|
|
||
| /** | ||
| * Applies a handler aspect that provides context to this route. The aspect is | ||
| * applied after path parameters are decoded, so the handler receives a plain | ||
| * Request rather than a tuple of (params, request). | ||
| */ | ||
| final def @@[Env0, Ctx <: Env](aspect: HandlerAspect[Env0, Ctx])(implicit | ||
| tag: Tag[Ctx], | ||
| ): Route[Env0, Err] = | ||
| self.transform[Env0](handler => handler @@ aspect) |
There was a problem hiding this comment.
The Routes class provides a 4th @@ overload: def @@[Env0]: ApplyContextAspect[Env, Err, Env0] (see Routes.scala:55-56), which allows explicit specification of the remaining environment type after context elimination (used as routes.@@[Int & Long](authContext) in the existing test at line 355). This overload is missing from Route, which means users with intersection type environments who need partial context elimination would have to first wrap their route in Routes. Consider adding an equivalent ApplyContextAspect overload to Route for API consistency with Routes.
Fix Route missing @@ overload for ApplyContextAspect and update test
- Replace broken [[zio.http.Routes.ApplyContextAspect]] Scaladoc link with backtick-quoted text to fix scaladoc generation failure - Update tests to use withContext to access Int context value, matching the original issue zio#3141 reproducer pattern Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Fix Scaladoc link error and use withContext in tests
|
|
/claim #3141
Summary
This PR fixes the
ClassCastExceptionthat occurs when combining a route with path parameters and aHandlerAspectmiddleware.Changes
@@operators to theRoutetrait to apply aspects after path parameter decodingRouteSpec.scalawith a regression test (issue ClassCassException when using HandlerAspect and Path parameters. #3141)Root Cause
The
Handler.@@operator assumes input isRequest, but applied to a parameterized route, the input is a Tuple.The fix applies the aspect at the
Routelevel where we can handle this correctly.