feat: Dynamic Routing Headers for HttpJson #1667

lqiu96 · 2023-05-05T16:36:21Z

Changes in this PR:

Create two new Request Param Callables for HttpJson (Unary and Server side streaming)
Pass the routing header in the Call Context to be used by transport specific logic
Move Routing Header generation logic to the Abstract class (shared between the transports)
Use RequestParamsBuilder to percent-encode both the header key and value (as part of https://github.com/googleapis/googleapis/blob/b9792eec95cc5c75e5fb173bb068aecde45881b1/google/api/routing.proto#L62-L64)
Regenerate the golden unit test and integration test cases
Add showcase tests for gRPC and HttpJson for Dynamic Routing Headers

Showcase Request Headers

Current Behavior (No manual percent-encoding on client side)

Manually percent-encoding the header key and values

Showcase Testing

Intercept the header values before the get sent out via ClientInterceptors (grpc and httpjson).

Thank you for opening a Pull Request! For general contributing guidelines, please refer to contributing guide

Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #1666 ☕️

gax-java/gax/src/main/java/com/google/api/gax/rpc/RequestParamsBuilder.java

blakeli0 · 2023-05-31T20:05:42Z

...ax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonUnaryRequestParamCallable.java

+  public ApiFuture<ResponseT> futureCall(RequestT request, ApiCallContext context) {
+    ApiCallContext newCallContext = context;
+    String encodedHeader = paramsEncoder.encode(request);
+    if (encodedHeader != null && !encodedHeader.isEmpty()) {


It might be a miss in the past, looks like we were sending the header even it is empty, do we have requirement that we should not send this header if it's empty?

I was following these two guidances from the routing.proto:
https://github.com/googleapis/googleapis/blob/6782d08a0fa56c39070f9d1337d629eaecfe2fc1/google/api/routing.proto#L134-L150
and
https://github.com/googleapis/googleapis/blob/6782d08a0fa56c39070f9d1337d629eaecfe2fc1/google/api/routing.proto#L109-L110

My understanding is that we should not be sending the header if it's empty. Just noticed that Grpc's callables don't check and I can add a check for them.

Can you confirm with Slava? You are probably right and I agree that we should not send header if it's empty, but want to make sure it would not break existing behavior(in case some server are expecting empty header).

Yep will do.

Conversation with Slava:

the behaviour should be to NOT send the x-goog-request-params at all server-side will likely do the right thing even if you send an empty value

blakeli0 · 2023-05-31T20:26:19Z

gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java

+
+    Map<String, String> headerMap = callOptions.getRequestHeaderMap();
+    for (Map.Entry<String, List<String>> extraHeaderEntrySet :
+        httpJsonContext.getExtraHeaders().entrySet()) {


OK, I though we already have logics to handle extra headers, but looks like httpJsonContext.getExtraHeaders() was never used, I think it still makes sense to use it, but the benefit is much smaller than I expected.
That being said, I'm not sure setting it to HttpJsonCallOptions is the best way. Based on these comment, HttpJsonCallOptions is for API specific data not request specific data, and the dynamic routing header is request specific, which is inline with what I'm thinking as well. We can change this behavior if we want but I would see if it there is a way to set the header to HttpJsonMetadata directly.
There is also a TODO below // TODO: add headers interceptor logic which could be related as well.

We can change this behavior if we want but I would see if it there is a way to set the header to HttpJsonMetadata directly.

I was actually looking into this before and I wasn't able to think of a clean way to do this. I believe the headers are for the class are initially set empty:

sdk-platform-java/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java

Line 83 in ef4a7ff

clientCall.start(new FutureListener<>(future), HttpJsonMetadata.newBuilder().build());

Ideally, I would be able to just populate the headers there, but I couldn't find a clean way to access the context.

ManagedHttpJsonChannel's newCall only takes in callOptions and I stored it there:

sdk-platform-java/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/ManagedHttpJsonChannel.java

Lines 73 to 83 in ef4a7ff

public <RequestT, ResponseT> HttpJsonClientCall<RequestT, ResponseT> newCall(

ApiMethodDescriptor<RequestT, ResponseT> methodDescriptor, HttpJsonCallOptions callOptions) {

return new HttpJsonClientCallImpl<>(

methodDescriptor,

endpoint,

callOptions,

httpTransport,

executor,

deadlineScheduledExecutorService);

}

Actually, I think I have a bit of leeway here. I see that HttpJsonClientCalls futureUnaryCall is package-private:

sdk-platform-java/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java

Lines 79 to 80 in ef4a7ff

static <RequestT, ResponseT> ApiFuture<ResponseT> futureUnaryCall(

HttpJsonClientCall<RequestT, ResponseT> clientCall, RequestT request) {

I'm thinking about just adding HttpJsonCallContext as a third param so I can have access to the headers without having to add it to the call options. I can populate the HttpJsonMetadata with the extraHeaders in the callcontext.

gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java

gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonMetadata.java

...ax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonUnaryRequestParamCallable.java

...ase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITDynamicRoutingHeaders.java

blakeli0 · 2023-05-31T21:02:10Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/RequestParamsBuilder.java

+   * @param headerKey the header key for the routing header param
+   * @param fieldValue the field value from a request
+   */
+  public void add(String headerKey, String fieldValue) {


Now that we know we have to encode the headers, I think we should do it in a more generic way, like in RequestUrlParamsEncoder or even encode all extraHeaders.

Do we need to encode all headers? I thought it was just the routing headers since it explicitly called for it. We have this header that I believe isn't encoded: x-goog-api-client -> gl-java/11.0.19 gapic/ gax/2.27.1-SNAPSHOT rest/.

Do we need to encode all headers?

That's a great question. I think we should but I'm not sure. There are some security risks of not encoding urls, so I was under the impression that headers should be encoded and are already encoded as well by the downstream grpc or http libraries. Probably not part of this PR but we can do a little more investigation on the headers encoding in general. As part of this PR, I think we should move this logic to RequestUrlParamsEncoder because that class is specific for the routing headers and the name is perfect for this logic.

Probably not part of this PR but we can do a little more investigation on the headers encoding in general.

Sure! I will create an issue in the backlog to investigate this.

As part of this PR, I think we should move this logic to RequestUrlParamsEncoder because that class is specific for the routing headers and the name is perfect for this logic.

Yep, I agree. The only concern I have is that changing RequestUrlParamsEncoder to now encode the header values might be a behavior breaking change:

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

Lines 68 to 79 in ef4a7ff

/**

* Encodes the {@code request} in a form of a URL parameters string, for example {@code

* "param1=value+1&param2=value2%26"}. This method may optionally validate that the name-value

* paris are URL-encoded, but it will not perform the actual encoding of them (it will only

* concatenate the valid individual name-value pairs in a valid URL parameters string). This is

* so, because in most practical cases the name-value paris are already URL-encoded.

*

* @param request request message

* @throws IllegalArgumentException if is not

*/

@Override

public String encode(RequestT request) {

as it expects the params to the percent encoded already.

This class is only used for encoding the request params(which is effective routing headers), and it is marked as @InternalAPI so we should be able to change it at anytime.

I think it would make sense to have all routing headers to be built with RequestParamsBuilder (both implicit and explicit) and have it return a map of the params. It would be consistent instead of having explicit -> RequestParamsBuilder and implicit -> ImmutableMap.Builder. Both would have some light validation done with checking that the header keys and values are non-null and non-empty.

It is adding a new public method to maintain, but I think it is better than having to add more logic for handling null PathTemplates in the original method. WDYT?

@blakeli0 @lqiu96 Did this effectively change the behavior of implicit routing headers from not encoding params to now URL encoding them?

Yes, it did. There was another discussion regarding should we add it or not for existing routing headers, we discussed offline and confirmed with Slava that all routing header should be encoded.

Yep, there was a discussion with Slava about the need to encode both the header key and value. We saw there was customer issue (b/283866374) regarding having an unencoded value.

blakeli0 · 2023-06-06T15:19:12Z

gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonMetadata.java

+    Map<String, Object> extraHeaders = new HashMap<>();
+    for (Map.Entry<String, List<String>> entrySet : headers.entrySet()) {
+      // HeaderValueList is always non-null. Check that it contains at least one value.
+      // Should only ever contain one value, but take the first one if there are multiple.


Can you create an issue and link it in the comment?

Metadata/ extraHeaders Issue: #1752

I'll add this as a TODO in the comments.

blakeli0 · 2023-06-06T15:20:14Z

gax-java/gax/clirr-ignored-differences.xml

@@ -7,4 +7,10 @@
    <className>com/google/api/gax/paging/Page</className>
    <method>* stream*(*)</method>
  </difference>
+  <!-- Remove url encoding validation from RequestUrlParamsEncoder (@InternalApi) -->
+  <difference>
+    <differenceType>7004</differenceType>


Since it is an internal api, is there a way to ignore all types and methods for this class?

It seems CLIRR doesn't have a clear way to exclude based on annotation: mojohaus/clirr-maven-plugin#27

I think the second best way to ignore all types and methods for this class is to exclude this file in the clirr pom config.

blakeli0 · 2023-06-06T15:30:40Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/RequestParamsBuilder.java

+   * @param headerKey the header key for the routing header param
+   * @param fieldValue the field value from a request
+   */
+  public void add(String headerKey, String fieldValue) {


I'm good with the change in RequestUrlParamsEncoder. That change also means that we may not need this method anymore, I think we should either keep implicit dynamic routing header as it is, or move this logic to the method above, and pass PathTemplate as null for implicit dynamic routing headers. Passing null around is not really a good practice and usually having an overloaded method is cleaner, but introducing new public method also has a cost.

sonarqubecloud · 2023-06-06T16:31:12Z

[gapic-generator-java-root] SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

83.9% Coverage
9.6% Duplication

sonarqubecloud · 2023-06-06T16:35:46Z

[java_showcase_integration_tests] SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

35.5% Coverage
12.4% Duplication

sonarqubecloud · 2023-06-06T16:40:17Z

[java_showcase_unit_tests] SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

83.6% Coverage
12.4% Duplication

blakeli0 · 2023-06-06T17:51:05Z

...ttpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonDirectServerStreamingCallable.java

@@ -64,6 +64,6 @@ public void call(
    HttpJsonClientCall<RequestT, ResponseT> call = HttpJsonClientCalls.newCall(descriptor, context);
    HttpJsonDirectStreamController<RequestT, ResponseT> controller =
        new HttpJsonDirectStreamController<>(call, responseObserver);
-    controller.start(request);
+    controller.start(request, context);


Like this change!

lqiu96 added 17 commits May 3, 2023 14:07

feat: Add RequestParamsExtractor to HttpJsonCallSettings

bf396ca

feat: Add RequestParamsCallable classes

340cf32

feat: Update HttpJsonCallableFactory to use new RequestParamCallables

a32e3ff

feat: Add RequestParam to CallOptions

38e8a43

feat: Add RequestParam header for HttpJson

5fd3e61

feat: Add initial Dynamic Routing Header showcase test

0fb5793

feat: Ignore empty string for Request Param

eb68bd7

feat: Regenerate test client code

355e977

feat: Do not generate HttpBinding logic for REST

a9a12ec

feat: Add Dynamic Routing Header showcase test cases

f2fc289

feat: Move header key value to constant

9475bcc

feat: Add URLEncoding for header

bad5c10

feat: Update showcase interceptor name

103d380

feat: Generate HttpJson Dyanmic Routing Header Stub

bb730a9

feat: Fix Request Params Test

bb4dd19

feat: Update variable names

9499e4b

feat: Add gRPC Dynamic Routing Header showcase test

be3ed79

product-auto-label bot added the size: xl Pull request size is extra large. label May 5, 2023

lqiu96 changed the title ~~Main dynamic routing headers~~ feat: Dynamic Routing Headers for HttpJson May 5, 2023

lqiu96 added 11 commits May 5, 2023 17:29

feat: Clean up IT showcase test

e6bf883

chore: Add comments for RequestBuilderParams

d727746

feat: Generate test client code with Implict Routing Headers

8a72b75

feat: Regenerate test client code

0dcf1a7

fix: Generate httpjson golden test case

6fb8dcd

chore: Fix format issues

88094f4

chore: Regenerate integration golden tests

c67bd36

chore: Add more tests for RequestParamsBuilder

ca1c8f6

chore: Update docs for showcase test

25b8ad1

Merge branch 'main' into main-dynamic_routing_headers

44cfdda

chore: Refactor TestClientInitializer to include interceptors

91beb43

blakeli0 reviewed May 30, 2023

View reviewed changes

gax-java/gax/src/main/java/com/google/api/gax/rpc/RequestParamsBuilder.java Outdated Show resolved Hide resolved

chore: Use PercentEscaper for percent encoding

3038819

lqiu96 marked this pull request as ready for review May 30, 2023 21:20

lqiu96 requested a review from a team as a code owner May 30, 2023 21:20

lqiu96 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 30, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 30, 2023

lqiu96 added 2 commits May 30, 2023 17:38

chore: Empty commit

fdce4e1

chore: Migrate transport specific code to abstract class

b6b0be6

blakeli0 reviewed May 31, 2023

View reviewed changes

lqiu96 added 9 commits May 31, 2023 23:36

chore: Address pr comments

14a3ee4

chore: Address pr comments

cc17b03

chore: Use HttpJsonMetadata for headers

0133e82

chore: Revert call options changes

8f2c5f3

chore: Set default request header map

c630cf4

chore: Move encoding logic to RequestUrlParamsEncoder

14c5f90

chore: Fix lint issues

15dfc28

chore: Add clirr ignore for RequestUrlParamsEncoder

1f01dc1

Merge branch 'main' into main-dynamic_routing_headers

bbbe31d

blakeli0 reviewed Jun 6, 2023

View reviewed changes

lqiu96 added 3 commits June 6, 2023 12:09

chore: Address PR comments

55518cb

Merge branch 'main' into main-dynamic_routing_headers

6674fef

chore: Address PR comments

90c8a57

blakeli0 reviewed Jun 6, 2023

View reviewed changes

blakeli0 approved these changes Jun 6, 2023

View reviewed changes

lqiu96 merged commit 003b993 into main Jun 6, 2023

lqiu96 deleted the main-dynamic_routing_headers branch June 6, 2023 18:17

release-please bot mentioned this pull request Jun 6, 2023

chore(main): release 2.21.0 #1722

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Dynamic Routing Headers for HttpJson #1667

feat: Dynamic Routing Headers for HttpJson #1667

lqiu96 commented May 5, 2023 •

edited

Loading

blakeli0 May 31, 2023

lqiu96 Jun 1, 2023

blakeli0 Jun 1, 2023

lqiu96 Jun 1, 2023

lqiu96 Jun 5, 2023

blakeli0 May 31, 2023

lqiu96 Jun 1, 2023

lqiu96 Jun 1, 2023

blakeli0 May 31, 2023

lqiu96 Jun 1, 2023

blakeli0 Jun 1, 2023

lqiu96 Jun 1, 2023

blakeli0 Jun 2, 2023

lqiu96 Jun 6, 2023 •

edited

Loading

blakeli0 Jun 6, 2023

meltsufin Jul 19, 2023

blakeli0 Jul 19, 2023

lqiu96 Jul 19, 2023

blakeli0 Jun 6, 2023

lqiu96 Jun 6, 2023 •

edited

Loading

blakeli0 Jun 6, 2023

lqiu96 Jun 6, 2023

blakeli0 Jun 6, 2023

sonarqubecloud bot commented Jun 6, 2023

sonarqubecloud bot commented Jun 6, 2023

sonarqubecloud bot commented Jun 6, 2023

blakeli0 Jun 6, 2023

	public <RequestT, ResponseT> HttpJsonClientCall<RequestT, ResponseT> newCall(
	ApiMethodDescriptor<RequestT, ResponseT> methodDescriptor, HttpJsonCallOptions callOptions) {

	return new HttpJsonClientCallImpl<>(
	methodDescriptor,
	endpoint,
	callOptions,
	httpTransport,
	executor,
	deadlineScheduledExecutorService);
	}

	static <RequestT, ResponseT> ApiFuture<ResponseT> futureUnaryCall(
	HttpJsonClientCall<RequestT, ResponseT> clientCall, RequestT request) {

	/**
	* Encodes the {@code request} in a form of a URL parameters string, for example {@code
	* "param1=value+1&param2=value2%26"}. This method may optionally validate that the name-value
	* paris are URL-encoded, but it will not perform the actual encoding of them (it will only
	* concatenate the valid individual name-value pairs in a valid URL parameters string). This is
	* so, because in most practical cases the name-value paris are already URL-encoded.
	*
	* @param request request message
	* @throws IllegalArgumentException if is not
	*/
	@Override
	public String encode(RequestT request) {

feat: Dynamic Routing Headers for HttpJson #1667

feat: Dynamic Routing Headers for HttpJson #1667

Conversation

lqiu96 commented May 5, 2023 • edited Loading

Changes in this PR:

Showcase Request Headers

Current Behavior (No manual percent-encoding on client side)

Manually percent-encoding the header key and values

Showcase Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jun 6, 2023

sonarqubecloud bot commented Jun 6, 2023

sonarqubecloud bot commented Jun 6, 2023

Choose a reason for hiding this comment

lqiu96 commented May 5, 2023 •

edited

Loading

lqiu96 Jun 6, 2023 •

edited

Loading

lqiu96 Jun 6, 2023 •

edited

Loading