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: Add google.api.api_version annotation to echo.proto #1484

Merged
merged 5 commits into from
Apr 9, 2024

Conversation

parthea
Copy link
Contributor

@parthea parthea commented Mar 23, 2024

This PR allows folks to run manual tests against AIP-4236 (See aip-dev/google.aip.dev#1331)

gapic-showcase run -v will log the request headers in verbose mode as per https://github.com/googleapis/gapic-showcase/blob/main/CHANGELOG.md#v090--2020-04-22

I created #1493 for echoing the x-goog-api-version header so that it can be used in automated integration tests.

@@ -43,6 +43,8 @@ service Echo {
// This service is meant to only run locally on the port 7469 (keypad digits
// for "show").
option (google.api.default_host) = "localhost:7469";
// See https://github.com/aip-dev/google.aip.dev/pull/1331
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Link to AIP once aip-dev/google.aip.dev#1331 is merged

@parthea parthea force-pushed the add-api-version-to-echo-proto branch from d6c5354 to 81c1a8c Compare March 23, 2024 15:18
@parthea
Copy link
Contributor Author

parthea commented Mar 23, 2024

Wait for #1485 which should resolve the error below which appears in https://github.com/googleapis/gapic-showcase/actions/runs/8402715704/job/23012403762?pr=1484

2024/03/23 15:19:14 google/showcase/v1beta1/echo.proto:47:10: Option "(google.api.api_version)" unknown. Ensure that your proto definition file imports the proto which defines the option.
Submodule path 'schema/googleapis': checked out '5c1f64e746af6e53713884df3eb7719f3956a66f'

PR #1485 updates the git submodule from SHA 5c1f64e746af6e53713884df3eb7719f3956a66f to SHA d81d0b9e6993d6ab425dff4d7c3d05fb2e59fa57. See the diff here which includes the google.api.api_version annotation.

@parthea parthea marked this pull request as ready for review April 8, 2024 22:14
@parthea parthea requested review from a team as code owners April 8, 2024 22:14
@noahdietz
Copy link
Collaborator

Adding the annotation is fine & necessary, but how do you expect generator integration tests to test that the generated code sends the proper api version?

There are two likely routes:

  • the showcase server "echos" back the metadata/headers it was sent as trailing metadata/response headers so that the generator integration tests can capture them and assert what it sent was expected
  • the server reports an error/fails the request if the API version isn't present in (one of) the expected places

I'd prefer the first one as it is not a breaking change.

@noahdietz
Copy link
Collaborator

Here is where it currently echos things back through the response:

func echoHeaders(ctx context.Context) {
md, ok := metadata.FromIncomingContext(ctx)
if !ok {
return
}
values := md.Get("x-goog-request-params")
for _, value := range values {
header := metadata.Pairs("x-goog-request-params", value)
grpc.SetHeader(ctx, header)
}
}
func echoStreamingHeaders(stream grpc.ServerStream) {
md, ok := metadata.FromIncomingContext(stream.Context())
if !ok {
return
}
values := md.Get("x-goog-request-params")
for _, value := range values {
header := metadata.Pairs("x-goog-request-params", value)
if stream.SetHeader(header) != nil {
return
}
}
}

@noahdietz
Copy link
Collaborator

I think this is the proper place to implement additional HTTP header/query string translation assuming API version will be a System Parameter:

// SystemParameters encapsulates the system parameters recognized by Showcase. These are a subset of
// Google's accepted system parameters described in
// https://cloud.google.com/apis/docs/system-parameters.
type SystemParameters struct {
EnumEncodingAsInt bool
}
// GetSystemParameters returns the SystemParameters encoded in request, and the query parameters in
// the request's query string that are not themselves system parameters.
func GetSystemParameters(request *http.Request) (systemParams *SystemParameters, queryParams map[string][]string, err error) {
return processQueryString(request.URL.RawQuery)
}
// processQueryString returns the SystemParameters encoded in queryString, and the query parameters in
// queryString that are not themselves system parameters.
//
// Since we want GAPICs to be strict in what they emit, and Showcase helps guarantee that, this
// function is strict in what it accepts:
// - no more than one instance of the $alt system parameter
// - $alt may be appear as "$alt" or "alt", always lower case
// - the only possible values for $alt are "json" or "json;enum-encoding=int"; again, lower case
// - the semicolon in "json;enum-encoding=int" must be URL escaped as "%3B" or "%3b"
// - the equal sign in "json;enum-encoding=int" may or may not be URL-escaped
func processQueryString(queryString string) (systemParams *SystemParameters, queryParams map[string][]string, err error) {

@noahdietz
Copy link
Collaborator

There might also be some code that needs updating in the genrest generator to ensure that system params end up being sent to the grpc handler implementation as grpc metadata.

Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

See comments.

@parthea
Copy link
Contributor Author

parthea commented Apr 8, 2024

Adding the annotation is fine & necessary, but how do you expect generator integration tests to test that the generated code sends the proper api version?

I only performed manual tests using this updated proto. gapic-showcase run -v will log the request headers in verbose mode as per https://github.com/googleapis/gapic-showcase/blob/main/CHANGELOG.md#v090--2020-04-22. I can see that the header exists in the logs. I created #1493 to follow up on adding support to echo back the x-goog-api-version header sent with the request to the showcase server.

2024/04/08 22:35:34 Received Unary Request for Method: /google.showcase.v1beta1.Echo/Echo
2024/04/08 22:35:34     Request headers:
2024/04/08 22:35:34       content-type: application/grpc
2024/04/08 22:35:34       user-agent: grpc-python/1.63.0.dev0 grpc-c/39.0.0 (linux; chttp2)
2024/04/08 22:35:34       x-goog-api-version: moose

@parthea parthea requested a review from noahdietz April 8, 2024 22:43
@parthea
Copy link
Contributor Author

parthea commented Apr 8, 2024

@noahdietz, Please could you take another look?

@@ -43,6 +43,8 @@ service Echo {
// This service is meant to only run locally on the port 7469 (keypad digits
// for "show").
option (google.api.default_host) = "localhost:7469";
// See https://github.com/aip-dev/google.aip.dev/pull/1331
option (google.api.api_version) = "moose";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok and just to make sure.....we don't want a "real" version here?

I understand from generator/client perspective this is an opaque string, but there is something to be said about having good test data...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in a377321

@noahdietz
Copy link
Collaborator

I created #1493 to follow up on adding support to echo back the x-goog-api-version header sent with the request to the showcase server.

Thanks SGTM

@noahdietz noahdietz self-requested a review April 8, 2024 23:01
@parthea parthea merged commit 9a2298b into main Apr 9, 2024
12 checks passed
@parthea parthea deleted the add-api-version-to-echo-proto branch April 9, 2024 15:07
zhumin8 added a commit to googleapis/sdk-platform-java that referenced this pull request Apr 22, 2024
update gapic-showcase so that we can pickup changes in
googleapis/gapic-showcase#1484

Steps: 
1) Updated the gapic-showcase version in `gapic-showcase/pom.xml` to
0.33.0
2) Ran `mvn compile -P update`
lqiu96 pushed a commit to googleapis/sdk-platform-java that referenced this pull request May 22, 2024
update gapic-showcase so that we can pickup changes in
googleapis/gapic-showcase#1484

Steps: 
1) Updated the gapic-showcase version in `gapic-showcase/pom.xml` to
0.33.0
2) Ran `mvn compile -P update`
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.

2 participants