Skip to content

feat: Add ability to echo request headers in trailing metadata#1501

Merged
parthea merged 12 commits intomainfrom
echo-request-headers
Apr 15, 2024
Merged

feat: Add ability to echo request headers in trailing metadata#1501
parthea merged 12 commits intomainfrom
echo-request-headers

Conversation

@parthea
Copy link
Copy Markdown
Contributor

@parthea parthea commented Apr 15, 2024 •
edited
Loading

This includes fixes from #1500. Please review #1500 first Done

Fixes #1493

Using this showcase server, I was able to see the http request headers in the trailing metadata

Response

responses.trailing_metadata()[0]
_Metadatum(key='x-goog-api-version', value='v1_20240408')

@parthea parthea marked this pull request as ready for review April 15, 2024 16:20
@parthea parthea requested review from a team April 15, 2024 16:20
noahdietz
noahdietz reviewed Apr 15, 2024
View reviewed changes
Comment thread server/services/echo_service.go
Comment thread schema/google/showcase/v1beta1/echo.proto Outdated
Comment thread schema/google/showcase/v1beta1/echo.proto Outdated
@parthea parthea changed the title feat: Add ability to echo request headers in response feat: Add ability to echo request headers in trailing metadata Apr 15, 2024
@parthea
Copy link
Copy Markdown
Contributor Author

parthea commented Apr 15, 2024

This works for gRPC. I haven't figured out how to get the information for REST

@parthea parthea requested a review from noahdietz April 15, 2024 18:35
@parthea
Copy link
Copy Markdown
Contributor Author

parthea commented Apr 15, 2024 •
edited
Loading

@noahdietz Please could you take another look? Should we also support this for REST? Can that be added in a separate PR?

@noahdietz
Copy link
Copy Markdown
Contributor

noahdietz commented Apr 15, 2024

Should we also support this for REST?

Yes, definitely.

Can that be added in a separate PR?

Yes, definitely.

We may want the generated REST handlers, like this:

gapic-showcase/server/genrest/echo.go

Lines 87 to 99 in a34fc37

response, err := backend.EchoServer.Echo(ctx, request)
if err != nil {
backend.ReportGRPCError(w, err)
return
}
json, err := marshaler.Marshal(response)
if err != nil {
backend.Error(w, http.StatusInternalServerError, "error json-encoding response: %s", err.Error())
return
}
w.Write(json)

...to pull the same metadata from the context (as the gRPC handlers do) and write them as HTTP headers. I'd probably add a helper here for copying context metadata into HTTP response headers, then invoke that in the spot above (generator code here).

@vchudnov-g may be interested in this.

noahdietz
noahdietz reviewed Apr 15, 2024
View reviewed changes
Comment thread server/genproto/compliance.pb.go
Copy link
Copy Markdown
Contributor

@noahdietz noahdietz Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert all of the changes in the server/genproto directory, the headers will just be replaced when things are regenerated by CI.

Copy link
Copy Markdown
Contributor Author

@parthea parthea Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@parthea parthea requested a review from noahdietz April 15, 2024 19:59
@parthea parthea enabled auto-merge (squash) April 15, 2024 20:37
@parthea parthea merged commit 48f9b74 into main Apr 15, 2024
@parthea parthea deleted the echo-request-headers branch April 15, 2024 20:40
@release-please release-please Bot mentioned this pull request Apr 15, 2024
Merged
@quartzmo quartzmo mentioned this pull request Apr 26, 2024
Merged
@aandreassa aandreassa mentioned this pull request May 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@noahdietz noahdietz noahdietz approved these changes

Assignees

@noahdietz noahdietz

@alicejli alicejli

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add support to echo back the x-goog-api-version header so that AIP-4236 can be tested

3 participants

@parthea @noahdietz @alicejli