Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

feat: REST Gapic (REGAPIC) Support #1177

Merged
merged 21 commits into from Oct 12, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
85bb35d
feat: Implement rest-gapic http transport
vam-google Jul 24, 2020
d6c2829
fix: fix bazel build
vam-google Jul 24, 2020
55b9f21
chore: Add tests to the httpjson protobuf-specific code
vam-google Sep 8, 2020
3c4c19d
chore: Add tests to the httpjson protobuf-specific code
vam-google Sep 8, 2020
aee533a
Merge remote-tracking branch 'upstream/master' into rest-gapic
vam-google Sep 8, 2020
e02e368
fix license header
vam-google Sep 8, 2020
7c08c24
Address PR feedback
vam-google Sep 12, 2020
d264bc6
Address PR feedback, add javadoc
vam-google Sep 20, 2020
3a982ff
Address PR feedback
vam-google Sep 22, 2020
a71e5d8
Merge remote-tracking branch 'upstream/master' into rest-gapic
vam-google Sep 22, 2020
07b20ba
merge upsream/master
vam-google Sep 22, 2020
7f72f2b
Address PR feedback
vam-google Oct 3, 2020
0c0328b
Merge remote-tracking branch 'upstream/master' into rest-gapic
vam-google Oct 3, 2020
5f257e6
Update license year
vam-google Oct 5, 2020
71474be
correct documentation typo
vam-google Oct 6, 2020
b231f0f
Add thrown exceptions into documentation, wrap gson exceptions into R…
vam-google Oct 6, 2020
d3b13f9
Merge remote-tracking branch 'upstream/master' into rest-gapic
vam-google Oct 7, 2020
79c41a1
Added a comment about triple nested generics
vam-google Oct 7, 2020
0dc2d89
Add character set (breaking change)
vam-google Oct 10, 2020
169fcf2
Rollback HttpResponseParser.parse() surface change
vam-google Oct 12, 2020
2619f63
Merge remote-tracking branch 'upstream/master' into rest-gapic
vam-google Oct 12, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions build.gradle
Expand Up @@ -132,6 +132,8 @@ subprojects {
'maven.io_grpc_grpc_protobuf': "io.grpc:grpc-protobuf:${libraries['version.io_grpc']}",
'maven.io_grpc_grpc_netty_shaded': "io.grpc:grpc-netty-shaded:${libraries['version.io_grpc']}",
'maven.io_grpc_grpc_alts': "io.grpc:grpc-alts:${libraries['version.io_grpc']}",
'maven.com_google_protobuf': "com.google.protobuf:protobuf-java:${libraries['version.com_google_protobuf']}",
'maven.com_google_protobuf_java_util': "com.google.protobuf:protobuf-java-util:${libraries['version.com_google_protobuf']}"
])
}

Expand Down
2 changes: 2 additions & 0 deletions gax-httpjson/BUILD.bazel
Expand Up @@ -19,6 +19,8 @@ _COMPILE_DEPS = [
"@com_google_auto_value_auto_value//jar",
"@com_google_http_client_google_http_client_jackson2//jar",
"@javax_annotation_javax_annotation_api//jar",
"@com_google_protobuf//:protobuf_java",
"@com_google_protobuf_java_util//jar",
"//gax:gax",
]

Expand Down
2 changes: 2 additions & 0 deletions gax-httpjson/build.gradle
Expand Up @@ -5,6 +5,8 @@ project.version = "0.75.3-SNAPSHOT" // {x-version-update:gax-httpjson:current}

