fix triple pojo overload issue #1523
Conversation
WalkthroughA validation mechanism was added to prevent method overloads for streaming in the method call type cache. The change uses Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/utils/SofaProtoUtils.java(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: EvenLjj
Repo: sofastack/sofa-rpc PR: 1488
File: remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ServerReqHeaderInterceptor.java:105-120
Timestamp: 2025-05-08T06:59:23.963Z
Learning: In SOFA RPC, TripleTracerAdapter.serverReceived() sets the invocation type on the SofaRequest based on the gRPC method type (CLIENT_STREAMING, SERVER_STREAMING, BIDI_STREAMING), but the SofaRequest.isAsync() method does not directly recognize these streaming types as asynchronous. According to EvenLjj, some modification in TripleTracerAdapter.serverReceived makes sofaRequest.isAsync() return true for streaming requests.
📚 Learning: 2025-05-08T06:59:23.963Z
Learnt from: EvenLjj
Repo: sofastack/sofa-rpc PR: 1488
File: remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ServerReqHeaderInterceptor.java:105-120
Timestamp: 2025-05-08T06:59:23.963Z
Learning: In SOFA RPC, TripleTracerAdapter.serverReceived() sets the invocation type on the SofaRequest based on the gRPC method type (CLIENT_STREAMING, SERVER_STREAMING, BIDI_STREAMING), but the SofaRequest.isAsync() method does not directly recognize these streaming types as asynchronous. According to EvenLjj, some modification in TripleTracerAdapter.serverReceived makes sofaRequest.isAsync() return true for streaming requests.
Applied to files:
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/utils/SofaProtoUtils.java
📚 Learning: 2025-05-08T06:34:38.670Z
Learnt from: EvenLjj
Repo: sofastack/sofa-rpc PR: 1488
File: remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapter.java:290-303
Timestamp: 2025-05-08T06:34:38.670Z
Learning: In SOFA-RPC's Triple protocol adapter, the server detects invocation type (streaming/sync) from gRPC method descriptors but intentionally doesn't propagate this to request properties or headers. This is by design to maintain compatibility with clients of different versions, as the server should rely on client-provided values rather than overriding them.
Applied to files:
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/utils/SofaProtoUtils.java
🧬 Code graph analysis (1)
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/utils/SofaProtoUtils.java (1)
core/exception/src/main/java/com/alipay/sofa/rpc/core/exception/RpcErrorType.java (1)
RpcErrorType(24-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: sca
| if (methodCallType.putIfAbsent(method.getName(), streamType) != null) { | ||
| throw new SofaRpcException(RpcErrorType.UNKNOWN, "triple pojo stream do not support method overload!!!"); | ||
| } |
There was a problem hiding this comment.
Use consistent error type and improve error message quality.
The validation logic correctly prevents method overloads, but there are two issues:
-
Inconsistent error type: This validation uses
RpcErrorType.UNKNOWN, while similar call-type validations elsewhere in this file (lines 92, 126, 134) useRpcErrorType.CLIENT_CALL_TYPE. For consistency, this should also useCLIENT_CALL_TYPE. -
Error message quality: The message has grammatical issues ("stream do not" → "streaming does not") and excessive exclamation marks. Consider a more professional message like:
"Triple POJO streaming does not support method overloads"
Apply this diff to address both issues:
- if (methodCallType.putIfAbsent(method.getName(), streamType) != null) {
- throw new SofaRpcException(RpcErrorType.UNKNOWN, "triple pojo stream do not support method overload!!!");
- }
+ if (methodCallType.putIfAbsent(method.getName(), streamType) != null) {
+ throw new SofaRpcException(RpcErrorType.CLIENT_CALL_TYPE,
+ "Triple POJO streaming does not support method overloads");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (methodCallType.putIfAbsent(method.getName(), streamType) != null) { | |
| throw new SofaRpcException(RpcErrorType.UNKNOWN, "triple pojo stream do not support method overload!!!"); | |
| } | |
| if (methodCallType.putIfAbsent(method.getName(), streamType) != null) { | |
| throw new SofaRpcException(RpcErrorType.CLIENT_CALL_TYPE, | |
| "Triple POJO streaming does not support method overloads"); | |
| } |
🤖 Prompt for AI Agents
In
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/utils/SofaProtoUtils.java
around lines 103 to 105, the exception uses RpcErrorType.UNKNOWN and an
unpolished message; change the error type to RpcErrorType.CLIENT_CALL_TYPE to
match other call-type validations and replace the message with a professional
sentence such as "Triple POJO streaming does not support method overloads"
(remove extra punctuation and fix grammar).
fix triple pojo overload issue
Summary by CodeRabbit