oxsean commented on PR #16325: URL: https://github.com/apache/dubbo/pull/16325#issuecomment-4808930829
## Review: Triple method resolution for overloaded wrapper methods The PR reworks `DescriptorUtils` so overloaded Triple wrapper methods are resolved by exact arg-type matching, only falling back to the generated unary/server-stream pair when that pair is verified (matching parameter *and* response types), and failing fast on genuinely ambiguous overloads. The core disambiguation logic is sound — the response-type check correctly tells a real generated unary twin apart from a user overload that merely shares a name. However, the rewrite silently dropped a pre-existing behavior. ### Findings (most severe first) **1. [correctness] Removed lowercase-first-char method fallback → capitalized method names no longer resolve** `DescriptorUtils.java:102-104` (`findReflectionMethodDescriptor`) and `:122-124` (`findTripleMethodDescriptor`) The old code retried `serviceDescriptor.getMethods(lowerMethod)` (lowercasing the first char) when the exact name missed. Both rewritten methods dropped it. `getMethods` is an exact `HashMap.get`. A client invoking RPC `"SayHello"` against a service whose Java method is `sayHello` previously resolved; now `findReflectionMethodDescriptor` returns `null` → NPE at `AbstractServerTransportListener#buildRpcInvocation` (`MethodMetadata.fromMethodDescriptor(null)` / `methodDescriptor.getMethodName()`) on the REST/HTTP-JSON path, and `findTripleMethodDescriptor` throws `UnimplementedException` on the gRPC wrapper path. Confirmed against `3.3` — the fallback existed and is now gone. Suggest adding a test for a capitalized method name to lock this in. **2. [correctness] Genuine 2-method overload (non-generated pair) now returns `null` → NPE on the reflection path** `DescriptorUtils.java:104` → `findGeneratedUnaryMethodDescriptor` (`:184-213`) For a non-stub service with two same-named methods — one unary `X foo(A)`, one server-stream `void foo(A, StreamObserver<Y>)` with `Y != X` — the old size==2 branch blindly returned the unary method. The new `findGeneratedUnaryMethodDescriptor` rejects the unverified pair and returns `null`. On the reflection path (no `rawMessage` to disambiguate) this null reaches `buildRpcInvocation` and NPEs. The "fail fast" intent is right, but it should throw `UnimplementedException` here, not return `null` that NPEs the caller. **3. [correctness, plausible] `parseRequestWrapper` swallows decode errors → corrupt wrapper misrouted instead of surfaced** `DescriptorUtils.java:162-171` The old code let a `TripleRequestWrapper.parseFrom` failure propagate. The new helper catches `IOException | RuntimeException` and returns `null`; resolution then falls through to `findGeneratedUnaryMethodDescriptor`. For a verified unary/server-stream pair, a truncated/corrupt first frame now silently resolves to the unary method and deserializes the corrupt body against it, converting a clear decode error into a misrouted call or a more obscure downstream failure. **4. [cleanup] Duplicated generic/echo dispatch across the two finder methods** `DescriptorUtils.java:92-101` and `:111-120` The identical `isGeneric → genericService()...` / `isEcho → echoService()...` prefix is copied into both `findReflectionMethodDescriptor` and `findTripleMethodDescriptor`; extract a shared `findWellKnownMethodDescriptor(methodName)` so the two stay in sync. (Minor: the stale class Javadoc "The MetaUtils provides..." at line 38 is pre-existing but worth fixing while here.) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
