feat: Add google.api.api_version annotation to echo.proto by parthea · Pull Request #1484 · googleapis/gapic-showcase

parthea · 2024-03-23T15:04:25Z

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.

parthea · 2024-03-23T15:04:41Z

schema/google/showcase/v1beta1/echo.proto

  // 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


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

… be opaque to clients as per AIP

parthea · 2024-03-23T15:29:08Z

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.

noahdietz · 2024-04-08T22:18:31Z

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 · 2024-04-08T22:20:36Z

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

gapic-showcase/server/services/echo_service.go

Lines 281 to 307 in 0622c2f

    
           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 · 2024-04-08T22:24:00Z

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

gapic-showcase/util/genrest/resttools/systemparam.go

Lines 24 to 47 in 0622c2f

    
           // 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 · 2024-04-08T22:25:57Z

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.

noahdietz

See comments.

parthea · 2024-04-08T22:40:48Z

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 · 2024-04-08T22:49:28Z

@noahdietz, Please could you take another look?

noahdietz · 2024-04-08T23:00:45Z

schema/google/showcase/v1beta1/echo.proto

  // 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";


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...

Fixed in a377321

noahdietz · 2024-04-08T23:01:05Z

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

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`

feat: Add google.api.api_version annotation to echo.proto

1378107

parthea commented Mar 23, 2024

View reviewed changes

update the api_version to a nonsense value to refelect that it should…

81c1a8c

… be opaque to clients as per AIP

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

Merge branch 'main' into add-api-version-to-echo-proto

28205f6

parthea marked this pull request as ready for review April 8, 2024 22:14

parthea requested review from a team April 8, 2024 22:14

Merge branch 'main' into add-api-version-to-echo-proto

606c9cf

noahdietz requested changes Apr 8, 2024

View reviewed changes

parthea mentioned this pull request Apr 8, 2024

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

Closed

parthea requested a review from noahdietz April 8, 2024 22:43

noahdietz reviewed Apr 8, 2024

View reviewed changes

noahdietz self-requested a review April 8, 2024 23:01

Address review feedback

a377321

noahdietz approved these changes Apr 9, 2024

View reviewed changes

parthea merged commit 9a2298b into main Apr 9, 2024

parthea deleted the add-api-version-to-echo-proto branch April 9, 2024 15:07

release-please bot mentioned this pull request Apr 9, 2024

chore(main): release 0.33.0 #1495

Merged

zhumin8 mentioned this pull request Apr 17, 2024

deps: update gapic-showcase to v0.33.0 googleapis/sdk-platform-java#2653

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add google.api.api_version annotation to echo.proto#1484

feat: Add google.api.api_version annotation to echo.proto#1484
parthea merged 5 commits intomainfrom
add-api-version-to-echo-proto

parthea commented Mar 23, 2024 •

edited

Loading

Uh oh!

parthea Mar 23, 2024

Uh oh!

parthea commented Mar 23, 2024

Uh oh!

noahdietz commented Apr 8, 2024

Uh oh!

noahdietz commented Apr 8, 2024

Uh oh!

noahdietz commented Apr 8, 2024

Uh oh!

noahdietz commented Apr 8, 2024

Uh oh!

noahdietz left a comment

Uh oh!

parthea commented Apr 8, 2024 •

edited

Loading

Uh oh!

parthea commented Apr 8, 2024

Uh oh!

noahdietz Apr 8, 2024

Uh oh!

parthea Apr 9, 2024

Uh oh!

noahdietz commented Apr 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

parthea commented Mar 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parthea Mar 23, 2024

Choose a reason for hiding this comment

Uh oh!

parthea commented Mar 23, 2024

Uh oh!

noahdietz commented Apr 8, 2024

Uh oh!

noahdietz commented Apr 8, 2024

Uh oh!

noahdietz commented Apr 8, 2024

Uh oh!

noahdietz commented Apr 8, 2024

Uh oh!

noahdietz left a comment

Choose a reason for hiding this comment

Uh oh!

parthea commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parthea commented Apr 8, 2024

Uh oh!

noahdietz Apr 8, 2024

Choose a reason for hiding this comment

Uh oh!

parthea Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

noahdietz commented Apr 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

parthea commented Mar 23, 2024 •

edited

Loading

parthea commented Apr 8, 2024 •

edited

Loading