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: REST Gapic (REGAPIC) Support #1177

Merged
merged 21 commits into from Oct 12, 2020
Merged

Conversation

vam-google
Copy link
Contributor

@vam-google vam-google commented Sep 8, 2020

This is needed to support gapic clients which use protobuf stubs for data model and httpjson/rest transport for transmitting them over the wire.

@google-cla google-cla bot added the cla: yes label Sep 8, 2020
@vam-google vam-google requested review from miraleung and vchudnov-g Sep 8, 2020
@codecov
Copy link

@codecov codecov bot commented Sep 8, 2020

Codecov Report

Merging #1177 into master will increase coverage by 0.21%.
The diff coverage is 89.61%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1177      +/-   ##
============================================
+ Coverage     79.14%   79.36%   +0.21%     
- Complexity     1198     1227      +29     
============================================
  Files           205      209       +4     
  Lines          5270     5344      +74     
  Branches        436      442       +6     
============================================
+ Hits           4171     4241      +70     
- Misses          925      930       +5     
+ Partials        174      173       -1     
Impacted Files Coverage Δ Complexity Δ
...api/gax/httpjson/ApiMessageHttpResponseParser.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...e/api/gax/httpjson/RestSerializationException.java 50.00% <50.00%> (ø) 1.00 <1.00> (?)
...m/google/api/gax/httpjson/ProtoRestSerializer.java 93.10% <93.10%> (ø) 14.00 <14.00> (?)
...m/google/api/gax/httpjson/HttpRequestRunnable.java 60.00% <100.00%> (+5.83%) 8.00 <3.00> (+3.00)
...api/gax/httpjson/ProtoMessageRequestFormatter.java 100.00% <100.00%> (ø) 6.00 <6.00> (?)
...e/api/gax/httpjson/ProtoMessageResponseParser.java 100.00% <100.00%> (ø) 4.00 <4.00> (?)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b856351...2619f63. Read the comment docs.

}
}

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

@miraleung miraleung Sep 8, 2020

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

@vam-google vam-google Sep 12, 2020

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.

@vam-google
Copy link
Contributor Author

@vam-google vam-google commented Sep 12, 2020

@miraleung PTAL

Copy link

@vchudnov-g vchudnov-g left a comment

Some questions about default values--it might be my own misunderstanding, but I want to ensure we're on the same page.

Also some requests for expanded doc/renaming.

I think this looks good. Modulo the questions above, I follow your approach!

}

