-
Notifications
You must be signed in to change notification settings - Fork 54
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
fix: Allow custom HttpRules for REST LROs #1288
Changes from 18 commits
4c14e48
8f5e313
a693085
f4baa0a
7bde2cf
a3527e1
2e1acbc
5f3dfe2
d8761bc
d837f97
dfd1e5a
0a891dc
51bde57
64fc726
09031c8
568ad44
3a72781
d7d1b12
9b050d3
5b31d41
316621c
d8e488f
46156e0
8a3ba93
1cd51fb
ec8ce91
443adec
c70d183
db34e1d
5fce328
5af299d
5f3e732
78c6d89
472a3ba
ebe84cc
6f8cbe0
1ae7726
e8d86df
b8afde0
445769c
9b42094
24dabca
dc40d5a
eeb5665
40e49da
4356783
9a636bb
1b3e292
a64100f
e8fb415
09b787c
94c00fb
d5453b7
667c3cf
e24b42f
62edb0e
c2cc0a8
4f9015f
c7631ba
674d814
52aaa23
1670957
abebe3d
0a70fe7
ff142e2
3f250ab
71b1244
98c1cfb
25acf97
f0a1692
85bcf90
166f6c4
7af9882
a68927e
0c4eec3
da232d2
1f40c11
88f7961
8f7aed0
31932b1
88487e3
4f6e39c
8d4d54c
9a4f437
350b811
bfe3753
1d5d967
fe6c4c7
c15e2b5
2c489ff
78be800
f179078
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
|
||
package com.google.api.generator.gapic.composer.rest; | ||
|
||
import com.google.api.HttpRule; | ||
import com.google.api.core.InternalApi; | ||
import com.google.api.gax.httpjson.ApiMethodDescriptor; | ||
import com.google.api.gax.httpjson.ApiMethodDescriptor.MethodType; | ||
|
@@ -63,6 +64,7 @@ | |
import com.google.common.annotations.VisibleForTesting; | ||
import com.google.common.collect.BiMap; | ||
import com.google.common.collect.ImmutableList; | ||
import com.google.common.collect.ImmutableMap; | ||
import com.google.protobuf.TypeRegistry; | ||
import java.util.ArrayList; | ||
import java.util.Arrays; | ||
|
@@ -73,6 +75,7 @@ | |
import java.util.Set; | ||
import java.util.function.BiFunction; | ||
import java.util.function.Function; | ||
import java.util.function.Predicate; | ||
import java.util.stream.Collectors; | ||
|
||
public class HttpJsonServiceStubClassComposer extends AbstractTransportServiceStubClassComposer { | ||
|
@@ -89,6 +92,7 @@ public class HttpJsonServiceStubClassComposer extends AbstractTransportServiceSt | |
.setType(FIXED_REST_TYPESTORE.get(TypeRegistry.class.getSimpleName())) | ||
.build()) | ||
.build(); | ||
private static final String LRO_NAME_PREFIX = "google.longrunning.Operations"; | ||
|
||
protected HttpJsonServiceStubClassComposer() { | ||
super(RestContext.instance()); | ||
|
@@ -110,6 +114,7 @@ private static TypeStore createStaticTypes() { | |
HttpJsonOperationSnapshot.class, | ||
HttpJsonStubCallableFactory.class, | ||
Map.class, | ||
ImmutableMap.class, | ||
ProtoMessageRequestFormatter.class, | ||
ProtoMessageResponseParser.class, | ||
ProtoRestSerializer.class, | ||
|
@@ -1075,6 +1080,7 @@ private List<Expr> getMethodTypeExpr(Method protoMethod) { | |
|
||
@Override | ||
protected List<Expr> createOperationsStubInitExpr( | ||
GapicContext context, | ||
Service service, | ||
Expr thisExpr, | ||
VariableExpr operationsStubClassVarExpr, | ||
|
@@ -1088,6 +1094,40 @@ protected List<Expr> createOperationsStubInitExpr( | |
if (standardOpStub.equals(operationsStubType.reference().fullName())) { | ||
arguments.add(TYPE_REGISTRY_VAR_EXPR); | ||
} | ||
Map<String, String> customHttpBindings = parseCustomHttpBindings(context); | ||
Map<String, String> operationCustomHttpBindings = | ||
filterCustomHttpBindingsMap(customHttpBindings, x -> x.getKey().contains(LRO_NAME_PREFIX)); | ||
if (operationCustomHttpBindings.size() > 0) { | ||
Expr operationCustomHttpBindingsBuilderExpr = | ||
MethodInvocationExpr.builder() | ||
.setStaticReferenceType(FIXED_REST_TYPESTORE.get(ImmutableMap.class.getSimpleName())) | ||
.setMethodName("builder") | ||
.setGenerics(Arrays.asList(TypeNode.STRING.reference(), TypeNode.STRING.reference())) | ||
.build(); | ||
|
||
for (Map.Entry<String, String> entrySet : operationCustomHttpBindings.entrySet()) { | ||
String selector = entrySet.getKey(); | ||
String path = entrySet.getValue(); | ||
operationCustomHttpBindingsBuilderExpr = | ||
MethodInvocationExpr.builder() | ||
.setExprReferenceExpr(operationCustomHttpBindingsBuilderExpr) | ||
.setMethodName("put") | ||
.setArguments( | ||
Arrays.asList( | ||
ValueExpr.withValue(StringObjectValue.withValue(selector)), | ||
ValueExpr.withValue(StringObjectValue.withValue(path)))) | ||
.build(); | ||
} | ||
|
||
operationCustomHttpBindingsBuilderExpr = | ||
MethodInvocationExpr.builder() | ||
.setExprReferenceExpr(operationCustomHttpBindingsBuilderExpr) | ||
.setMethodName("build") | ||
.setReturnType(FIXED_REST_TYPESTORE.get(ImmutableMap.class.getSimpleName())) | ||
.build(); | ||
|
||
arguments.add(operationCustomHttpBindingsBuilderExpr); | ||
} | ||
|
||
return Collections.singletonList( | ||
AssignmentExpr.builder() | ||
|
@@ -1103,6 +1143,43 @@ protected List<Expr> createOperationsStubInitExpr( | |
.build()); | ||
} | ||
|
||
private Map<String, String> parseCustomHttpBindings(GapicContext context) { | ||
Map<String, String> customHttpBindings = new HashMap<>(); | ||
com.google.api.Service service = context.serviceYamlProto(); | ||
if (service != null && service.getHttp() != null) { | ||
for (HttpRule httpRule : service.getHttp().getRulesList()) { | ||
customHttpBindings.put(httpRule.getSelector(), getValueBasedOnPatternCase(httpRule)); | ||
} | ||
} | ||
return customHttpBindings; | ||
} | ||
|
||
private Map<String, String> filterCustomHttpBindingsMap( | ||
Map<String, String> customHttpBindings, Predicate<Map.Entry<String, String>> predicate) { | ||
return customHttpBindings.entrySet().stream() | ||
.filter(predicate) | ||
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); | ||
} | ||
|
||
private String getValueBasedOnPatternCase(HttpRule httpRule) { | ||
switch (httpRule.getPatternCase().getNumber()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be good if we can provide a reference to the http proto for these mappings. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll remember to add it in later. I'm not sure if this is the best approach yet, so I haven't been adding any docs or tests yet. |
||
case 2: | ||
return httpRule.getGet(); | ||
case 3: | ||
return httpRule.getPut(); | ||
case 4: | ||
return httpRule.getPost(); | ||
case 5: | ||
return httpRule.getDelete(); | ||
case 6: | ||
return httpRule.getPatch(); | ||
case 8: | ||
return httpRule.getCustom().getPath(); | ||
default: | ||
return null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This condition should never happen, but in case it happens, can we throw a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah makes sense. Since this is really only meant for handling Operations, do you think I should also only catch for GET, POST, DELETE and throw I'm leaning towards that and documenting that this is meant only for the Operations use case (and I think this one off change should only handle Operations) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I agree. |
||
} | ||
} | ||
|
||
@Override | ||
protected List<Statement> createLongRunningClient(Service service, TypeStore typeStore) { | ||
Method pollingMethod = service.operationPollingMethod(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -147,6 +147,12 @@ public Builder<RequestT> setAdditionalPaths(String... rawAdditionalPaths) { | |
return this; | ||
} | ||
|
||
@InternalApi | ||
public Builder<RequestT> updateRawPath(String rawPath) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to initialize the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The I might be missing something obvious, but I'm not seeing a way to change this value without updating once I get the customHttpBindings map. |
||
this.rawPath = rawPath; | ||
return this; | ||
} | ||
|
||
@InternalApi | ||
public Builder<RequestT> updateRawPath(String target, String replacement) { | ||
this.rawPath = this.rawPath.replace(target, replacement); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add all the default
operation -> url
mapping to this map and override then if needed. Basically we need to move as much code as possible to the generator from gax, as gax is a runtime dependency and it would be much harder to make changes there later.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add the defaults for bunch of mixins (or for only Operation module). I just saw that it is possible to have way more mixins (https://github.com/googleapis/gapic-showcase/blob/b94ecfc51059b49770e5bdb6f0d7ea07903158e8/schema/google/showcase/v1beta1/showcase_v1beta1.yaml#L46-L85) so I didn't want to just create a default list in the gapic-generator that needed to be manually updated later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I think that makes sense, we could do it the other way also, construct a map with all the bindings from the yaml, and populate default for the LRO operations. Or if we are confident about the list of mixins, we could pre-populate them as well, a related question, did you get a chance to see how mixins work for location and iam? Are we going to have similar problem for them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can see, I think only the Operation proto is currently being used. I would imagine if Location and IAM are used, they would be created the same way the Operations Client was in GAX: googleapis/gax-java#1456 and we would reference it via https://github.com/googleapis/google-cloud-java/blob/10bb0cb494f64b864408ede46834e1046351370c/java-speech/google-cloud-speech/src/main/java/com/google/cloud/speech/v1/SpeechClient.java#L164-L166.
I was running under the assumption that they would be added in sometime in the future, but I'm not sure about it. Plus each generated client (operation/ iam/ location) would have the initial defaults set from the proto file: https://github.com/googleapis/gapic-generator-java/blob/d1a16195937df041f65a52717ddf9dc6ceb09b4f/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/longrunning/stub/HttpJsonOperationsStub.java#L84