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 14 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 @@ -20,6 +20,8 @@ _COMPILE_DEPS = [
"@com_google_auto_value_auto_value_annotations//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.76.0" // {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,41 @@
/*
* 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;

/**
* A functional interface to be implemented for each request message to extract specific fields from
* it. For advanced usage only.
*/
@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.

HTTP response

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

*/
MessageFormatT parse(InputStream httpContent);

/* Serialize an object into an HTTP body, which is written out to output.
*
* @param response the object to serialize.
* @param output the output stream to append the serialization to. */
* @param response the object to serialize
*/
@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);
}
}
}
@@ -0,0 +1,46 @@
/*
* 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;

/**
* An exception thrown when a protobuf message cannot be serialized/deserialized for REST
* interactions.
*/
@BetaApi
public class ProtoRestSerializationException extends RuntimeException {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the proto comes from external sources, as they usually do, this should be a checked exception.

Copy link
Contributor Author

@vam-google vam-google Oct 5, 2020

Choose a reason for hiding this comment

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

Unfortunately I can't make it checked:

  1. There is a long-standing controversy around checked vs unchecked exceptions (for example: https://www.ibm.com/developerworks/library/j-jtp05254/index.html). This does not prove that checked exceptions are wrong, but their application is not something that community agrees upon unanimously.

  2. gax-java architecture does not foresee usage of checked exceptions on its surface. If any exception happens during a call on any of the stages, sooner or later it will be wrapped to an unchecked exception, most likely one of the defined in gax itself. I can't change it without rewriting in a backward-incompatible way the whole gax's exception handling logic. This means that if I don't wrap a checked IOException into an unchecked one here, it will be wrapped somewhere up the stack before a user can see it, plus going through the trouble of propagating the checked aspect of it explicitly to that level.

  3. For example, check ApiExceptionFactory - all of the created exceptions are unchecked.
    Many of those exceptions mean that the particular call in which they were thrown must be terminated and cannot be completed successfully (i.e. there is no reasonable way to recover from the exception except starting the whole call over again). This is in alignment with Oracle's recommendation (https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html): "If a client can reasonably be expected to recover from an exception, make it a checked exception. If a client cannot do anything to recover from the exception, make it an unchecked exception". If the ProtoRestSerializationException is thrown the only thing a user can do is to fix something in their call and try it over again from the beginning.

  4. On practice, for this particular case, an attempt to keep the exceptions checked hits the wall of troubles almost immediately. Specifically ProtoRestSerializer#toJson() is called from ProtoMessageResponseParser#parse(), which, in its turn implements HttpResponseParser#parse() interface method. To propagate it further, we will have to add throws IOException to the interface method, which is a breaking change, or wrap it in an unchecked on the HttpResponseParser#parse() level. This is where it begins, if we propagate further if will get more and more complicated until it gets wrapped anyways or gax-java exception handling model is revisited as a whole.

Copy link
Contributor

Choose a reason for hiding this comment

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

HttpResponseParser#parse() does not document that it throws any exceptions. That strongly suggests it doesn't want any to be thrown, not that it thinks throwing a runtime exception is OK. You might need to return a default value here or null instead of throwing an exception.

Copy link
Contributor Author

@vam-google vam-google Oct 5, 2020

Choose a reason for hiding this comment

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

Yes, the interface does not document the thrown exceptions, but they are definitely possible by the existing implementaiton of this interface ApiMessageHttpResponseParser#l94, that calls Gson#fromJson, which throws two exceptions: JsonIOException, JsonSyntaxException, both unchecked (JsonIOException is also unchecked, even though it is named like it extends IOException, while it does not). Gson (google's libraryh) implementation seems following the no-checked exceptions philosophy, same as gax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Swallowing the error information by returning null in case of an exception seems worse than throwing an unchecked exception, plus the existing implementation already throws them. WDYT about just updating the interface method documentation?

Copy link
Contributor Author

@vam-google vam-google Oct 6, 2020

Choose a reason for hiding this comment

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

I renamed ProtoRestSerializationException to RestSerializationException (to make it less proto-specific), added it (still as an unchecked exception) to the documentation of the ApiMessageHttpResponseParse and in other relevant classes.

Note, ApiMessageHttpResponseParse had broken documentation block, so stricktly speaking it never had documentation, but rather had a multi-line comment (/* */ was used instead of /** */).


private static final long serialVersionUID = -6485633460933364916L;

public ProtoRestSerializationException(String message, Throwable cause) {
super(message, cause);
}
}