Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(server): echo request headers in response over REST transport #1509

Merged
merged 1 commit into from Apr 29, 2024

Conversation

vchudnov-g
Copy link
Contributor

@vchudnov-g vchudnov-g commented Apr 26, 2024

Towards #1493, but note that this will occur for all Showcase services, not just Echo.

This can be verified manually on the shell via:

❯ curl -i -X POST -H "X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0" -H 'Content-Type: application/json' http://localhost:7469/v1beta1/echo:echo -d'{"content":"g
reetings"}'                                                                                                                                                 
HTTP/1.1 200 OK
X-Showcase-Request-Accept: */*
X-Showcase-Request-Content-Length: 23
X-Showcase-Request-Content-Type: application/json
X-Showcase-Request-User-Agent: curl/8.5.0
X-Showcase-Request-X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0
Date: Fri, 26 Apr 2024 21:33:24 GMT
Content-Length: 104
Content-Type: text/plain; charset=utf-8

{
  "content":  "greetings",
  "severity":  "UNNECESSARY",
  "requestId":  "",
  "otherRequestId":  ""
}

and works even for error responses:

❯ curl -i -X POST -H "X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0" -H 'Content-Type: application/json' http://localhost:7469/v1beta1/echo:echo -d'{"foo":"greet
ings"}'                                                                                                                                                     
HTTP/1.1 400 Bad Request
X-Showcase-Request-Accept: */*
X-Showcase-Request-Content-Length: 19
X-Showcase-Request-Content-Type: application/json
X-Showcase-Request-User-Agent: curl/8.5.0
X-Showcase-Request-X-Goog-Api-Client: rest/0.0.0 gapic/0.0.0
Date: Fri, 26 Apr 2024 21:47:05 GMT
Content-Length: 160
Content-Type: text/plain; charset=utf-8

{"error":{"code":400,"message":"error reading body params '*': proto: (line 1:2): unknown field \"foo\"","details":null,"Body":"","Header":null,"Errors":null}

@vchudnov-g vchudnov-g requested review from a team as code owners April 26, 2024 21:40
@vchudnov-g vchudnov-g changed the title feat: echo request headers in response over REST transport feat(server): echo request headers in response over REST transport Apr 26, 2024
Copy link
Contributor

@parthea parthea left a comment

Choose a reason for hiding this comment

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

LGTM. I tested this locally using the code in googleapis/gapic-generator-python#1999

@parthea parthea merged commit de93c4c into main Apr 29, 2024
11 checks passed
@parthea parthea deleted the echo-headers branch April 29, 2024 22:04
@noahdietz
Copy link
Collaborator

FWIW I don't think @vchudnov-g had landed on a header prefix yet, at least the suggestions in the document he wrote are different than what is implemented here...

@parthea
Copy link
Contributor

parthea commented Apr 29, 2024

At the time of merging this PR it seemed like we had settled on a prefix based on the comment thread in the document.

@noahdietz
Copy link
Collaborator

At the time of merging this PR it seemed like we had settled on a prefix based on the comment thread in the document.

You're right, @vchudnov-g had settled on that!

@vchudnov-g
Copy link
Contributor Author

One thought that occurs to me: maybe for the same reasons we have prefixes in the echoed http headers, we should also have them in the gRPC trailers that echo the gRPC request headers? Thoughts?

@noahdietz
Copy link
Collaborator

One thought that occurs to me: maybe for the same reasons we have prefixes in the echoed http headers, we should also have them in the gRPC trailers that echo the gRPC request headers? Thoughts?

Yeah it probably makes sense to do, but let's make sure we blast it out in release notes for those that already have trailer tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants