breaking: Drop @RPC annotation, use RxRouter.of[T].withPathPrefix(...)#506
Conversation
#501 introduced an @rpc annotation for parity with airframe's wvlet.airframe.http.RPC. On reflection that conflicts with uni's existing pattern — the rest of wvlet.uni.http.rpc.* (RPCRouter, RPCRoute, RPCClient) is already annotation-free, with RPC-ness determined by which router class you instantiate. Dropping @rpc brings RxRouter in line with that pattern and lets the macro shed a non-trivial annotation-detection branch. Behavioral changes: - wvlet.uni.http.rpc.RPC removed. - RxRouter.of[T] now treats every public method on T as a POST endpoint, unconditionally. The previous fallback to @endpoint scanning is gone — use Router.of[T] when you want @Endpoint-driven routes. - New builder method: RxRouter#withPathPrefix(prefix). Replaces the @rpc(path = "/v1") configuration: Before: @rpc(path = "/v1") trait Foo After: RxRouter.of[Foo].withPathPrefix("/v1") - withPathPrefix on a StemNode propagates to all children. - @rpc(description = "...") is dropped; describe via ScalaDoc. Code generation pipeline (HttpCodeGenerator/ServiceScanner/ uniHttpClients) is unchanged — it never depended on the annotation. Renamed scripted test sbt-test/codegen/rpc-client-annotated to rpc-client-with-provider to reflect what it actually exercises (an RPC trait whose companion extends RxRouterProvider). Refs #498. Reverts the annotation portion of #501; keeps the rest of the runtime API surface intact.
There was a problem hiding this comment.
Code Review
This pull request simplifies the RPC routing system by removing the @rpc annotation requirement and introducing a convention-based approach where all public methods are exposed as POST endpoints. It also adds a withPathPrefix method for manual namespacing. Key feedback includes addressing a visibility issue for macro-expanded code, ensuring withPathPrefix supports additive nesting to prevent overwriting existing prefixes, and optimizing route generation performance using lazy val in both EndpointNode and StemNode.
| // already filters them by owner, but we also exclude by name as belt-and-suspenders against | ||
| // unusual user-supplied method surfaces and future Surface changes. | ||
| private val ObjectMethodNames: Set[String] = Set( | ||
| private[router] val ObjectMethodNames: Set[String] = Set( |
There was a problem hiding this comment.
The visibility of ObjectMethodNames is restricted to private[router]. However, this set is referenced inside a macro quote ('{ ... }) in buildRxRouterImpl. When the macro is expanded at a call site outside of the wvlet.uni.http.router package, the generated code will fail to compile because it cannot access this private member. Please make this member public or move it to a location accessible by the macro expansion.
val ObjectMethodNames: Set[String] = Set(| Route(HttpMethod.POST, rpcPath, pathComponents, controllerSurface, ms) | ||
| } | ||
|
|
||
| override def withPathPrefix(prefix: String): RxRouter = copy(pathPrefix = prefix) |
There was a problem hiding this comment.
The current implementation of withPathPrefix in EndpointNode overwrites the existing pathPrefix instead of prepending to it. This causes issues when withPathPrefix is called on a StemNode (which propagates the call to its children) if those children already have their own prefixes defined. The prefixes should be additive to support proper router nesting.
override def withPathPrefix(prefix: String): RxRouter =
val newPrefix = if pathPrefix.isEmpty then prefix else s"${prefix.stripSuffix("/")}/${pathPrefix.stripPrefix("/")}"
copy(pathPrefix = newPrefix)| override def isLeaf: Boolean = true | ||
| override def toRoutes: Seq[Route] = routes | ||
|
|
||
| override def toRoutes: Seq[Route] = |
There was a problem hiding this comment.
The toRoutes method performs duplicate detection and path generation every time it is called. Since RxRouter definitions are typically static and the underlying surfaces are immutable, it is more efficient to use a lazy val to cache the resulting sequence of routes.
override lazy val toRoutes: Seq[Route] =| override def toRoutes: Seq[Route] = children.flatMap(_.toRoutes) | ||
| override def name: String = f"stem-${this.hashCode()}%08x" | ||
| override def isLeaf: Boolean = false | ||
| override def toRoutes: Seq[Route] = children.flatMap(_.toRoutes) |
There was a problem hiding this comment.
- ObjectMethodNames: now public (was private[router]). The buildRxRouter macro emits `RouterMacros.ObjectMethodNames.contains(...)` at the call site, which lives outside this package, so private[router] would break user-side compilation. - withPathPrefix on EndpointNode: composes additively rather than overwriting. Previously a stem-level prefix would erase any prefix the children already carried; now the outer prefix is prepended, preserving inner prefixes for nested router composition. - toRoutes: now `lazy val` on both EndpointNode and StemNode. Routers are typically built once at startup, so caching the route list (and the duplicate-detection check) avoids redundant work and allocations on each call. Adds a regression test for the additive composition case Gemini called out (stem prefix /api applied over per-child prefixes /v1, /v2 should yield /api/v1/... and /api/v2/...).
Summary
Revisits the architectural decision from #501. The rest of
`wvlet.uni.http.rpc.*` (`RPCRouter`, `RPCRoute`, `RPCClient`) is
intentionally annotation-free — RPC-ness is determined by which
router class you instantiate. `@RPC` was added in #501 mostly for
parity with airframe; on reflection that conflicts with uni's own
pattern and required a non-trivial annotation-detection branch in the
`RxRouter` macro that the existing Surface metadata didn't need.
Drop the annotation and lean on the convention.
Behavioral changes
`wvlet.uni.http.rpc.RPC` removed.
`RxRouter.of[T]` now treats every public method on `T` as a POST
endpoint, unconditionally. The previous fallback to `@Endpoint`
scanning is gone — use `Router.of[T]` when you want `@Endpoint`
routes.
New builder: `RxRouter#withPathPrefix(prefix)` replaces `@RPC(path)`.
Before:
```scala
@rpc(path = "/v1")
trait FileApi:
def upload(name: String): String
object FileApi extends RxRouterProvider:
override def router = RxRouter.of[FileApi]
```
After:
```scala
trait FileApi:
def upload(name: String): String
object FileApi extends RxRouterProvider:
override def router = RxRouter.of[FileApi].withPathPrefix("/v1")
```
`withPathPrefix` on a `StemNode` propagates to all children.
`@RPC(description = "...")` is dropped; describe via ScalaDoc.
The codegen pipeline (`HttpCodeGenerator` / `ServiceScanner` /
`uniHttpClients`) is unchanged — it never depended on the annotation.
Renamed sbt scripted test `rpc-client-annotated` → `rpc-client-with-provider`
to reflect what it now exercises (an RPC trait whose companion extends
`RxRouterProvider`).
Test plan
pass (added 3 `withPathPrefix` tests, removed the @RPC-specific
ones)
pass on Node.js
tests pass
Refs #498. Reverts the annotation portion of #501; keeps the rest of
the runtime API surface intact.