Propagate response headers with grpc error metadata #32
Conversation
WalkthroughThe pull request introduces two new classes, Changes
Poem
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: 2
Outside diff range, codebase verification and nitpick comments (2)
src/Internal/CallContext.php (1)
30-38: Add error handling inJson::decodemethod to manage JSON parsing errors.The
Json::decodemethod insrc/Internal/Json.phpusesjson_decodebut lacks error handling for potential parsing errors. Consider implementing checks usingjson_last_error()to ensure robust handling of malformed JSON inputs.
- File:
src/Internal/Json.php- Method:
Json::decodeAnalysis chain
Method
decodeis effectively implemented but ensure robust error handling.The method effectively decodes the JSON payload and initializes a
CallContextinstance. Ensure thatJson::decodeproperly handles JSON parsing errors. IfJson::decodedoes not handle exceptions, consider adding error handling in this method to manage malformed JSON inputs gracefully.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in Json::decode method. # Test: Search for the Json::decode implementation. Expect: Exception handling for JSON errors. ast-grep --lang php --pattern $'class Json { $$$ decode($_, $_) { $$$ } $$$ }'Length of output: 93
Script:
#!/bin/bash # Description: Search for the decode method in any Json class to verify its existence and error handling. # Test: Use ripgrep to find the decode method in any Json class. rg 'class Json' -A 10 | rg 'function decode'Length of output: 46
Script:
#!/bin/bash # Description: Search for any decode method in the codebase that might handle JSON parsing. # Test: Use ripgrep to find any decode method that might be responsible for JSON parsing. rg 'function decode' -A 5Length of output: 658
src/Server.php (1)
Line range hint
123-161: Enhancements and potential issues in error response handling.The
workerGrpcErrormethod has been significantly modified to include a nullableContextInterfaceand to utilize theResponseHeaderswithin the error handling process. This is a crucial part of the PR's objective to propagate response headers with gRPC error metadata.Key observations:
- Nullable Context: The method now accepts a nullable
ContextInterface, which allows for more flexible error handling. However, this also introduces the risk of null dereferencing when accessing methods oncontext. It's important to ensure that all calls tocontextmethods are safe and handle the possibility of a null context.- Response Header Propagation: The method attempts to merge response headers with error details, which aligns with the PR's goals. However, the implementation assumes that
responseHeadersis always initialized whencontextis not null. This might not always be the case, and could lead to issues ifresponseHeadersis null.Consider adding null checks and more defensive programming to ensure that the headers and error details are always correctly handled.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- composer.json (1 hunks)
- src/Internal/CallContext.php (1 hunks)
- src/ResponseHeaders.php (1 hunks)
- src/Server.php (6 hunks)
Additional comments not posted (2)
src/Internal/CallContext.php (1)
20-24: Constructor implementation is clean and modern.The use of PHP 8 promoted properties in the constructor enhances readability and maintainability. The type declarations for parameters ensure that the method can only be called with the correct types.
composer.json (1)
75-79:branch-aliasaddition is correctly implemented.The addition of the
branch-aliassection is correctly formatted and follows best practices for Composer configuration. This change will facilitate easier dependency management and versioning during development.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/Server.php (6 hunks)
Additional comments not posted (4)
src/Server.php (4)
136-139: Proper handling of response headers in gRPC responses.The modifications to the
workerSendmethod ensure that response headers are correctly packaged and sent with gRPC responses. This change is essential for maintaining the integrity of the communication protocol and ensuring that all necessary headers are included in responses.Verify that headers are correctly handled in all response scenarios to ensure that no headers are missed or incorrectly formatted.
Run the following script to verify the correct handling of headers in responses:
Verification successful
Proper handling of response headers in gRPC responses confirmed.
The
workerSendmethod is used appropriately to ensure that headers are included in both successful and error responses. The headers are encoded as JSON, indicating they are structured correctly. No issues were found with the handling of headers.
- Usage of
workerSendinsrc/Server.phpconfirms proper handling of headers.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct handling of headers in gRPC responses. # Test: Search for the usage of `workerSend` method. Expect: Proper handling and inclusion of headers in responses. rg --type php -A 5 $'workerSend'Length of output: 1051
70-101: Streamlined gRPC request and response handling.The modifications to the
servemethod enhance clarity and efficiency by usingCallContextfor managing request headers and context. The direct invocation of service methods and the inclusion of response headers in error responses align well with the PR objectives.Ensure that the integration of
CallContextwith other components is thoroughly tested, especially in error scenarios, to confirm that all necessary headers are correctly propagated.Run the following script to verify the integration of
CallContext:
128-131: Enhanced flexibility in error handling.The addition of a nullable
ContextInterfaceparameter to theworkerErrormethod allows for more nuanced error handling, supporting the propagation of error context. This change is a positive step towards improving error diagnostics and response management.Verify that the new parameter is used effectively across all error handling scenarios to ensure that error contexts are properly utilized.
Run the following script to verify the usage of the new
ContextInterfaceparameter:
Line range hint
141-157: Correct encoding of gRPC error statuses.The modifications to the
createGrpcErrormethod ensure that gRPC error statuses are correctly formatted and encoded. This change is essential for maintaining the integrity of error communication and ensuring that all necessary details are included in error responses.Verify that error statuses are correctly encoded in all error scenarios to ensure that no details are missed or incorrectly formatted.
Run the following script to verify the correct encoding of gRPC error statuses:
rauanmayemir
force-pushed
the
feature/resp_headers
branch
2 times, most recently
from
25b43a5 to
af71e16
Compare
|
@rauanmayemir Feature was released from the RR side. |
rauanmayemir
force-pushed
the
feature/resp_headers
branch
from
492e85f to
a7e813f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Server.php (2)
79-89: Ensure proper exception handling when decoding call contextIf the
CallContext::decodemethod throws aJsonException, it is caught by the general\Throwablecatch block. Consider adding a specific catch block for\JsonExceptionto handle JSON decoding errors separately, providing clearer error handling and response to the client.
91-100: Reduce code duplication in response handlingThe logic for building the
$headersarray and sending the response via$this->workerSendis duplicated in both the success and error handling paths. Refactor this code into a separate method to improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle.For example, you could introduce a private method to handle the response sending:
private function sendResponse( WorkerInterface $worker, string $body, ResponseHeaders $responseHeaders, ResponseTrailers $responseTrailers, ?string $error = null ): void { $headers = array_filter([ 'headers' => $responseHeaders->count() ? $responseHeaders->packHeaders() : null, 'trailers' => $responseTrailers->count() ? $responseTrailers->packTrailers() : null, 'error' => $error, ]); $this->workerSend( worker: $worker, body: $body, headers: !empty($headers) ? Json::encode($headers) : '{}', ); }Then, update your
servemethod to usesendResponse:// On success $this->sendResponse( worker: $worker, body: $response, responseHeaders: $responseHeaders, responseTrailers: $responseTrailers ); // On GRPCException $this->sendResponse( worker: $worker, body: '', responseHeaders: $responseHeaders, responseTrailers: $responseTrailers, error: $this->createGrpcError($e) );Also applies to: 103-112
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
composer.json(1 hunks)src/Internal/CallContext.php(1 hunks)src/ResponseTrailers.php(1 hunks)src/Server.php(5 hunks)
🔇 Additional comments (2)
src/ResponseTrailers.php (1)
14-73: Implementation ofResponseTrailersclass is well-structuredThe
ResponseTrailersclass effectively manages response trailers with clean and maintainable code. It correctly implements theIteratorAggregateandCountableinterfaces, and thepackTrailersmethod handles empty trailer serialization appropriately.composer.json (1)
80-84: LGTM! Branch alias follows semantic versioning.The branch alias mapping of "3.x" to "3.4.x-dev" is appropriate for adding new features without breaking changes. This aligns well with the introduction of gRPC error metadata propagation.
Let's verify the version compatibility with RoadRunner's gRPC error metadata feature:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Server.php (2)
112-114: Consider propagating headers in generic error case.The generic error handler doesn't propagate any headers that might have been set before the error occurred. Consider maintaining consistency with the gRPC error path by including any set headers/trailers.
} catch (\Throwable $e) { - $this->workerError($worker, $this->isDebugMode() ? (string) $e : $e->getMessage()); + $headers = []; + $responseHeaders->count() === 0 or $headers['headers'] = $responseHeaders->packHeaders(); + $responseTrailers->count() === 0 or $headers['trailers'] = $responseTrailers->packTrailers(); + $this->workerError($worker, $this->isDebugMode() ? (string) $e : $e->getMessage(), Json::encode($headers)); }
138-141: Consider enhancing workerError to support header propagation.To support the new header propagation feature consistently, consider updating workerError to accept and handle headers.
-private function workerError(WorkerInterface $worker, string $message): void +private function workerError(WorkerInterface $worker, string $message, string $headers = '{}'): void { - $worker->error($message); + $worker->respond(new Payload('', $headers)); + $worker->error($message); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Internal/CallContext.php(1 hunks)src/Server.php(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Internal/CallContext.php
🔇 Additional comments (4)
src/Server.php (4)
13-13: LGTM! Import changes align with new functionality.The addition of CallContext and Json imports supports the new header propagation feature.
75-87: LGTM! Clean context initialization with header support.The initialization of ResponseHeaders and ResponseTrailers, followed by their integration into the context, provides a clean foundation for header propagation throughout the request lifecycle.
91-99: LGTM! Consistent header packing in success path.The success path properly handles both headers and trailers, with clean conditional packing and JSON encoding.
Line range hint
151-167: LGTM! Robust gRPC error serialization.The createGrpcError method provides a clean implementation for serializing gRPC errors with proper status codes, messages, and details. The base64 encoding ensures safe transport of the binary protobuf data.
rauanmayemir
force-pushed
the
feature/resp_headers
branch
from
e16d532 to
4c1bcab
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/ContextTest.php (2)
80-89: Consider adding PHPDoc for test clarity.The test method is well-structured and covers both successful and failure cases. Consider adding a PHPDoc comment to document the test's purpose and expectations.
+ /** + * Tests retrieval of individual trailer values from the context. + * Verifies both existing and non-existing trailer scenarios. + */ public function testGetOutgoingTrailer(): void
91-98: Consider enhancing test coverage with multiple trailers.While the test is well-structured, consider adding a test case with multiple trailers to ensure proper handling of multiple key-value pairs.
public function testGetOutgoingTrailers(): void { $outgoingTrailers = new ResponseTrailers([ 'X-Some-Trailer' => 'foobar', + 'X-Correlation-Id' => '123456', + 'X-Custom-Trailer' => 'test-value' ]); $ctx = new Context([ResponseTrailers::class => $outgoingTrailers]); $this->assertSame($outgoingTrailers, $ctx->getValue(ResponseTrailers::class)); }tests/ServerTest.php (1)
147-179: Consider enhancing test robustness.While the test effectively verifies the basic functionality, consider adding these improvements:
- Test edge cases with empty or null headers/trailers
- Verify the status code in the error response
- Add assertions for the main exception message
public function testInvokeGrpcException(): void { $worker = m::mock(WorkerInterface::class); $worker->shouldReceive('waitPayload') ->times(2) ->andReturn( new Payload( body: $this->packMessage('withDetailsAndHeaders'), header: '{"context": {}, "service": "service.Test", "method": "Throw"}', ), null, ); $worker->shouldReceive('respond')->once() ->withArgs(static function (Payload $payload) { $header = \json_decode($payload->header, true); $status = new Status(); $status->mergeFromString(\base64_decode($header['error'])); /** @var DetailsMessageForException $message */ $message = $status->getDetails()->offsetGet(0)->unpack(); $outgoingHeaders = \json_decode($header['headers'], true); $outgoingTrailers = \json_decode($header['trailers'], true); - return $message instanceof DetailsMessageForException + return $message instanceof DetailsMessageForException + && $status->getCode() === 3 + && $status->getMessage() === 'main exception message' && $message->getMessage() === 'details message' && $outgoingHeaders === ['foo' => 'bar'] && $outgoingTrailers === ['baz' => 'bar']; }); $this->server->serve($worker); }src/Server.php (3)
75-87: Consider adding null checks and validation.While the implementation is solid, consider adding these defensive programming measures:
- Validate the decoded call context
- Add null checks for the context array merge
$responseHeaders = new ResponseHeaders(); $responseTrailers = new ResponseTrailers(); try { $call = CallContext::decode($request->header); + if ($call === null) { + throw new \InvalidArgumentException('Invalid call context'); + } $context = new Context(\array_merge( - $call->context, + $call->context ?? [], [ ResponseHeaders::class => $responseHeaders, ResponseTrailers::class => $responseTrailers, ], ));
101-111: Consider enhancing error context.The error handling is solid, but consider adding more context to help with debugging:
$headers = [ 'error' => $this->createGrpcError($e), + 'error_type' => \get_class($e), + 'error_time' => (new \DateTimeImmutable())->format(\DateTime::RFC3339_EXTENDED), ]; $responseHeaders->count() === 0 or $headers['headers'] = $responseHeaders->packHeaders(); $responseTrailers->count() === 0 or $headers['trailers'] = $responseTrailers->packTrailers();
Line range hint
151-167: Consider adding serialization error handling.While the implementation is correct, consider handling potential serialization failures:
private function createGrpcError(GRPCExceptionInterface $e): string { + try { $status = new Status([ 'code' => $e->getCode(), 'message' => $e->getMessage(), 'details' => \array_map( static function ($detail) { + if (!method_exists($detail, 'serializeToString')) { + throw new \InvalidArgumentException('Invalid detail object'); + } $message = new Any(); $message->pack($detail); return $message; }, $e->getDetails(), ), ]); return \base64_encode($status->serializeToString()); + } catch (\Throwable $error) { + return \base64_encode((new Status([ + 'code' => StatusCode::INTERNAL, + 'message' => 'Failed to serialize error details', + ]))->serializeToString()); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
psalm-baseline.xml(1 hunks)src/Internal/CallContext.php(1 hunks)src/ResponseTrailers.php(1 hunks)src/Server.php(5 hunks)tests/ContextTest.php(2 hunks)tests/ServerTest.php(2 hunks)tests/Stub/TestService.php(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- psalm-baseline.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Internal/CallContext.php
- src/ResponseTrailers.php
🔇 Additional comments (4)
tests/ContextTest.php (1)
10-10: LGTM!The import statement for
ResponseTrailersis properly added and follows the existing pattern.tests/Stub/TestService.php (2)
38-47: LGTM! Good test coverage for header propagation.The new test case "withDetailsAndHeaders" effectively demonstrates the header propagation feature in error scenarios.
84-84: LGTM! Good coverage of trailer propagation.The addition of response trailers alongside headers provides comprehensive testing of metadata propagation in successful responses.
src/Server.php (1)
13-14: LGTM! Clean import organization.The new imports are properly organized and necessary for the enhanced functionality.
This change propagates response headers as part of grpc error response. This is helpful when your server needs to send certain headers (like
X-Correlation-Id, etc) even in case of Exception.Update: this change also introduces
ResponseTrailerswhich could be used alongsideResponseHeaders. It is expected to send data like this:And if it's a successful response, then binary payload headers part will also accordingly change from this:
to this:
ref: roadrunner-server/roadrunner#2006
@rustatian:
Note, this PR should be synced with RR 2024.3 release.
Summary by CodeRabbit
New Features
CallContextfor enhanced request header processing.ResponseTrailersclass for managing response headers and trailers.Improvements
Testing Enhancements
ResponseTrailersfunctionality.Dependency Updates
spiral/roadrunnerpackage to allow versions from2024.3and above.