@Override
public String serialize(ResponseT response) {

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

@vam-google vam-google Sep 18, 2020

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?

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

@vam-google vam-google Sep 18, 2020

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.

@@ -0,0 +1,141 @@
/*
* Copyright 2018 Google LLC

Choose a reason for hiding this comment

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

2020

Copy link
Contributor Author

@vam-google vam-google Sep 18, 2020

Choose a reason for hiding this comment

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

Fixed

try {
requestSerializer.fromJson(
new ByteArrayInputStream("heh".getBytes(StandardCharsets.UTF_8)), Field.newBuilder());
Truth.assertThat(true).isFalse();

Choose a reason for hiding this comment

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

Is there a way to simply assert false, preferably with a message that parsing should have failed?

Copy link
Contributor

@miraleung miraleung Sep 21, 2020

Choose a reason for hiding this comment

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

Ditto on assertThrows.

"{\n"
+ " \"cardinality\": \"CARDINALITY_OPTIONAL\",\n"
+ " \"number\": 2,\n"
+ " \"name\": \"field_name1\",\n"

Choose a reason for hiding this comment

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

super nitty nit, feel free to ignore: consider using whimsical names for values that are completely arbitrary, just to make that super-obvious (eg "foofoo", "oscar", "olivia")

public void parseInvalidJson() {
try {
parser.parse(new ByteArrayInputStream("invalid".getBytes(StandardCharsets.UTF_8)));
Truth.assertThat(true).isFalse();

Choose a reason for hiding this comment

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

Is there a way to simply assert false, preferably with a message that parsing should have failed?

Copy link
Contributor Author

@vam-google vam-google Sep 18, 2020

Choose a reason for hiding this comment

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

I could not find a way to do it in Truth =(. You can do it in vanilla junit, but since we are using Truth in gax, I wanted to be consistent so used truth even here. As for a comment, I think it is not necessary.

Copy link
Contributor

@miraleung miraleung Sep 21, 2020

Choose a reason for hiding this comment

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

What about assertThrows? Looks like Truth's authors recommend sticking with vanilla JUnit for exceptions.

Copy link
Contributor Author

@vam-google vam-google Sep 22, 2020

Choose a reason for hiding this comment

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

Ok I just switched to vanilla Assert.fail() version. assertThrows if fine, but it seems to be designed with java8+lambdas in mind. In java7 world (that we are stuck in here) using it with the explicit internal anonymous classes looks way to gnarly.

expectedFields.put("optName1", "1");
expectedFields.put("optName3", "three");

Truth.assertThat(fields).isEqualTo(expectedFields);

Choose a reason for hiding this comment

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

What does it mean for optName2 optName4 to not be here? They're still settable by the app developer, correct?

Copy link
Contributor Author

@vam-google vam-google Sep 18, 2020

Choose a reason for hiding this comment

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

This tests for not putting default values. As we mentioned earlier, the whole default values thing will be revisited either way. This is what i needed to make MVP work.

Choose a reason for hiding this comment

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

Ah, OK.

@vam-google vam-google requested review from miraleung and vchudnov-g Sep 21, 2020
Copy link
Contributor

@miraleung miraleung left a comment

LGTM with nits addressed.

}

public void putQueryParam(Map<String, List<String>> fields, String fieldName, Object fieldValue) {
if (isDefaultValue(fieldName, fieldValue)) {
Copy link
Contributor

@miraleung miraleung Sep 21, 2020

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?

public void parseInvalidJson() {
try {
parser.parse(new ByteArrayInputStream("invalid".getBytes(StandardCharsets.UTF_8)));
Truth.assertThat(true).isFalse();
Copy link
Contributor

@miraleung miraleung Sep 21, 2020

Choose a reason for hiding this comment

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

What about assertThrows? Looks like Truth's authors recommend sticking with vanilla JUnit for exceptions.

try {
requestSerializer.fromJson(
new ByteArrayInputStream("heh".getBytes(StandardCharsets.UTF_8)), Field.newBuilder());
Truth.assertThat(true).isFalse();
Copy link
Contributor

@miraleung miraleung Sep 21, 2020

Choose a reason for hiding this comment

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

Ditto on assertThrows.

@vam-google vam-google requested a review from as a code owner Sep 22, 2020
@vam-google vam-google requested a review from miraleung Sep 23, 2020
@vam-google
Copy link
Contributor Author

@vam-google vam-google commented Sep 23, 2020

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

@miraleung miraleung Sep 25, 2020

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

@elharo elharo Sep 25, 2020

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

@vchudnov-g
Copy link

@vchudnov-g vchudnov-g commented Sep 25, 2020

I'll continue reviewing this tomorrow, but one quick note: I suggest we make it practice to not merge from upstream once a review has started until all approvals are in, simply because it makes it a bit harder to see what has changed since the last time we reviewed that's still relevant to this PR. Then we merge, and if any additional changes are required, we can have a quick incremental review of the merge.

And mea culpa, if I were quicker on the review this would less of an issue ;-)

Copy link

@vchudnov-g vchudnov-g left a comment

Please do add the TODO I comment on below and address the other comments below. Aside from that, lgtm.

}

@Override
public String serialize(ResponseT response) {

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?

import java.util.Map;

/**
* This class is responsible for serializing/deserializing protobuf {@link Message} to/from a proper

Choose a reason for hiding this comment

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

May I suggest a rewording along these lines?:

"This class serializes/deserializes messages for REST interactions. It serializes requests protobufs into REST messages, splitting the Message into the JSON request body, URL path parameters, and query parameters. It deserializes JSON responses into response protobufs."

Copy link
Contributor Author

@vam-google vam-google Oct 2, 2020

Choose a reason for hiding this comment

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

Done

import java.util.List;
import java.util.Map;

public class ProtoRestSerializer<RequestT extends Message> {

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!

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.

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

}

public void putQueryParam(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.

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.

.build();
HttpRequest httpRequest = httpRequestRunnable.createHttpRequest();
Truth.assertThat(httpRequest.getContent()).isInstanceOf(EmptyContent.class);
String expectedUrl = ENDPOINT + "name/feline" + "?food=bird&food=mouse&size=small";

Choose a reason for hiding this comment

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

still a nit, but please do consider this ;-)

expectedFields.put("optName1", "1");
expectedFields.put("optName3", "three");

Truth.assertThat(fields).isEqualTo(expectedFields);

Choose a reason for hiding this comment

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

Ah, OK.

Copy link
Contributor

@elharo elharo left a comment

some nits, and some serious concerns about exception handling.

@@ -39,13 +39,14 @@

/* 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

@elharo elharo Sep 25, 2020

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

@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.

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

@@ -39,13 +39,14 @@

/* 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.
*/
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

@elharo elharo Sep 25, 2020

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

@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.

Corrected

implements HttpRequestFormatter<RequestT> {

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

@elharo elharo Sep 25, 2020

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

@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.

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

@miraleung miraleung Oct 7, 2020

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

@elharo elharo Oct 10, 2020

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. :-(

try {
return JsonFormat.printer().print(message);
} catch (InvalidProtocolBufferException e) {
throw new RuntimeException("Failed to serialize message to JSON", e);
Copy link
Contributor

@elharo elharo Sep 25, 2020

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.

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

@elharo elharo Sep 25, 2020

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.

@vam-google vam-google requested review from and elharo Oct 3, 2020
@@ -0,0 +1,46 @@
/*
* Copyright 2018 Google LLC
Copy link
Contributor

@elharo elharo Oct 3, 2020

Choose a reason for hiding this comment

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

2020

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.

Updated.

* interactions.
*/
@BetaApi
public class ProtoRestSerializationException extends RuntimeException {
Copy link
Contributor

@elharo elharo Oct 3, 2020

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

@elharo elharo Oct 5, 2020

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

@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.

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 /** */).

@@ -82,7 +82,7 @@ RequestT fromJson(InputStream message, Message.Builder builder) {
JsonFormat.parser().ignoringUnknownFields().merge(json, builder);
return (RequestT) builder.build();
} catch (IOException e) {
throw new RuntimeException("Failed to parse response message", e);
throw new ProtoRestSerializationException("Failed to parse response message", e);
Copy link
Contributor

@elharo elharo Oct 3, 2020

Choose a reason for hiding this comment

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

definitely checked here

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.

Please check the other comment about checked exception.

@vam-google vam-google requested a review from elharo Oct 5, 2020
@@ -39,13 +39,14 @@

/* 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

@elharo elharo Oct 5, 2020

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

@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.

Corrected


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

@elharo elharo Oct 5, 2020

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

@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.

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

@vam-google
Copy link
Contributor Author

@vam-google vam-google commented Oct 5, 2020

@elharo PTAL.
tl;dr; Let me know if it is more convenient for you to discuss the exceptions thing in person (chat/gvc). Thanks!

@vam-google vam-google requested a review from elharo Oct 6, 2020
@vam-google
Copy link
Contributor Author

@vam-google vam-google commented Oct 6, 2020

@elharo PTAL

implements HttpRequestFormatter<RequestT> {

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

@miraleung miraleung Oct 7, 2020

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

@elharo elharo left a comment

  1. Is there an issue for this?
  2. Update to master

@vam-google
Copy link
Contributor Author

@vam-google vam-google commented Oct 7, 2020

@elharo

  1. There is no github issue for this, but it is a part of a big DIREGAPIC project, and there is a pagination-specific task for tracking it in the internal bug tracking system.

  2. Merged master

@vam-google vam-google requested a review from elharo Oct 7, 2020
@vam-google
Copy link
Contributor Author

@vam-google vam-google commented Oct 8, 2020

@elharo Please take a look. I have other pending work, that depends on this PR to be pushed first, I would like to push it ASAP. Thanks!

.fromJson(new InputStreamReader(httpResponseBody), responseType);
try {
return getResponseMarshaller()
.fromJson(new InputStreamReader(httpResponseBody), responseType);
Copy link
Contributor

@elharo elharo Oct 10, 2020

Choose a reason for hiding this comment

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

Important: this needs a character set, typically detected from the HTTP headers in this case

Copy link
Contributor Author

@vam-google vam-google Oct 11, 2020

Choose a reason for hiding this comment

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

added

implements HttpRequestFormatter<RequestT> {

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

@elharo elharo Oct 10, 2020

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. :-(

*/
@SuppressWarnings("unchecked")
RequestT fromJson(InputStream message, Message.Builder builder) {
try (Reader json = new InputStreamReader(message)) {
Copy link
Contributor

@elharo elharo Oct 10, 2020

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

@vam-google vam-google Oct 10, 2020

Choose a reason for hiding this comment

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

added