feat: Update Gapic generator and Gax to emit api-versioning via header

[zhumin8]

This is part 2, following changes in #2630

Changes in this pr:

• ApiClientHeaderProvider: add key and token setter for api version.
• AbstractServiceStubSettingsClassComposer: add logic to set this header when api-version is present (not null or empty)
• Unit and golden tests to reflect above changes. (This includeds a few new golden files created for tests based on the dedicated proto created in feat: Parser to consume the api-versioning value from proto #2630)

This pr does not include gapic-showcased testing for now. Showcase test draft: #2737

---

manual tested with gapic-showcase v0.33.0

steps:

• Use gapic-showcase v0.33.0 (which includes update in 1484)
• use gapic-showcase run -v which prints the request header
• From showcase folder, run mvn verify -P enable-integration-tests -Dit.test=ITAutoPopulatedFields.java -pl=gapic-showcase

Printed header in manual testing
Grpc

2024/04/16 10:26:41 Received Unary Request for Method: /google.showcase.v1beta1.Echo/Echo
2024/04/16 10:26:41     Request headers:
2024/04/16 10:26:41       grpc-accept-encoding: gzip
2024/04/16 10:26:41       content-type: application/grpc
2024/04/16 10:26:41       x-goog-api-version: v1_20240408
2024/04/16 10:26:41       :authority: localhost:7469
2024/04/16 10:26:41       user-agent: grpc-java-netty/1.62.2
2024/04/16 10:26:41       x-goog-api-client: gl-java/17.0.7__Eclipse-Adoptium__Temurin-17.0.7-7 gapic/0.0.1-SNAPSHOT gax/2.46.2-SNAPSHOT grpc/1.62.2
2024/04/16 10:26:41     Request:  error:{code:2}  request_id:"1e26ee16-ed07-4eeb-be0e-49b769678779"  other_request_id:"40883e5d-3b20-4add-854c-9808bd009260"
2024/04/16 10:26:41     Returning Error: rpc error: code = Unknown desc = 

Httpjson

2024/04/16 10:26:41 Received POST request matching '/v1beta1/echo:echo': "/v1beta1/echo:echo"                                                               
2024/04/16 10:26:41   urlPathParams (expect 0, have 0): map[]                 
2024/04/16 10:26:41   urlRequestHeaders:                                                                                                                    
    User-Agent: "Google-HTTP-Java-Client/1.44.1 (gzip)"                                                                                                     
    Content-Type: "application/json; charset=utf-8"                           
    Connection: "keep-alive"                                                                                                                                
    Accept-Encoding: "gzip"                                                   
    X-Goog-Api-Client: "gl-java/17.0.7__Eclipse-Adoptium__Temurin-17.0.7-7 gapic/0.0.1-SNAPSHOT gax/2.46.2-SNAPSHOT rest/"                                  
    X-Goog-Api-Version: "v1_20240408"                                                                                                                       
    Accept: "text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2"            
    Content-Length: "129"                                                     
2024/04/16 10:26:41   request: {                                                                                                                            
  "error":  {                                                                                                                                               
    "code":  500,                                                                                                                                           
    "message":  "",                                                                                                                                         
    "details":  []                                                            
  },                                                                                                                                                        
  "severity":  "UNNECESSARY",                                                                                                                               
  "header":  "",                                                                                                                                            
  "otherHeader":  "",                                                         
  "requestId":  "e690a84b-176d-421e-9e5d-ba752f68fec8",                       
  "otherRequestId":  "47e0fbde-056a-43ae-bba6-8b6770d861f7"                   
}    

[zhumin8]

@lqiu96 As we talked off-line, I am still working on some details, plus integration testing. But feel free to take a first look.
(Build and tests are expected to fail for now, because changes in #2630 which this pr depends on is not included. I will rebase and fix when #2630 goes in.)

[lqiu96]

I think it should be Token != null && token != "".

The constructor sets it to null, but there is a public setter so it can be set to "". IIRC, we should treat both as null.

[zhumin8]

I am not exactly sure I am following your thoughts here.
If you are talking about the public setter in the Builder, then I think we are safe here. This constructor is protected and is meant to enforce construction through the builder.

or is this what you have in mind?

ApiClientHeaderProvider emptyProvider = ApiClientHeaderProvider.newBuilder().setApiVersionToken("")
        .build();

I don't think it's this HeaderProvider's responsibility to make any judgement on the header values. This logic belongs to AbstractServiceStubSettingsClassComposer.java via service.hasApiVersion(). I'll add in relevant tests to demonstrate.

[lqiu96]

I don't think it's this HeaderProvider's responsibility to make any judgement on the header values. This logic belongs to AbstractServiceStubSettingsClassComposer.java via service.hasApiVersion(). I'll add in relevant tests to demonstrate.

The composer only checks the api versionvalue from the proto file. This case is for any runtime configurations that the user adds to the client library. We currently treat any service configuration where api_version of "" as null and I am currently thinking that it would make sense for the same behavior here.

IIUC, ClientContext does make judgement on the header values and does some validation:

sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java

Lines 297 to 308 in fe0481e

	Set<String> conflicts = Sets.intersection(userHeaders.keySet(), internalHeaders.keySet());
	for (String key : conflicts) {
	if ("user-agent".equals(key)) {
	conflictResolution.put(key, userHeaders.get(key) + " " + internalHeaders.get(key));
	continue;
	}
	// Backwards compat: quota project id can conflict if its overriden in settings
	if (QUOTA_PROJECT_ID_HEADER_KEY.equals(key) && settings.getQuotaProjectId() != null) {
	continue;
	}
	throw new IllegalArgumentException("Header provider can't override the header: " + key);
	}

. I was thinking that it should be enforced, but that isn't required from this PR.

[zhumin8]

Working through testing changes after separated out api_version_testing.proto. Will update once ready.

[zhumin8]

Force pushed after rebase with main + mainly test updates.

[lqiu96]

Now that this is included as a parameterized test. I don't think we need this method anymore.

[zhumin8]

Ah. good catch. Done aea6e77.

[lqiu96]

My personal opinion is that I think this case should result in the ApiVersion header not being passed (similar to how it would behave if the service proto doesn't have this option). I know the client library can't reasonably do validation all values passed in to the ApiClientHeader, but empty string and null are cases which we know aren't valid ApiVersions.

The reason I have this case in mind is because it is something we check for inside the EndpointContext:

sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/rpc/EndpointContext.java

Lines 220 to 222 in 75352d1

	if (universeDomain != null && universeDomain.isEmpty()) {
	throw new IllegalArgumentException("The universe domain value cannot be empty.");
	}

[zhumin8]

Thanks for the context.

The way I see it currently is that, this logic of validating api version should not be repeated if possible. One benefit is that in case of future changes to it, say additional logic to treat a group of versions in a way (totally hypothetical here), change can be centralized.
Currently, this apiVersion information flows from:
proto --> via Parser --> model (Service) --> via composer --> output code (StubSettings) --> creates a ApiClientHeaderProvider instance.
ApiClientHeaderProvider.java does not need to know the details about how to validate api versions and it is not its responsibility to validate.

WDYT?

[lqiu96]

I think we might be on two different pages here. I don't intend for ApiClientHeaderProvider to try and validate the value that is set by the service team inside the proto file. If that value is not set by the team or is set as empty, then I trust that the generator has the correct logic to determine when the setApiVersionToken() code should or should not be generated.

My concern is that both ApiClientHeaderProvider and the ApiVersionToken setter is public. Users can create an ApiClientHeaderProvider separately and then pass it into the client via StubSettings (

sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/rpc/StubSettings.java

Lines 442 to 449 in 849bb50

	protected B setInternalHeaderProvider(HeaderProvider internalHeaderProvider) {
	this.internalHeaderProvider = internalHeaderProvider;
	if (this.quotaProjectId == null
	&& internalHeaderProvider.getHeaders().containsKey(QUOTA_PROJECT_ID_HEADER_KEY)) {
	this.quotaProjectId = internalHeaderProvider.getHeaders().get(QUOTA_PROJECT_ID_HEADER_KEY);
	}
	return self();
	}

). I believe it's possible given that we previously have suggested to users to set their own header provider values for certain workaround use cases.

I agree that's it's a extremely rare possibility that users are both creating a custom HeaderProvider and intentionally setting the ApiVersionToken to "", but because it's possible, I don't want to rule that possibility out. IMO, we should keep consistent with the default behavior done automatically for GAPICs (i.e. if it is set to be null or empty string, then it's not set in the header).

Granted this is a small nit pick that I think is a better UX. If you feel strongly otherwise, I don't want to continue to block on this small part. I think the other bits LGTM and I will do another pass before approving. Thanks for all the changes :)

[zhumin8]

Followup discussions directed off-line.
Merging this pr for now. May raise followup based on discussion result.

[sonarcloud]

Quality Gate failed for 'gapic-generator-java-root'

Failed conditions
68.4% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

[sonarcloud]

Quality Gate passed for 'java_showcase_integration_tests'

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[snippet-bot]

Here is the summary of possible violations 😱

There is a possible violation for not having product prefix.

• gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/samples/servicesettings/stub/SyncEchoWithVersionMethod.golden:19, tag goldensample_generated_EchoWithVersionStubSettings_EchoWithVersionMethod_sync

The end of the violation section. All the stuff below is FYI purposes only.

---

Here is the summary of changes.

You are about to add 1 region tag.

• gapic-generator-java/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/samples/servicesettings/stub/SyncEchoWithVersionMethod.golden:19, tag goldensample_generated_EchoWithVersionStubSettings_EchoWithVersionMethod_sync

---

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

• Refresh this comment