fix the triple header context transfer issue #1467
Conversation
WalkthroughThis pull request refactors the handling of request headers and context management in the RPC server. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Interceptor as ServerReqHeaderInterceptor
participant Converter as HeaderConverter
participant Tracer as TripleTracerAdapter
Client->>Interceptor: Send RPC request with headers
Interceptor->>Converter: convertHeaderToContext(call, headers, request, serviceDef)
Converter->>Tracer: Invoke serverReceived() for tracing
Converter->>Tracer: Call getUserId(headers) to extract user ID
Tracer-->>Converter: Return user ID (or null)
Converter-->>Interceptor: Return constructed Context with header info
Interceptor-->>Client: Continue processing request with context
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ServerReqHeaderInterceptor.java (1)
165-178: Add method documentation.Consider adding Javadoc to document the purpose, parameters, and return value of this protected method.
+ /** + * Converts request headers into a Context with server span, request headers, and user ID. + * + * @param call the server call + * @param requestHeaders the request headers + * @param sofaRequest the SOFA request + * @param serverServiceDefinition the server service definition + * @return the Context with server span and tracing information + */ protected <ReqT, RespT> Context convertHeaderToContext(ServerCall<ReqT, RespT> call, Metadata requestHeaders, SofaRequest sofaRequest, ServerServiceDefinition serverServiceDefinition) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
all/pom.xml(1 hunks)bom/pom.xml(1 hunks)core/api/src/main/java/com/alipay/sofa/rpc/common/Version.java(1 hunks)pom.xml(1 hunks)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ServerReqHeaderInterceptor.java(2 hunks)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/TracingContextKey.java(2 hunks)remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapter.java(4 hunks)
✅ Files skipped from review due to trivial changes (2)
- all/pom.xml
- core/api/src/main/java/com/alipay/sofa/rpc/common/Version.java
🔇 Additional comments (5)
pom.xml (1)
80-80: Version Revision Updated Correctly.
The<revision>property has been updated to5.13.3-SNAPSHOT, which is consistent with the intended versioning strategy for a snapshot release. Ensure that any downstream modules or scripts that depend on this property are updated accordingly.bom/pom.xml (1)
13-13: Consistent Revision Property Change.
Updating<revision>to5.13.3-SNAPSHOThere aligns with the changes made in the parentpom.xml. This helps maintain uniform versioning across the project artifacts.remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/TracingContextKey.java (1)
34-35: LGTM! Well-structured user ID tracking implementation.The implementation follows the established pattern for context key management and maintains consistency with existing code.
Also applies to: 76-78
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/interceptor/ServerReqHeaderInterceptor.java (1)
66-66: LGTM! Clean refactoring of context management.The extraction of header-to-context conversion logic improves code organization and maintainability.
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapter.java (1)
56-56: LGTM! Good use of static import.The static import improves code readability.
| public static String getUserId(Metadata requestHeaders) { | ||
| String unitInfo = requestHeaders.get(HEAD_KEY_UNIT_INFO); | ||
| Map<String, String> unitInfoMap = JSON.parseObject(unitInfo, | ||
| new TypeReference<Map<String, String>>() { | ||
| }); | ||
| if (unitInfoMap != null) { | ||
| return unitInfoMap.get(USERID_KEY); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check and error handling.
The method should handle null input and JSON parsing errors gracefully.
public static String getUserId(Metadata requestHeaders) {
+ if (requestHeaders == null) {
+ return null;
+ }
String unitInfo = requestHeaders.get(HEAD_KEY_UNIT_INFO);
+ if (unitInfo == null) {
+ return null;
+ }
+ try {
Map<String, String> unitInfoMap = JSON.parseObject(unitInfo,
new TypeReference<Map<String, String>>() {
});
if (unitInfoMap != null) {
return unitInfoMap.get(USERID_KEY);
}
return null;
+ } catch (Exception e) {
+ LOGGER.warn("Failed to parse unit info: " + unitInfo, e);
+ return null;
+ }
}📝 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.
| public static String getUserId(Metadata requestHeaders) { | |
| String unitInfo = requestHeaders.get(HEAD_KEY_UNIT_INFO); | |
| Map<String, String> unitInfoMap = JSON.parseObject(unitInfo, | |
| new TypeReference<Map<String, String>>() { | |
| }); | |
| if (unitInfoMap != null) { | |
| return unitInfoMap.get(USERID_KEY); | |
| } | |
| return null; | |
| } | |
| public static String getUserId(Metadata requestHeaders) { | |
| if (requestHeaders == null) { | |
| return null; | |
| } | |
| String unitInfo = requestHeaders.get(HEAD_KEY_UNIT_INFO); | |
| if (unitInfo == null) { | |
| return null; | |
| } | |
| try { | |
| Map<String, String> unitInfoMap = JSON.parseObject(unitInfo, | |
| new TypeReference<Map<String, String>>() { | |
| }); | |
| if (unitInfoMap != null) { | |
| return unitInfoMap.get(USERID_KEY); | |
| } | |
| return null; | |
| } catch (Exception e) { | |
| LOGGER.warn("Failed to parse unit info: " + unitInfo, e); | |
| return null; | |
| } | |
| } |
…rry_pick # Conflicts: # all/pom.xml # bom/pom.xml # core/api/src/main/java/com/alipay/sofa/rpc/common/Version.java # pom.xml
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
remoting/remoting-triple/src/test/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapterTest.java (3)
56-64: Enhance test coverage with additional test cases.The test method covers basic scenarios but could be improved with additional test cases:
- Test with null metadata input
- Test with JSON containing no userid key
- Add descriptive test names using
@DisplayNameor comments@Test public void testGetUserId() { + // Test 1: Null metadata + Assert.assertNull(TripleTracerAdapter.getUserId(null)); + + // Test 2: Empty metadata Metadata metadata = new Metadata(); Assert.assertNull(TripleTracerAdapter.getUserId(metadata)); + // Test 3: Invalid JSON string metadata.put(TripleHeadKeys.HEAD_KEY_UNIT_INFO, "test"); Assert.assertNull(TripleTracerAdapter.getUserId(metadata)); + // Test 4: JSON without userid key + metadata.put(TripleHeadKeys.HEAD_KEY_UNIT_INFO, "{\"other\":\"value\"}"); + Assert.assertNull(TripleTracerAdapter.getUserId(metadata)); + // Test 5: Valid JSON with userid metadata.put(TripleHeadKeys.HEAD_KEY_UNIT_INFO, "{\"userid\":\"99\"}"); Assert.assertEquals("99", TripleTracerAdapter.getUserId(metadata)); }
56-64: Enhance test coverage with additional scenarios.The test method covers basic scenarios well, but could be more comprehensive.
Consider adding these test cases:
@Test public void testGetUserId() { + // Test null metadata + Assert.assertNull("Should handle null metadata", TripleTracerAdapter.getUserId(null)); + Metadata metadata = new Metadata(); - Assert.assertNull(TripleTracerAdapter.getUserId(metadata)); + Assert.assertNull("Should handle missing header", TripleTracerAdapter.getUserId(metadata)); metadata.put(TripleHeadKeys.HEAD_KEY_UNIT_INFO, "test"); - Assert.assertNull(TripleTracerAdapter.getUserId(metadata)); + Assert.assertNull("Should handle invalid JSON", TripleTracerAdapter.getUserId(metadata)); + + // Test empty JSON object + metadata.put(TripleHeadKeys.HEAD_KEY_UNIT_INFO, "{}"); + Assert.assertNull("Should handle empty JSON object", TripleTracerAdapter.getUserId(metadata)); metadata.put(TripleHeadKeys.HEAD_KEY_UNIT_INFO, "{\"userid\":\"99\"}"); - Assert.assertEquals("99", TripleTracerAdapter.getUserId(metadata)); + Assert.assertEquals("Should extract userid from valid JSON", "99", TripleTracerAdapter.getUserId(metadata)); }
56-64: Enhance test coverage with additional test cases.The test method covers basic scenarios but could be strengthened with additional test cases:
@Test public void testGetUserId() { Metadata metadata = new Metadata(); Assert.assertNull(TripleTracerAdapter.getUserId(metadata)); + Assert.assertNull(TripleTracerAdapter.getUserId(null)); metadata.put(TripleHeadKeys.HEAD_KEY_UNIT_INFO, "test"); Assert.assertNull(TripleTracerAdapter.getUserId(metadata)); + metadata.put(TripleHeadKeys.HEAD_KEY_UNIT_INFO, ""); + Assert.assertNull(TripleTracerAdapter.getUserId(metadata)); + metadata.put(TripleHeadKeys.HEAD_KEY_UNIT_INFO, "{}"); + Assert.assertNull(TripleTracerAdapter.getUserId(metadata)); metadata.put(TripleHeadKeys.HEAD_KEY_UNIT_INFO, "{\"userid\":\"99\"}"); Assert.assertEquals("99", TripleTracerAdapter.getUserId(metadata)); }remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapter.java (2)
120-120: Consider using the new getUserId method for consistency.The JSON serialization in
beforeSendcould be refactored to use the same JSON utility asgetUserId.- header.put(HEAD_KEY_UNIT_INFO.name(), JSONUtils.toJSONString(map)); + header.put(HEAD_KEY_UNIT_INFO.name(), JSON.toJSONString(map));
120-120: Consider using the same JSON library consistently.The code uses
JSONUtils.toJSONString()for serialization butJSON.parseObject()for deserialization. Consider using the same JSON library consistently to avoid potential compatibility issues.- header.put(HEAD_KEY_UNIT_INFO.name(), JSONUtils.toJSONString(map)); + header.put(HEAD_KEY_UNIT_INFO.name(), JSON.toJSONString(map));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapter.java(4 hunks)remoting/remoting-triple/src/test/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapterTest.java(1 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- remoting/remoting-triple/src/test/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapterTest.java
- remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapter.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: sca
🔇 Additional comments (5)
remoting/remoting-triple/src/main/java/com/alipay/sofa/rpc/tracer/sofatracer/TripleTracerAdapter.java (5)
120-120: LGTM! Consistent JSON serialization.The code uses
JSONUtils.toJSONStringfor serialization, maintaining consistency with the rest of the codebase.
307-323: LGTM! Robust implementation with proper error handling.The implementation:
- Handles null input gracefully
- Uses try-catch for JSON parsing errors
- Follows logging best practices
- Aligns with the past review suggestions
19-20: LGTM! Good choice of JSON parser.Using FastJSON is appropriate for this use case as it provides good performance and type-safe parsing.
307-323: Implementation looks good and follows previous review feedback.The method properly handles:
- Null checks for input
- JSON parsing errors with appropriate logging
- Type-safe parsing using TypeReference
307-323: Previous null check suggestion has been addressed.The implementation looks good with proper error handling and logging. The method correctly handles:
- Null or missing unit info
- JSON parsing errors
- Missing user ID in the parsed map
fix the triple header context transfer issue
Summary by CodeRabbit