Skip to content

Commit 229da5d

Browse files
miraleungmiraleung
authored
fix(mixins): Gate mixin RPC on HTTP rules, add yaml doc/http overrides [ggj] (#727)
* fix(mixins): Gate mixin RPC on HTTP rules, override with doc/http in yaml * fix: comment, request params
1 parent 83e2e3b commit 229da5d

11 files changed

Lines changed: 121 additions & 246 deletions

File tree

‎src/main/java/com/google/api/generator/gapic/protoparser/HttpRuleParser.java‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -50,15 +50,24 @@ public static Optional<List<String>> parseHttpBindings(
5050
checkHttpFieldIsValid(httpRule.getBody(), inputMessage, true);
5151
}
5252

53+
return parseHttpRuleHelper(httpRule, Optional.of(inputMessage), messageTypes);
54+
}
55+
56+
public static Optional<List<String>> parseHttpRule(HttpRule httpRule) {
57+
return parseHttpRuleHelper(httpRule, Optional.empty(), Collections.emptyMap());
58+
}
59+
60+
private static Optional<List<String>> parseHttpRuleHelper(
61+
HttpRule httpRule, Optional<Message> inputMessageOpt, Map<String, Message> messageTypes) {
5362
// Get pattern.
54-
Set<String> uniqueBindings = getPatternBindings(httpRule);
63+
Set<String> uniqueBindings = getHttpVerbPattern(httpRule);
5564
if (uniqueBindings.isEmpty()) {
5665
return Optional.empty();
5766
}
5867

5968
if (httpRule.getAdditionalBindingsCount() > 0) {
6069
for (HttpRule additionalRule : httpRule.getAdditionalBindingsList()) {
61-
uniqueBindings.addAll(getPatternBindings(additionalRule));
70+
uniqueBindings.addAll(getHttpVerbPattern(additionalRule));
6271
}
6372
}
6473

@@ -69,27 +78,31 @@ public static Optional<List<String>> parseHttpBindings(
6978
for (String binding : bindings) {
7079
// Handle foo.bar cases by descending into the subfields.
7180
String[] descendantBindings = binding.split("\\.");
72-
Message containingMessage = inputMessage;
81+
Optional<Message> containingMessageOpt = inputMessageOpt;
7382
for (int i = 0; i < descendantBindings.length; i++) {
7483
String subField = descendantBindings[i];
84+
if (!containingMessageOpt.isPresent()) {
85+
continue;
86+
}
87+
7588
if (i < descendantBindings.length - 1) {
76-
Field field = containingMessage.fieldMap().get(subField);
77-
containingMessage = messageTypes.get(field.type().reference().fullName());
89+
Field field = containingMessageOpt.get().fieldMap().get(subField);
90+
containingMessageOpt = Optional.of(messageTypes.get(field.type().reference().fullName()));
7891
Preconditions.checkNotNull(
79-
containingMessage,
92+
containingMessageOpt.get(),
8093
String.format(
8194
"No containing message found for field %s with type %s",
8295
field.name(), field.type().reference().simpleName()));
8396
} else {
84-
checkHttpFieldIsValid(subField, containingMessage, false);
97+
checkHttpFieldIsValid(subField, containingMessageOpt.get(), false);
8598
}
8699
}
87100
}
88101

89102
return Optional.of(bindings);
90103
}
91104

92-
private static Set<String> getPatternBindings(HttpRule httpRule) {
105+
private static Set<String> getHttpVerbPattern(HttpRule httpRule) {
93106
String pattern = null;
94107
// Assign a temp variable to prevent the formatter from removing the import.
95108
PatternCase patternCase = httpRule.getPatternCase();

‎src/main/java/com/google/api/generator/gapic/protoparser/Parser.java‎

Lines changed: 70 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
package com.google.api.generator.gapic.protoparser;
1616

1717
import com.google.api.ClientProto;
18+
import com.google.api.DocumentationRule;
19+
import com.google.api.HttpRule;
1820
import com.google.api.ResourceDescriptor;
1921
import com.google.api.ResourceProto;
2022
import com.google.api.generator.engine.ast.TypeNode;
@@ -239,6 +241,33 @@ public static List<Service> parseServices(
239241
.filter(a -> MIXIN_ALLOWLIST.contains(a.getName()))
240242
.map(a -> a.getName())
241243
.collect(Collectors.toSet());
244+
// Holds the methods to be mixed in.
245+
// Key: proto_package.ServiceName.RpcName.
246+
// Value: HTTP rules, which clobber those in the proto.
247+
// Assumes that http.rules.selector always specifies RPC names in the above format.
248+
Map<String, List<String>> mixedInMethodsToHttpRules = new HashMap<>();
249+
Map<String, String> mixedInMethodsToDocs = new HashMap<>();
250+
// Parse HTTP rules and documentation, which will override the proto.
251+
if (serviceYamlProtoOpt.isPresent()) {
252+
for (HttpRule httpRule : serviceYamlProtoOpt.get().getHttp().getRulesList()) {
253+
Optional<List<String>> httpBindingsOpt = HttpRuleParser.parseHttpRule(httpRule);
254+
if (!httpBindingsOpt.isPresent()) {
255+
continue;
256+
}
257+
for (String rpcFullNameRaw : httpRule.getSelector().split(",")) {
258+
String rpcFullName = rpcFullNameRaw.trim();
259+
mixedInMethodsToHttpRules.put(rpcFullName, httpBindingsOpt.get());
260+
}
261+
}
262+
for (DocumentationRule docRule :
263+
serviceYamlProtoOpt.get().getDocumentation().getRulesList()) {
264+
for (String rpcFullNameRaw : docRule.getSelector().split(",")) {
265+
String rpcFullName = rpcFullNameRaw.trim();
266+
mixedInMethodsToDocs.put(rpcFullName, docRule.getDescription());
267+
}
268+
}
269+
}
270+
242271
Set<String> apiDefinedRpcs = new HashSet<>();
243272
for (Service service : services) {
244273
if (blockedCodegenMixinApis.contains(service)) {
@@ -252,28 +281,55 @@ public static List<Service> parseServices(
252281
if (servicesContainBlocklistedApi && !mixedInApis.isEmpty()) {
253282
for (int i = 0; i < services.size(); i++) {
254283
Service originalService = services.get(i);
255-
List<Method> updatedMethods = new ArrayList<>(originalService.methods());
284+
List<Method> updatedOriginalServiceMethods = new ArrayList<>(originalService.methods());
256285
// If mixin APIs are present, add the methods to all other services.
257286
for (Service mixinService : blockedCodegenMixinApis) {
258-
if (!mixedInApis.contains(
259-
String.format("%s.%s", mixinService.protoPakkage(), mixinService.name()))) {
287+
final String mixinServiceFullName = serviceFullNameFn.apply(mixinService);
288+
if (!mixedInApis.contains(mixinServiceFullName)) {
260289
continue;
261290
}
262-
mixinService
263-
.methods()
291+
Function<Method, String> methodToFullProtoNameFn =
292+
m -> String.format("%s.%s", mixinServiceFullName, m.name());
293+
// Filter mixed-in methods based on those listed in the HTTP rules section of
294+
// service.yaml.
295+
List<Method> updatedMixinMethods =
296+
mixinService.methods().stream()
297+
// Mixin method inclusion is based on the HTTP rules list in service.yaml.
298+
.filter(
299+
m -> mixedInMethodsToHttpRules.containsKey(methodToFullProtoNameFn.apply(m)))
300+
.map(
301+
m -> {
302+
// HTTP rules and RPC documentation in the service.yaml file take
303+
// precedence.
304+
String fullMethodName = methodToFullProtoNameFn.apply(m);
305+
List<String> httpBindings =
306+
mixedInMethodsToHttpRules.containsKey(fullMethodName)
307+
? mixedInMethodsToHttpRules.get(fullMethodName)
308+
: m.httpBindings();
309+
String docs =
310+
mixedInMethodsToDocs.containsKey(fullMethodName)
311+
? mixedInMethodsToDocs.get(fullMethodName)
312+
: m.description();
313+
return m.toBuilder()
314+
.setHttpBindings(httpBindings)
315+
.setDescription(docs)
316+
.build();
317+
})
318+
.collect(Collectors.toList());
319+
// Overridden RPCs defined in the protos take precedence.
320+
updatedMixinMethods.stream()
321+
.filter(m -> !apiDefinedRpcs.contains(m.name()))
264322
.forEach(
265-
m -> {
266-
// Overridden RPCs defined in the protos take precedence.
267-
if (!apiDefinedRpcs.contains(m.name())) {
268-
updatedMethods.add(
323+
m ->
324+
updatedOriginalServiceMethods.add(
269325
m.toBuilder()
270326
.setMixedInApiName(serviceFullNameFn.apply(mixinService))
271-
.build());
272-
}
273-
});
274-
outputMixinServiceSet.add(mixinService);
327+
.build()));
328+
outputMixinServiceSet.add(
329+
mixinService.toBuilder().setMethods(updatedMixinMethods).build());
275330
}
276-
services.set(i, originalService.toBuilder().setMethods(updatedMethods).build());
331+
services.set(
332+
i, originalService.toBuilder().setMethods(updatedOriginalServiceMethods).build());
277333
}
278334
}
279335

‎test/integration/apis/kms/v1/cloudkms_test_mixins_v1.yaml‎

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ documentation:
2121
Gets the access control policy for a resource. Returns an empty policy
2222
if the resource exists and does not have a policy set.
2323
24+
# This RPC shouldn't appear in the proto even though the documentation field is set.
2425
- selector: google.iam.v1.IAMPolicy.SetIamPolicy
2526
description: |-
2627
Sets the access control policy on the specified resource. Replaces
@@ -29,31 +30,37 @@ documentation:
2930
Can return `NOT_FOUND`, `INVALID_ARGUMENT`, and `PERMISSION_DENIED`
3031
errors.
3132
33+
# Test the overriding of comments.
3234
- selector: google.iam.v1.IAMPolicy.TestIamPermissions
3335
description: |-
34-
Returns permissions that a caller has on the specified resource. If the
35-
resource does not exist, this will return an empty set of
36-
permissions, not a `NOT_FOUND` error.
37-
38-
Note: This operation is designed to be used for building
39-
permission-aware UIs and command-line tools, not for authorization
40-
checking. This operation may "fail open" without warning.
36+
This is a different comment for TestIamPermissions in the yaml file that should clobber the documentation in iam_policy.proto.
4137
4238
http:
4339
rules:
40+
- selector: google.cloud.location.Locations.GetLocation
41+
get: "/v1/{name=locations/*}"
42+
body: '*'
43+
# Test different HTTP verbs.
44+
- selector: google.cloud.location.Locations.ListLocations
45+
get: "/v1/{filter=*/*}/locations"
46+
body: '*'
47+
additional_bindings:
48+
- post: '/v1/{page_size=*}'
49+
body: '*'
4450
- selector: google.iam.v1.IAMPolicy.GetIamPolicy
4551
get: '/v1/{resource=projects/*/locations/*/keyRings/*}:getIamPolicy'
4652
additional_bindings:
4753
- get: '/v1/{resource=projects/*/locations/*/keyRings/*/cryptoKeys/*}:getIamPolicy'
4854
- get: '/v1/{resource=projects/*/locations/*/keyRings/*/importJobs/*}:getIamPolicy'
49-
- selector: google.iam.v1.IAMPolicy.SetIamPolicy
50-
post: '/v1/{resource=projects/*/locations/*/keyRings/*}:setIamPolicy'
51-
body: '*'
52-
additional_bindings:
53-
- post: '/v1/{resource=projects/*/locations/*/keyRings/*/cryptoKeys/*}:setIamPolicy'
54-
body: '*'
55-
- post: '/v1/{resource=projects/*/locations/*/keyRings/*/importJobs/*}:setIamPolicy'
56-
body: '*'
55+
# Test the omission of SetIamPolicy - this should no longer appear in the generated client.
56+
# - selector: google.iam.v1.IAMPolicy.SetIamPolicy
57+
# post: '/v1/{resource=projects/*/locations/*/keyRings/*}:setIamPolicy'
58+
# body: '*'
59+
# additional_bindings:
60+
# - post: '/v1/{resource=projects/*/locations/*/keyRings/*/cryptoKeys/*}:setIamPolicy'
61+
# body: '*'
62+
# - post: '/v1/{resource=projects/*/locations/*/keyRings/*/importJobs/*}:setIamPolicy'
63+
# body: '*'
5764
- selector: google.iam.v1.IAMPolicy.TestIamPermissions
5865
post: '/v1/{resource=projects/*/locations/*/keyRings/*}:testIamPermissions'
5966
body: '*'

‎test/integration/goldens/kms/com/google/cloud/kms/v1/KeyManagementServiceClient.java‎

Lines changed: 4 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@
3636
import com.google.common.util.concurrent.MoreExecutors;
3737
import com.google.iam.v1.GetIamPolicyRequest;
3838
import com.google.iam.v1.Policy;
39-
import com.google.iam.v1.SetIamPolicyRequest;
4039
import com.google.iam.v1.TestIamPermissionsRequest;
4140
import com.google.iam.v1.TestIamPermissionsResponse;
4241
import com.google.protobuf.ByteString;
@@ -3334,62 +3333,8 @@ public final UnaryCallable<GetLocationRequest, Location> getLocationCallable() {
33343333

33353334
// AUTO-GENERATED DOCUMENTATION AND METHOD.
33363335
/**
3337-
* Sets the access control policy on the specified resource. Replaces any existing policy.
3338-
*
3339-
* <p>Sample code:
3340-
*
3341-
* <pre>{@code
3342-
* try (KeyManagementServiceClient keyManagementServiceClient =
3343-
* KeyManagementServiceClient.create()) {
3344-
* SetIamPolicyRequest request =
3345-
* SetIamPolicyRequest.newBuilder()
3346-
* .setResource(KeyRingName.of("[PROJECT]", "[LOCATION]", "[KEY_RING]").toString())
3347-
* .setPolicy(Policy.newBuilder().build())
3348-
* .build();
3349-
* Policy response = keyManagementServiceClient.setIamPolicy(request);
3350-
* }
3351-
* }</pre>
3352-
*
3353-
* @param request The request object containing all of the parameters for the API call.
3354-
* @throws com.google.api.gax.rpc.ApiException if the remote call fails
3355-
*/
3356-
public final Policy setIamPolicy(SetIamPolicyRequest request) {
3357-
return setIamPolicyCallable().call(request);
3358-
}
3359-
3360-
// AUTO-GENERATED DOCUMENTATION AND METHOD.
3361-
/**
3362-
* Sets the access control policy on the specified resource. Replaces any existing policy.
3363-
*
3364-
* <p>Sample code:
3365-
*
3366-
* <pre>{@code
3367-
* try (KeyManagementServiceClient keyManagementServiceClient =
3368-
* KeyManagementServiceClient.create()) {
3369-
* SetIamPolicyRequest request =
3370-
* SetIamPolicyRequest.newBuilder()
3371-
* .setResource(KeyRingName.of("[PROJECT]", "[LOCATION]", "[KEY_RING]").toString())
3372-
* .setPolicy(Policy.newBuilder().build())
3373-
* .build();
3374-
* ApiFuture<Policy> future =
3375-
* keyManagementServiceClient.setIamPolicyCallable().futureCall(request);
3376-
* // Do something.
3377-
* Policy response = future.get();
3378-
* }
3379-
* }</pre>
3380-
*/
3381-
public final UnaryCallable<SetIamPolicyRequest, Policy> setIamPolicyCallable() {
3382-
return stub.setIamPolicyCallable();
3383-
}
3384-
3385-
// AUTO-GENERATED DOCUMENTATION AND METHOD.
3386-
/**
3387-
* Returns permissions that a caller has on the specified resource. If the resource does not
3388-
* exist, this will return an empty set of permissions, not a NOT_FOUND error.
3389-
*
3390-
* <p>Note: This operation is designed to be used for building permission-aware UIs and
3391-
* command-line tools, not for authorization checking. This operation may "fail open" without
3392-
* warning.
3336+
* This is a different comment for TestIamPermissions in the yaml file that should clobber the
3337+
* documentation in iam_policy.proto.
33933338
*
33943339
* <p>Sample code:
33953340
*
@@ -3414,12 +3359,8 @@ public final TestIamPermissionsResponse testIamPermissions(TestIamPermissionsReq
34143359

34153360
// AUTO-GENERATED DOCUMENTATION AND METHOD.
34163361
/**
3417-
* Returns permissions that a caller has on the specified resource. If the resource does not
3418-
* exist, this will return an empty set of permissions, not a NOT_FOUND error.
3419-
*
3420-
* <p>Note: This operation is designed to be used for building permission-aware UIs and
3421-
* command-line tools, not for authorization checking. This operation may "fail open" without
3422-
* warning.
3362+
* This is a different comment for TestIamPermissions in the yaml file that should clobber the
3363+
* documentation in iam_policy.proto.
34233364
*
34243365
* <p>Sample code:
34253366
*

0 commit comments

Comments
 (0)