dependencies {
compile project(':gax'),
libraries['maven.com_google_protobuf'],
libraries['maven.com_google_protobuf_java_util'],
libraries['maven.com_google_code_gson_gson'],
libraries['maven.com_google_guava_guava'],
libraries['maven.com_google_code_findbugs_jsr305'],
Expand Down
@@ -0,0 +1,34 @@
/*
* Copyright 2020 Google LLC
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
* met:
*
* * Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* * Redistributions in binary form must reproduce the above
* copyright notice, this list of conditions and the following disclaimer
* in the documentation and/or other materials provided with the
* distribution.
* * Neither the name of Google LLC nor the names of its
* contributors may be used to endorse or promote products derived from
* this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
package com.google.api.gax.httpjson;

public interface FieldsExtractor<RequestT, ParamsT> {
miraleung marked this conversation as resolved.
Show resolved Hide resolved
vchudnov-g marked this conversation as resolved.
Show resolved Hide resolved
ParamsT extract(RequestT request);
}
Expand Up @@ -103,7 +103,8 @@ HttpRequest createHttpRequest() throws IOException {
}

// Populate URL path and query parameters.
GenericUrl url = new GenericUrl(getEndpoint() + requestFormatter.getPath(getRequest()));
String endpoint = normalizeEndpoint(getEndpoint());
GenericUrl url = new GenericUrl(endpoint + requestFormatter.getPath(getRequest()));
Map<String, List<String>> queryParams = requestFormatter.getQueryParamNames(getRequest());
for (Entry<String, List<String>> queryParam : queryParams.entrySet()) {
if (queryParam.getValue() != null) {
Expand All @@ -120,6 +121,21 @@ HttpRequest createHttpRequest() throws IOException {
return httpRequest;
}

// This will be frequently executed, so avoiding using regexps if not necessary.
private String normalizeEndpoint(String endpoint) {
miraleung marked this conversation as resolved.
Show resolved Hide resolved
String normalized = endpoint;
// Set protocol as https by default if not set explicitly
if (!normalized.contains("://")) {
normalized = "https://" + normalized;
}

if (normalized.charAt(normalized.length() - 1) != '/') {
normalized += '/';
}

return normalized;
}

@Override
public void run() {
try {
Expand Down
@@ -0,0 +1,111 @@
/*
* Copyright 2020 Google LLC
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
* met:
*
* * Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* * Redistributions in binary form must reproduce the above
* copyright notice, this list of conditions and the following disclaimer
* in the documentation and/or other materials provided with the
* distribution.
* * Neither the name of Google LLC nor the names of its
* contributors may be used to endorse or promote products derived from
* this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
package com.google.api.gax.httpjson;

import com.google.api.pathtemplate.PathTemplate;
import com.google.protobuf.Message;
import java.util.List;
import java.util.Map;

public class ProtoMessageRequestFormatter<RequestT extends Message>
miraleung marked this conversation as resolved.
Show resolved Hide resolved
implements HttpRequestFormatter<RequestT> {

private final FieldsExtractor<RequestT, String> requestBodyExtractor;
private final FieldsExtractor<RequestT, Map<String, List<String>>> queryParamsExtractor;
Copy link
Contributor

Choose a reason for hiding this comment

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

triple nested generics strongly suggests that there's a more detailed object type should be created for this. I.e. avoid FieldsExtractor of Map of List.

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 agree that three nested generics is too much, but unfortunately I'm forced to do it here. This field is used solely to implement the Map<String, List<String>> getQueryParamNames(MessageFormatT apiMessage); method of this class, which, in its turn, is declared in the interface implemented by this class (HttpRequestFormatter<MessageFormatT>). I.e. this triple generic thing was predetermined by the existing architecture which I really don't want to mess with unless it is absolutely necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a brief comment to this effect, so that future well-meaning readers won't try to "fix" this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am increasingly less and less happy with GAX and GSON. Their mistakes are cascading down the dependency tree. :-(

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,
PathTemplate pathTemplate,
FieldsExtractor<RequestT, Map<String, String>> pathVarsExtractor) {
this.requestBodyExtractor = requestBodyExtractor;
this.queryParamsExtractor = queryParamsExtractor;
this.pathTemplate = pathTemplate;
this.pathVarsExtractor = pathVarsExtractor;
}

public static <RequestT extends Message>
ProtoMessageRequestFormatter.Builder<RequestT> newBuilder() {
return new Builder<>();
}

@Override
public Map<String, List<String>> getQueryParamNames(RequestT apiMessage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why a method called getQueryParamNames is returning a map? Names would seem to be a list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is implementing an existing interface. As I understand it, key is a name of a parameter, List is its values (to support array query parameters):

?param1=val2,val3
    ^         ^ 
String key   List<String> map value

return queryParamsExtractor.extract(apiMessage);
}

@Override
public String getRequestBody(RequestT apiMessage) {
return requestBodyExtractor.extract(apiMessage);
}

@Override
public String getPath(RequestT apiMessage) {
return pathTemplate.instantiate(pathVarsExtractor.extract(apiMessage));
}

@Override
public PathTemplate getPathTemplate() {
return pathTemplate;
}

public static class Builder<RequestT extends Message> {
private FieldsExtractor<RequestT, String> requestBodyExtractor;
private FieldsExtractor<RequestT, Map<String, List<String>>> queryParamsExtractor;
private String path;
private FieldsExtractor<RequestT, Map<String, String>> pathVarsExtractor;

public Builder<RequestT> setRequestBodyExtractor(
FieldsExtractor<RequestT, String> requestBodyExtractor) {
this.requestBodyExtractor = requestBodyExtractor;
return this;
}

public Builder<RequestT> setQueryParamsExtractor(
FieldsExtractor<RequestT, Map<String, List<String>>> queryParamsExtractor) {
this.queryParamsExtractor = queryParamsExtractor;
return this;
}

public Builder<RequestT> setPath(
String path, FieldsExtractor<RequestT, Map<String, String>> pathVarsExtractor) {
this.path = path;
this.pathVarsExtractor = pathVarsExtractor;
return this;
}

public ProtoMessageRequestFormatter<RequestT> build() {
return new ProtoMessageRequestFormatter<>(
requestBodyExtractor, queryParamsExtractor, PathTemplate.create(path), pathVarsExtractor);
}
}
}
@@ -0,0 +1,72 @@
/*
* Copyright 2020 Google LLC
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
* met:
*
* * Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* * Redistributions in binary form must reproduce the above
* copyright notice, this list of conditions and the following disclaimer
* in the documentation and/or other materials provided with the
* distribution.
* * Neither the name of Google LLC nor the names of its
* contributors may be used to endorse or promote products derived from
* this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
package com.google.api.gax.httpjson;

import com.google.protobuf.Message;
import java.io.InputStream;

public class ProtoMessageResponseParser<ResponseT extends Message>
miraleung marked this conversation as resolved.
Show resolved Hide resolved
implements HttpResponseParser<ResponseT> {

private final ResponseT defaultInstance;

private ProtoMessageResponseParser(ResponseT defaultInstance) {
this.defaultInstance = defaultInstance;
}

public static <RequestT extends Message>
ProtoMessageResponseParser.Builder<RequestT> newBuilder() {
return new ProtoMessageResponseParser.Builder<>();
}

@Override
public ResponseT parse(InputStream httpContent) {
return ProtoRestSerializer.<ResponseT>create()
.fromJson(httpContent, defaultInstance.newBuilderForType());
}

@Override
public String serialize(ResponseT response) {
vchudnov-g marked this conversation as resolved.
Show resolved Hide resolved

Choose a reason for hiding this comment

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

I'm curious, and maybe I'll find the answer below: when does a client need to serialize a response message into a (JSON) string? Isn't it always paring JSON into messages? Or are we making this available purely for use by servers?
(it might be nice to note the answer in a comment in the interface)

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 was wondering the same thing. I don't have a good answer for you =).
Note, this class implements an interface (HttpResponseParser), so I'm basically forced here to implement it. At the same time I can't find any prod usages of this method. It is used only once in a test, I'm not sure why. In other words, the safest and simplest thing was just to implement this method properly, especially taking into account that it takes only one line of code.

Choose a reason for hiding this comment

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

OK. Maybe a brief comment inside the function to the effect that we're not using this in prod but this is needed by the interface?

return ProtoRestSerializer.create().toJson(response);
}

public static class Builder<ResponseT extends Message> {
private ResponseT defaultInstance;

public Builder<ResponseT> setDefaultInstance(ResponseT defaultInstance) {
this.defaultInstance = defaultInstance;
return this;
}

public ProtoMessageResponseParser<ResponseT> build() {
return new ProtoMessageResponseParser<>(defaultInstance);
}
}
}
@@ -0,0 +1,105 @@
/*
* Copyright 2020 Google LLC
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
* met:
*
* * Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* * Redistributions in binary form must reproduce the above
* copyright notice, this list of conditions and the following disclaimer
* in the documentation and/or other materials provided with the
* distribution.
* * Neither the name of Google LLC nor the names of its
* contributors may be used to endorse or promote products derived from
* this software without specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
package com.google.api.gax.httpjson;

import com.google.common.collect.ImmutableList;
import com.google.protobuf.InvalidProtocolBufferException;
import com.google.protobuf.Message;
import com.google.protobuf.util.JsonFormat;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.io.Reader;
import java.util.List;
import java.util.Map;

public class ProtoRestSerializer<RequestT extends Message> {
miraleung marked this conversation as resolved.
Show resolved Hide resolved

Choose a reason for hiding this comment

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

This is serializing to and also parsing from JSON, right? Maybe you could rename it so it doesn't sound like it's just serializing. How about RequestConverterRestProto? Similarly for HttpResponseParser, which both parses and serializes. Maybe that should be called ResponseConverterRestProto?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class translates to/from text format and the message. It is not json only (also queryParameters for example). I would not call it conversion, because it does it for data to be transmitted over the network and reconstructed on the other machine (which is technically the definition of serialization, which is a more specific term than conversion).

So it technically does both serialization and deserialization. I called it just *Serializer similarly to java.io.Serializable interface, which marks classes that are expected to be both serialized and deserialized (otherwise serialization would not make sense). I could call it SerializerDeserializer, but that looks horrible. I could also split it on two classes, probably (one serializer and deserializer), but it looks as an overkill to me.

Choose a reason for hiding this comment

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

OK, I think the comment helps a lot. Thanks!

private ProtoRestSerializer() {}

public static <RequestT extends Message> ProtoRestSerializer<RequestT> create() {
return new ProtoRestSerializer<>();
}

public String toJson(RequestT message) {
try {
return JsonFormat.printer().print(message);
} catch (InvalidProtocolBufferException e) {
throw new RuntimeException("Failed to serialize message to JSON", e);

Choose a reason for hiding this comment

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

In this error message and the one below, will printing e help the user isolate where the error comes from (specific message/field)? If not, maybe we should add here some helpful info...

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'm not sure what you mean by printing. It will throw an exception, preserving the original error as "cause" argument (e). How that exception is handled and if/how it will be shown in the log depends on many other factors (it is technically beyond the scope here, we can't trace all possible try/catch constructs this method may be executed under, if any). It is a typical and idiomatic way of doing it (throw a reasonable exception, letting the outer code to deal with it).

Choose a reason for hiding this comment

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

OK, so all the info is contained in e and callers have the option of dealing with that. Sounds good.

Copy link
Contributor

Choose a reason for hiding this comment

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

RuntimeException is almost certainly wrong here. better to bubble the InvalidProtocolBufferException or just decare it in throws as an IOException if you don't want to expose the type.

Copy link
Contributor Author

@vam-google vam-google Oct 3, 2020

Choose a reason for hiding this comment

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

Thanks for pointing it out! When I was prototyping it I needed the checked exception to be silenced out and did the laziest thing possible - just wrapped it in a raw RuntimeException and forgot to clean that out.

Note, IOException is a checked exception and InvalidProtocolBufferException extends it and thus is checked too.

Instead of declaring IOExceptions in throws I declared a proper unchecked ProtoRestSerializationException. Note, these methods are explicitly called from the generated code and are part of the long chain of interactions for every single rpc call. Propagating the checked exception to the generated code and then to the long chain of various calls would be problematic and in general we do not use checked exceptions on gax surface.

}
}

@SuppressWarnings("unchecked")
miraleung marked this conversation as resolved.
Show resolved Hide resolved
public RequestT fromJson(InputStream message, Message.Builder builder) {
try (Reader json = new InputStreamReader(message)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

requires character set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

JsonFormat.parser().ignoringUnknownFields().merge(json, builder);
return (RequestT) builder.build();
} catch (IOException e) {
throw new RuntimeException("Failed to parse response message", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

same story. Please, never do this.

Copy link
Contributor Author

@vam-google vam-google Oct 3, 2020

Choose a reason for hiding this comment

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

Replaced with a newly declared ProtoRestSerializationException extends RuntimeExcdeption (same as above) to stay consistent with the rest of the exceptions in gax-java and to avoid the the hassle of propagating/catching checked exceptions in the generated code.

}
}

public void toPathParam(Map<String, String> fields, String fieldName, Object fieldValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Could this be named putPathParam or similar? When I read toFooBar, I assume the method will convert the args to a FooBar, like toBody() below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, makes sense. I guess this method evolved from returning it to putting it in a map. which is not pretty, but lets reduce the footprint of the generated gapic stub. But I did not update the name. Put makes more sense. Renamed.

if (isDefaultValue(fieldName, fieldValue)) {

Choose a reason for hiding this comment

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

I'm thinking this if is harmless but unnecessary. If the condition is true, the path segment will wind up being filled with the default value even if the if isn't here. It might be less confusing to remove it. And maybe add a comment to the effect of "while we prioritize field presence in what we send over the wire, parameters that appear in the path are ALWAYS sent over the wire."

Choose a reason for hiding this comment

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

(but I realize I'm not clear on where these put* methods are invoked)

return;
}
fields.put(fieldName, String.valueOf(fieldValue));
}

public void toQueryParam(Map<String, List<String>> fields, String fieldName, Object fieldValue) {
if (isDefaultValue(fieldName, fieldValue)) {

Choose a reason for hiding this comment

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

I don't think we want this if here, since as per §A2 in the default values doc, we need to prioritize field presence. So even if the query param the user set matches the default value, it should be included. You do this for body below, and that's correct. You don't do this for path params above, but IIUC, it won't matter because it'll get filled in either way.

Choose a reason for hiding this comment

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

(but I realize I'm not clear on where these put* methods are invoked)

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 believe I added this because it was literally breaking the execution of the generated clients. I guess it is especially relevant for strings, which have empty strings as default values, and this leads to query params being something like ?path_param=&path_param2=, which made either GCE server or the other parts of gax unhappy (or something along those lines, I don't remember specifics).

We will get back to default parameters either way. To match default values doc, plust we will have to revisit it if/when we move to using field presence feature of proto3. Please lets keep this as is for now, because it was added to fix specific failures and I have the whole toolchain tested with this present.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's great context, could you please add a comment?

Choose a reason for hiding this comment

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

Sounds reasonable, but let's flag this more prominently. Could you add below your existing comment something like TODO: Revisit this approach to ensure proper default-value handling as per design.

return;
}

ImmutableList.Builder<String> paramValueList = ImmutableList.builder();
if (fieldValue instanceof List<?>) {
for (Object fieldValueItem : (List<?>) fieldValue) {
paramValueList.add(String.valueOf(fieldValueItem));
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: Curious whether gax-java is still on Java 7, otherwise we could use a forEach here.

Copy link
Contributor

Choose a reason for hiding this comment

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

GAX is still on Java 7, as are all GCP libraries. See go/nojava7

}
} else {
paramValueList.add(String.valueOf(fieldValue));
}

fields.put(fieldName, paramValueList.build());
}

public String toBody(String fieldName, RequestT fieldValue) {
return toJson(fieldValue);
}

private boolean isDefaultValue(String fieldName, Object fieldValue) {
if (fieldValue instanceof Number) {
return ((Number) fieldValue).longValue() == 0L;
} else if (fieldValue instanceof String) {
return ((String) fieldValue).isEmpty();
}

return false;
}
}