-
Notifications
You must be signed in to change notification settings - Fork 53
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 81 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 |
---|---|---|
|
@@ -38,4 +38,4 @@ for gapic-generator-java's Bazel build. | |
|
||
```sh | ||
mvn fmt:format | ||
``` | ||
``` |
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,10 +75,10 @@ | |
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 { | ||
|
||
private static final HttpJsonServiceStubClassComposer INSTANCE = | ||
new HttpJsonServiceStubClassComposer(); | ||
|
||
|
@@ -89,6 +91,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()); | ||
|
@@ -109,7 +112,9 @@ private static TypeStore createStaticTypes() { | |
HttpJsonCallSettings.class, | ||
HttpJsonOperationSnapshot.class, | ||
HttpJsonStubCallableFactory.class, | ||
HttpRule.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, | ||
|
@@ -1089,6 +1095,47 @@ protected List<Expr> createOperationsStubInitExpr( | |
arguments.add(TYPE_REGISTRY_VAR_EXPR); | ||
} | ||
|
||
// If the Service contains custom HttpRules for Operations, we pass a map of the custom rules to | ||
// the Operations Client | ||
Map<String, HttpRule> operationCustomHttpRules = parseOperationsCustomHttpRules(context); | ||
if (operationCustomHttpRules.size() > 0) { | ||
Expr operationCustomHttpBindingsBuilderExpr = | ||
MethodInvocationExpr.builder() | ||
.setStaticReferenceType(FIXED_REST_TYPESTORE.get(ImmutableMap.class.getSimpleName())) | ||
.setMethodName("builder") | ||
.setGenerics( | ||
Arrays.asList( | ||
TypeNode.STRING.reference(), | ||
FIXED_REST_TYPESTORE.get(HttpRule.class.getSimpleName()).reference())) | ||
.build(); | ||
|
||
// Sorting is done to ensure consistent ordering of the entries in the Custom HttpRule Map | ||
for (String selector : | ||
operationCustomHttpRules.keySet().stream().sorted().collect(Collectors.toList())) { | ||
HttpRule httpRule = operationCustomHttpRules.get(selector); | ||
Expr httpRuleBuilderExpr = createHttpRuleExpr(httpRule, true); | ||
|
||
operationCustomHttpBindingsBuilderExpr = | ||
MethodInvocationExpr.builder() | ||
.setExprReferenceExpr(operationCustomHttpBindingsBuilderExpr) | ||
.setMethodName("put") | ||
.setArguments( | ||
Arrays.asList( | ||
ValueExpr.withValue(StringObjectValue.withValue(selector)), | ||
httpRuleBuilderExpr)) | ||
.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() | ||
.setVariableExpr( | ||
|
@@ -1103,6 +1150,73 @@ protected List<Expr> createOperationsStubInitExpr( | |
.build()); | ||
} | ||
|
||
/* Build an Expr that creates an HttpRule. Creates a builder and adds the http verb, custom path, and any additional bindings. `additional_bindings` can only be nested one layer deep, so we only check once */ | ||
private Expr createHttpRuleExpr(HttpRule httpRule, boolean checkAdditionalBindings) { | ||
Expr httpRuleBuilderExpr = | ||
MethodInvocationExpr.builder() | ||
.setStaticReferenceType(FIXED_REST_TYPESTORE.get(HttpRule.class.getSimpleName())) | ||
.setMethodName("newBuilder") | ||
.build(); | ||
|
||
httpRuleBuilderExpr = | ||
MethodInvocationExpr.builder() | ||
.setExprReferenceExpr(httpRuleBuilderExpr) | ||
// toLowerCase as the PatternCase result is all uppercase | ||
.setMethodName(setMethodFormat(httpRule.getPatternCase().toString().toLowerCase())) | ||
.setArguments( | ||
ValueExpr.withValue( | ||
StringObjectValue.withValue(getOperationsURIValueFromHttpRule(httpRule)))) | ||
.setReturnType(FIXED_REST_TYPESTORE.get(HttpRule.class.getSimpleName())) | ||
.build(); | ||
|
||
if (checkAdditionalBindings) { | ||
for (HttpRule additionalBindings : httpRule.getAdditionalBindingsList()) { | ||
httpRuleBuilderExpr = | ||
MethodInvocationExpr.builder() | ||
.setExprReferenceExpr(httpRuleBuilderExpr) | ||
.setMethodName("addAdditionalBindings") | ||
.setArguments(Arrays.asList(createHttpRuleExpr(additionalBindings, false))) | ||
.build(); | ||
} | ||
} | ||
|
||
httpRuleBuilderExpr = | ||
MethodInvocationExpr.builder() | ||
.setExprReferenceExpr(httpRuleBuilderExpr) | ||
.setMethodName("build") | ||
.setReturnType(FIXED_REST_TYPESTORE.get(HttpRule.class.getSimpleName())) | ||
.build(); | ||
return httpRuleBuilderExpr; | ||
} | ||
|
||
/* Parses the Service Yaml file's for custom HttpRules. Filter the HttpRules for ones that match Operations */ | ||
private Map<String, HttpRule> parseOperationsCustomHttpRules(GapicContext context) { | ||
Predicate<HttpRule> predicate = x -> x.getSelector().contains(LRO_NAME_PREFIX); | ||
com.google.api.Service service = context.serviceYamlProto(); | ||
if (service == null || service.getHttp() == null) { | ||
return ImmutableMap.of(); | ||
} | ||
return service.getHttp().getRulesList().stream() | ||
.filter(predicate) | ||
.collect(Collectors.toMap(HttpRule::getSelector, x -> x)); | ||
} | ||
|
||
/* This is meant to be used for the OperationsClient Mixin OperationsClient's RPCs are mapped to GET/POST/DELETE and this function only expects those HttpVerbs to be used */ | ||
private String getOperationsURIValueFromHttpRule(HttpRule httpRule) { | ||
switch (httpRule.getPatternCase().getNumber()) { | ||
case 2: | ||
return httpRule.getGet(); | ||
case 4: | ||
return httpRule.getPost(); | ||
case 5: | ||
return httpRule.getDelete(); | ||
default: | ||
throw new IllegalArgumentException( | ||
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 would probably write some true unit tests for this method and the method above, as the input and output are very easy to mock and verify, but I know they are mostly already covered in the golden unit tests, so as long as we covered all the cases, it's fine to me. 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'm a bit lost of this part. The Echo goldens for rest should have it right? |
||
"Operations HttpRule should only contain GET/POST/DELETE. Invalid: " | ||
+ httpRule.getSelector()); | ||
} | ||
} | ||
|
||
@Override | ||
protected List<Statement> createLongRunningClient(Service service, TypeStore typeStore) { | ||
Method pollingMethod = service.operationPollingMethod(); | ||
|
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.
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 comment
The 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.