V1: Implement rest of gRPC server endpoints #638
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…expected response (#629) This addresses the TODO about how comparing the actual response to expected needed to be more sophisticated. In particular: 1. It is acceptable for the actual headers/trailers to contain more than what's in the test expectation. Not only is it common for some HTTP client/server frameworks to add add'l headers (like "Date", "User-Agent", etc.), but we also don't want to hard-code all of the protocol-specific headers into the test case data (e.g. "Content-Type"). 2. It is acceptable for a client or server to merge headers and trailers for responses that have empty response streams and an error. These can be conveyed via "trailers-only responses" in the gRPC protocol, in which case it is not possible to separate headers and trailers. Furthermore, such responses are actually "headers-only" on the wire, so we allow the sets to be merged into either headers or trailers under these specific conditions. 3. It is acceptable for the server to omit the Connect GET query params in its response. 4. It is acceptable for the client to omit the raw Connect error details. 5. `Any` messages need smarter treatment than just `proto.Equal` since two semantically equivalent messages could be serialized differently, which means the `value` byte slices in two messages may differ and cause `proto.Equal` to consider them unequal.
This finally brings it all together. This also adds a single test case to the embedded test suites (which gets expanded to 20 test cases with default config, due to permutations with HTTP versions, protocols, codecs, etc).
jhump
approved these changes
Nov 7, 2023
internal/app/grpcserver/impl.go
Outdated
Comment on lines
104
to
109
| if err := grpcutil.AddHeaderMetadata(stream.Context(), responseDefinition.ResponseHeaders); err != nil { | ||
| return err | ||
| } | ||
| if err := grpcutil.AddTrailerMetadata(stream.Context(), responseDefinition.ResponseTrailers); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
It is only necessary to set headers and trailers through the context for unary RPCs. For all stream RPCs, it is more clear to instead use the SetHeader and SetTrailer methods on stream.
There was a problem hiding this comment.
Gotcha. Changed all the streaming endpoints do use the stream methods. Also, since unary was now the only one using AddHeader/TrailerMetadata, I removed it from the grpcutil package and just inlined right in the unary endpoint.
smaye81
changed the title
jhump
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements the rest of the endpoints for the gRPC server (client streaming, server streaming, and bidi)