Skip to content
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

feat: Add support for additional_bindings #993

Merged
merged 1 commit into from
May 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,16 @@ private List<Expr> getRequestFormatterExpr(Method protoMethod) {
"putPathParam")))
.apply(expr);

if (!protoMethod.httpBindings().lowerCamelAdditionalPatterns().isEmpty()) {
expr =
methodMaker
.apply(
"setAdditionalPaths",
protoMethod.httpBindings().lowerCamelAdditionalPatterns().stream()
.map(a -> ValueExpr.withValue(StringObjectValue.withValue(a)))
.collect(Collectors.toList()))
.apply(expr);
}
TypeNode fieldsVarGenericType =
TypeNode.withReference(
ConcreteReference.builder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableSet;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import javax.annotation.Nullable;

@AutoValue
Expand Down Expand Up @@ -63,6 +65,8 @@ public int compareTo(HttpBinding o) {

public abstract String pattern();

public abstract List<String> additionalPatterns();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to use List here? Can we use Set? Does order matter? Do duplicates matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think technically duplicates are allowed even though they will be useless. Bindings come in list from proto file descriptor, so keeping them as list here.


public abstract Set<HttpBinding> pathParameters();

public abstract Set<HttpBinding> queryParameters();
Expand All @@ -84,8 +88,18 @@ public static HttpBindings.Builder builder() {
// in .proto file: "/global/instanceTemplates/{instance_template=*}"
// in .java file: "/global/instanceTemplates/{instanceTemplate=*}"
public String lowerCamelPattern() {
String lowerCamelPattern = pattern();
for (HttpBinding pathParam : pathParameters()) {
return lowerCamelPattern(pattern(), pathParameters());
}

public List<String> lowerCamelAdditionalPatterns() {
return additionalPatterns().stream()
.map(a -> lowerCamelPattern(a, pathParameters()))
.collect(Collectors.toList());
}

private static String lowerCamelPattern(String originalPattern, Set<HttpBinding> pathParameters) {
String lowerCamelPattern = originalPattern;
for (HttpBinding pathParam : pathParameters) {
lowerCamelPattern =
lowerCamelPattern.replaceAll(
"\\{" + pathParam.name(), "{" + JavaStyle.toLowerCamelCase(pathParam.name()));
Expand All @@ -107,6 +121,8 @@ public abstract static class Builder {

public abstract HttpBindings.Builder setPattern(String pattern);

public abstract HttpBindings.Builder setAdditionalPatterns(List<String> additionalPatterns);

abstract String pattern();

public abstract HttpBindings.Builder setPathParameters(Set<HttpBinding> pathParameters);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

public class HttpRuleParser {
private static final String ASTERISK = "*";
Expand Down Expand Up @@ -68,11 +69,12 @@ private static HttpBindings parseHttpRuleHelper(
// Get pattern.
String pattern = getHttpVerbPattern(httpRule);
ImmutableSet.Builder<String> bindingsBuilder = ImmutableSet.builder();
bindingsBuilder.addAll(PatternParser.getPattenBindings(pattern));
bindingsBuilder.addAll(PatternParser.getPatternBindings(pattern));
if (httpRule.getAdditionalBindingsCount() > 0) {
for (HttpRule additionalRule : httpRule.getAdditionalBindingsList()) {
// TODO: save additional bindings path in HttpRuleBindings
bindingsBuilder.addAll(PatternParser.getPattenBindings(getHttpVerbPattern(additionalRule)));
bindingsBuilder.addAll(
PatternParser.getPatternBindings(getHttpVerbPattern(additionalRule)));
}
}

Expand Down Expand Up @@ -104,6 +106,10 @@ private static HttpBindings parseHttpRuleHelper(
return HttpBindings.builder()
.setHttpVerb(HttpBindings.HttpVerb.valueOf(httpRule.getPatternCase().toString()))
.setPattern(pattern)
.setAdditionalPatterns(
httpRule.getAdditionalBindingsList().stream()
.map(HttpRuleParser::getHttpVerbPattern)
.collect(Collectors.toList()))
.setPathParameters(
validateAndConstructHttpBindings(
pathParamNames, message, messageTypes, patternSampleValues))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public class PatternParser {

// This method tries to parse all named segments from pattern and sort in natual order
// e.g. /v1beta1/{table_name=tests/*}/{routing_id=instances/*}/** -> (routing_id, table_name)
public static Set<String> getPattenBindings(String pattern) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Thanks for correcting my mistake!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I actually thought it was my typo, only now realized that it was the class you created =).

public static Set<String> getPatternBindings(String pattern) {
ImmutableSortedSet.Builder<String> bindings = ImmutableSortedSet.naturalOrder();
if (pattern.isEmpty()) {
return bindings.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public static RoutingHeaderRule parse(
key = fieldName;
pathTemplate = String.format("{%s=**}", key);
} else {
Set<String> namedSegments = PatternParser.getPattenBindings(pathTemplate);
Set<String> namedSegments = PatternParser.getPatternBindings(pathTemplate);
Preconditions.checkArgument(
namedSegments.size() == 1,
String.format(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,9 @@ public class HttpJsonEchoStub extends EchoStub {
fields, "fooBar.name", request.getFooBar().getName());
return fields;
})
.setAdditionalPaths(
"/v1beta1/{fooBar.name=cats/*/dogs/*}/echo:foo",
"/v1beta1/{fooBar.name=turtles/*/parrots/*}/echo:foo")
.setQueryParamsExtractor(
request -> {
Map<String, List<String>> fields = new HashMap<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.google.api.generator.gapic.model.HttpBindings.HttpVerb;
import com.google.api.generator.gapic.model.RoutingHeaderRule.RoutingHeaderParam;
import com.google.common.collect.ImmutableSet;
import java.util.Arrays;
import org.junit.Test;

public class MethodTest {
Expand All @@ -35,6 +36,7 @@ public class MethodTest {
HttpBindings.builder()
.setPathParameters(ImmutableSet.of(HttpBinding.create("table", true, false, "")))
.setPattern("/pattern/test")
.setAdditionalPatterns(Arrays.asList("/extra_pattern/test", "/extra_pattern/hey"))
.setIsAsteriskBody(false)
.setHttpVerb(HttpVerb.GET)
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,19 @@
public class PatternParserTest {
@Test
public void getPattenBindings_shouldReturnEmptySetIfPatternIsEmpty() {
assertThat(PatternParser.getPattenBindings("")).isEmpty();
assertThat(PatternParser.getPatternBindings("")).isEmpty();
}

@Test
public void getPattenBindings_shouldFilterOutUnboundVariables() {
Set<String> actual = PatternParser.getPattenBindings("{routing_id=projects/*}/**");
Set<String> actual = PatternParser.getPatternBindings("{routing_id=projects/*}/**");
assertThat(actual).hasSize(1);
}

@Test
public void getPattenBindings_shouldReturnBindingsInNatualOrder() {
Set<String> actual =
PatternParser.getPattenBindings("{routing_id=projects/*}/{name=instance/*}");
PatternParser.getPatternBindings("{routing_id=projects/*}/{name=instance/*}");
assertThat(actual).containsExactly("name", "routing_id").inOrder();
}
}
8 changes: 8 additions & 0 deletions src/test/proto/echo_grpcrest.proto
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,14 @@ service Echo {
option (google.api.http) = {
post: "/v1beta1/{foo_bar.name=projects/*/foobars/*}/echo:foo"
body: "*"
additional_bindings {
post: "/v1beta1/{foo_bar.name=cats/*/dogs/*}/echo:foo"
body: "*"
}
additional_bindings {
post: "/v1beta1/{foo_bar.name=turtles/*/parrots/*}/echo:foo"
body: "*"
}
};
}
}
Expand Down