fix: REST: Make make LRO stub accept APIs of different versions (#1622)

Prior to this fix it worked only with `v1` APIs. `HttpJsonOperationsStub` is shared by multiple different services similarly to `GrpcOperationsStub`. Unlike grpc version, the verison of the actual API that this stub is used by is supposed to be encoded in the http url, so the `HttpJsonOperationsStub` must adjust to the actual version of the API it is being used from. Similarly we also need to make the url patterns less restrictive, to accept operaion names in different formats (this is what caused the `{name=}` pattern changes).

The solution is hacky, but simple and should work for all known APIs. It is based on the fact that even though the `HttpJsonOperationsStub` is shared by all clients (i.e. is in `gax`), its `HttpJsonStubCallbackFactory` argument, on the other hand, is a generated one (i.e. is in a gapic client for a specific API) and its package corresponds to the API version, which is a convention for all google APIs (i.e. version in the proto package corresponds to the version of the API).

Doing this the other way would require a lot of work (parsing yaml, generating operations stub on the fly etc), and we may come back to it if even needed (most probably never).

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

@@ -68,6 +68,8 @@ public static <RequestT, ResponseT> Builder<RequestT, ResponseT> newBuilder() {
         .setType(MethodType.UNARY);
   }
 
+  public abstract Builder<RequestT, ResponseT> toBuilder();
+
   @AutoValue.Builder
   public abstract static class Builder<RequestT, ResponseT> {
 
@@ -76,6 +78,8 @@ public abstract static class Builder<RequestT, ResponseT> {
     public abstract Builder<RequestT, ResponseT> setRequestFormatter(
         HttpRequestFormatter<RequestT> requestFormatter);
 
+    public abstract HttpRequestFormatter<RequestT> getRequestFormatter();
+
     public abstract Builder<RequestT, ResponseT> setResponseParser(
         HttpResponseParser<ResponseT> responseParser);
 

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

@@ -30,6 +30,7 @@
 package com.google.api.gax.httpjson;
 
 import com.google.api.core.BetaApi;
+import com.google.api.core.InternalApi;
 import com.google.api.pathtemplate.PathTemplate;
 import com.google.protobuf.Message;
 import java.util.List;
@@ -45,16 +46,19 @@ public class ProtoMessageRequestFormatter<RequestT extends Message>
   // Map<String, List<String>> returned value type of the getQueryParamNames interface method
   // implemented by this class.
   private final FieldsExtractor<RequestT, Map<String, List<String>>> queryParamsExtractor;
+  private final String rawPath;
   private final PathTemplate pathTemplate;
   private final FieldsExtractor<RequestT, Map<String, String>> pathVarsExtractor;
 
   private ProtoMessageRequestFormatter(
       FieldsExtractor<RequestT, String> requestBodyExtractor,
       FieldsExtractor<RequestT, Map<String, List<String>>> queryParamsExtractor,
+      String rawPath,
       PathTemplate pathTemplate,
       FieldsExtractor<RequestT, Map<String, String>> pathVarsExtractor) {
     this.requestBodyExtractor = requestBodyExtractor;
     this.queryParamsExtractor = queryParamsExtractor;
+    this.rawPath = rawPath;
     this.pathTemplate = pathTemplate;
     this.pathVarsExtractor = pathVarsExtractor;
   }
@@ -64,6 +68,13 @@ ProtoMessageRequestFormatter.Builder<RequestT> newBuilder() {
     return new Builder<>();
   }
 
+  public Builder<RequestT> toBuilder() {
+    return new Builder<RequestT>()
+        .setPath(rawPath, pathVarsExtractor)
+        .setQueryParamsExtractor(queryParamsExtractor)
+        .setRequestBodyExtractor(requestBodyExtractor);
+  }
+
   /* {@inheritDoc} */
   @Override
   public Map<String, List<String>> getQueryParamNames(RequestT apiMessage) {
@@ -93,7 +104,7 @@ public PathTemplate getPathTemplate() {
   public static class Builder<RequestT extends Message> {
     private FieldsExtractor<RequestT, String> requestBodyExtractor;
     private FieldsExtractor<RequestT, Map<String, List<String>>> queryParamsExtractor;
-    private String path;
+    private String rawPath;
     private FieldsExtractor<RequestT, Map<String, String>> pathVarsExtractor;
 
     public Builder<RequestT> setRequestBodyExtractor(
@@ -109,15 +120,25 @@ public Builder<RequestT> setQueryParamsExtractor(
     }
 
     public Builder<RequestT> setPath(
-        String path, FieldsExtractor<RequestT, Map<String, String>> pathVarsExtractor) {
-      this.path = path;
+        String rawPath, FieldsExtractor<RequestT, Map<String, String>> pathVarsExtractor) {
+      this.rawPath = rawPath;
       this.pathVarsExtractor = pathVarsExtractor;
       return this;
     }
 
+    @InternalApi
+    public Builder<RequestT> updateRawPath(String target, String replacement) {
+      this.rawPath = this.rawPath.replace(target, replacement);
+      return this;
+    }
+
     public ProtoMessageRequestFormatter<RequestT> build() {
       return new ProtoMessageRequestFormatter<>(
-          requestBodyExtractor, queryParamsExtractor, PathTemplate.create(path), pathVarsExtractor);
+          requestBodyExtractor,
+          queryParamsExtractor,
+          rawPath,
+          PathTemplate.create(rawPath),
+          pathVarsExtractor);
     }
   }
 }

gax-httpjson/src/main/java/com/google/api/gax/httpjson/longrunning/stub/HttpJsonOperationsStub.java

@@ -53,13 +53,16 @@
 import com.google.longrunning.ListOperationsResponse;
 import com.google.longrunning.Operation;
 import com.google.protobuf.Empty;
+import com.google.protobuf.Message;
 import com.google.protobuf.TypeRegistry;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 // AUTO-GENERATED DOCUMENTATION AND CLASS.
 /**
@@ -69,6 +72,9 @@
  */
 @BetaApi("A restructuring of stub classes is planned, so this may break in the future")
 public class HttpJsonOperationsStub extends OperationsStub {
+  private static final Pattern CLIENT_PACKAGE_VERSION_PATTERN =
+      Pattern.compile("\\.(?<version>v\\d+[a-zA-Z]*\\d*[a-zA-Z]*\\d*)\\.[\\w.]*stub");
+
   private static final ApiMethodDescriptor<ListOperationsRequest, ListOperationsResponse>
       listOperationsMethodDescriptor =
           ApiMethodDescriptor.<ListOperationsRequest, ListOperationsResponse>newBuilder()
@@ -77,7 +83,7 @@ public class HttpJsonOperationsStub extends OperationsStub {
               .setRequestFormatter(
                   ProtoMessageRequestFormatter.<ListOperationsRequest>newBuilder()
                       .setPath(
-                          "/v1/{name=operations}",
+                          "/v1/{name=**}/operations",
                           request -> {
                             Map<String, String> fields = new HashMap<>();
                             ProtoRestSerializer<ListOperationsRequest> serializer =
@@ -111,7 +117,7 @@ public class HttpJsonOperationsStub extends OperationsStub {
               .setRequestFormatter(
                   ProtoMessageRequestFormatter.<GetOperationRequest>newBuilder()
                       .setPath(
-                          "/v1/{name=operations/**}",
+                          "/v1/{name=**/operations/*}",
                           request -> {
                             Map<String, String> fields = new HashMap<>();
                             ProtoRestSerializer<GetOperationRequest> serializer =
@@ -141,7 +147,7 @@ public class HttpJsonOperationsStub extends OperationsStub {
               .setRequestFormatter(
                   ProtoMessageRequestFormatter.<DeleteOperationRequest>newBuilder()
                       .setPath(
-                          "/v1/{name=operations/**}",
+                          "/v1/{name=**/operations/*}",
                           request -> {
                             Map<String, String> fields = new HashMap<>();
                             ProtoRestSerializer<DeleteOperationRequest> serializer =
@@ -166,7 +172,7 @@ public class HttpJsonOperationsStub extends OperationsStub {
               .setRequestFormatter(
                   ProtoMessageRequestFormatter.<CancelOperationRequest>newBuilder()
                       .setPath(
-                          "/v1/{name=operations/**}:cancel",
+                          "/v1/{name=**/operations/*}:cancel",
                           request -> {
                             Map<String, String> fields = new HashMap<>();
                             ProtoRestSerializer<CancelOperationRequest> serializer =
@@ -249,25 +255,34 @@ protected HttpJsonOperationsStub(
       throws IOException {
     this.callableFactory = callableFactory;
 
+    Matcher packageMatcher =
+        CLIENT_PACKAGE_VERSION_PATTERN.matcher(callableFactory.getClass().getPackage().getName());
+
+    String apiVersion = packageMatcher.find() ? packageMatcher.group("version") : null;
+
     HttpJsonCallSettings<ListOperationsRequest, ListOperationsResponse>
         listOperationsTransportSettings =
             HttpJsonCallSettings.<ListOperationsRequest, ListOperationsResponse>newBuilder()
-                .setMethodDescriptor(listOperationsMethodDescriptor)
+                .setMethodDescriptor(
+                    getApiVersionedMethodDescriptor(listOperationsMethodDescriptor, apiVersion))
                 .setTypeRegistry(typeRegistry)
                 .build();
     HttpJsonCallSettings<GetOperationRequest, Operation> getOperationTransportSettings =
         HttpJsonCallSettings.<GetOperationRequest, Operation>newBuilder()
-            .setMethodDescriptor(getOperationMethodDescriptor)
+            .setMethodDescriptor(
+                getApiVersionedMethodDescriptor(getOperationMethodDescriptor, apiVersion))
             .setTypeRegistry(typeRegistry)
             .build();
     HttpJsonCallSettings<DeleteOperationRequest, Empty> deleteOperationTransportSettings =
         HttpJsonCallSettings.<DeleteOperationRequest, Empty>newBuilder()
-            .setMethodDescriptor(deleteOperationMethodDescriptor)
+            .setMethodDescriptor(
+                getApiVersionedMethodDescriptor(deleteOperationMethodDescriptor, apiVersion))
             .setTypeRegistry(typeRegistry)
             .build();
     HttpJsonCallSettings<CancelOperationRequest, Empty> cancelOperationTransportSettings =
         HttpJsonCallSettings.<CancelOperationRequest, Empty>newBuilder()
-            .setMethodDescriptor(cancelOperationMethodDescriptor)
+            .setMethodDescriptor(
+                getApiVersionedMethodDescriptor(cancelOperationMethodDescriptor, apiVersion))
             .setTypeRegistry(typeRegistry)
             .build();
 
@@ -296,6 +311,24 @@ protected HttpJsonOperationsStub(
     backgroundResources = new BackgroundResourceAggregation(clientContext.getBackgroundResources());
   }
 
+  private static <RequestT extends Message, ResponseT>
+      ApiMethodDescriptor<RequestT, ResponseT> getApiVersionedMethodDescriptor(
+          ApiMethodDescriptor<RequestT, ResponseT> methodDescriptor, String apiVersion) {
+    if (apiVersion == null) {
+      return methodDescriptor;
+    }
+
+    ApiMethodDescriptor.Builder<RequestT, ResponseT> descriptorBuilder =
+        methodDescriptor.toBuilder();
+    ProtoMessageRequestFormatter<RequestT> requestFormatter =
+        (ProtoMessageRequestFormatter<RequestT>) descriptorBuilder.getRequestFormatter();
+
+    return descriptorBuilder
+        .setRequestFormatter(
+            requestFormatter.toBuilder().updateRawPath("/v1/", '/' + apiVersion + '/').build())
+        .build();
+  }
+
   @InternalApi
   public static List<ApiMethodDescriptor> getMethodDescriptors() {
     List<ApiMethodDescriptor> methodDescriptors = new ArrayList<>();