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 8 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,37 @@
/*
* 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.core.BetaApi;

@BetaApi
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
Expand Up @@ -39,13 +39,14 @@ public interface HttpResponseParser<MessageFormatT> {

/* Parse the http body content JSON stream into the MessageFormatT.
*
* @param httpContent the body of an http response. */
* @param httpContent the body of an http response.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no period per Oracle guidelines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected. It seems like we don't have consistency regarding this in our gax library (some docs use dots some don't)

*/
MessageFormatT parse(InputStream httpContent);

/* Serialize an object into an HTTP body, which is written out to output.
*
* @param response the object to serialize.
Copy link
Contributor

Choose a reason for hiding this comment

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

no period

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected

* @param output the output stream to append the serialization to. */
*/
@InternalApi
String serialize(MessageFormatT response);
}
@@ -0,0 +1,120 @@
/*
* 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.core.BetaApi;
import com.google.api.pathtemplate.PathTemplate;
import com.google.protobuf.Message;
import java.util.List;
import java.util.Map;

/** Creates parts of a HTTP request from a protobuf message. */
@BetaApi
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<>();
}

/* {@inheritDoc} */
@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);
}

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

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

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

// This has class has compound setter methods (multiple arguments in setters), that is why not
// using @AutoValue.
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,78 @@
/*
* 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.core.BetaApi;
import com.google.protobuf.Message;
import java.io.InputStream;

/** The implementation of {@link HttpResponseParser} which works with protobuf messages. */
@BetaApi
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<>();
}

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

/* {@inheritDoc} */
@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);
}

// Convert to @AutoValue if this class gets more complicated
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);
}
}
}