feat: REST Gapic (REGAPIC) Support #1177

vam-google · 2020-09-08T22:05:31Z

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

codecov · 2020-09-08T22:17:40Z

Codecov Report

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

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

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpRequestRunnable.java

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoMessageRequestFormatter.java

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoMessageResponseParser.java

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

miraleung · 2020-09-08T23:35:35Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

+    }
+  }
+
+  public void toPathParam(Map<String, String> fields, String fieldName, Object fieldValue) {


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.

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.

gax-httpjson/src/test/java/com/google/api/gax/httpjson/ProtoMessageRequestFormatterTest.java

gax-httpjson/src/test/java/com/google/api/gax/httpjson/ProtoMessageResponseParserTest.java

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

gax-httpjson/src/main/java/com/google/api/gax/httpjson/FieldsExtractor.java

vam-google · 2020-09-12T06:35:04Z

@miraleung PTAL

vchudnov-g

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!

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoMessageRequestFormatter.java

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoMessageResponseParser.java

vchudnov-g · 2020-09-16T19:41:15Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoMessageResponseParser.java

+  }
+
+  @Override
+  public String serialize(ResponseT response) {


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)

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.

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?

vchudnov-g · 2020-09-16T19:44:44Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

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


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

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

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

vchudnov-g · 2020-09-16T20:30:37Z

gax-httpjson/src/test/java/com/google/api/gax/httpjson/ProtoRestSerializerTest.java

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


vchudnov-g · 2020-09-16T20:32:51Z

gax-httpjson/src/test/java/com/google/api/gax/httpjson/ProtoRestSerializerTest.java

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


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

Ditto on assertThrows.

vchudnov-g · 2020-09-16T20:35:45Z

gax-httpjson/src/test/java/com/google/api/gax/httpjson/ProtoRestSerializerTest.java

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


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")

vchudnov-g · 2020-09-16T20:37:28Z

gax-httpjson/src/test/java/com/google/api/gax/httpjson/ProtoMessageResponseParserTest.java

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


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

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.

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

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.

vchudnov-g · 2020-09-16T20:39:13Z

gax-httpjson/src/test/java/com/google/api/gax/httpjson/ProtoRestSerializerTest.java

+    expectedFields.put("optName1", "1");
+    expectedFields.put("optName3", "three");
+
+    Truth.assertThat(fields).isEqualTo(expectedFields);


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

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.

miraleung

LGTM with nits addressed.

miraleung · 2020-09-21T21:21:03Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

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


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

miraleung · 2020-09-21T21:23:40Z

gax-httpjson/src/test/java/com/google/api/gax/httpjson/ProtoMessageResponseParserTest.java

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


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

miraleung · 2020-09-21T21:24:09Z

gax-httpjson/src/test/java/com/google/api/gax/httpjson/ProtoRestSerializerTest.java

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


Ditto on assertThrows.

vam-google · 2020-09-23T07:58:20Z

@miraleung @vchudnov-g PTAL

miraleung · 2020-09-25T00:16:44Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

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


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

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

vchudnov-g · 2020-09-25T00:25:38Z

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 ;-)

vchudnov-g

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

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoMessageRequestFormatter.java

vchudnov-g · 2020-09-25T00:31:45Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoMessageResponseParser.java

+  }
+
+  @Override
+  public String serialize(ResponseT response) {


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?

vchudnov-g · 2020-09-25T00:36:14Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

+import java.util.Map;
+
+/**
+ * This class is responsible for serializing/deserializing protobuf {@link Message} to/from a proper


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

vchudnov-g · 2020-09-25T00:37:56Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

+import java.util.List;
+import java.util.Map;
+
+public class ProtoRestSerializer<RequestT extends Message> {


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

vchudnov-g · 2020-09-25T00:39:41Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

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


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

vchudnov-g · 2020-09-25T00:46:21Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

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


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.

vchudnov-g · 2020-09-25T00:47:37Z

gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpRequestRunnableTest.java

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


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

vchudnov-g · 2020-09-25T00:49:18Z

gax-httpjson/src/test/java/com/google/api/gax/httpjson/ProtoRestSerializerTest.java

+    expectedFields.put("optName1", "1");
+    expectedFields.put("optName3", "three");
+
+    Truth.assertThat(fields).isEqualTo(expectedFields);


elharo

some nits, and some serious concerns about exception handling.

elharo · 2020-09-25T12:54:11Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpResponseParser.java

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


nit: no period per Oracle guidelines

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

elharo · 2020-09-25T12:54:23Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpResponseParser.java

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


elharo · 2020-09-25T12:55:54Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoMessageRequestFormatter.java

+    implements HttpRequestFormatter<RequestT> {
+
+  private final FieldsExtractor<RequestT, String> requestBodyExtractor;
+  private final FieldsExtractor<RequestT, Map<String, List<String>>> queryParamsExtractor;


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.

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.

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

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

elharo · 2020-09-25T12:58:37Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

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


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.

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.

elharo · 2020-09-25T12:58:58Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

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


same story. Please, never do this.

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.

elharo · 2020-10-03T13:00:18Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializationException.java

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


elharo · 2020-10-03T13:01:01Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializationException.java

+ * interactions.
+ */
+@BetaApi
+public class ProtoRestSerializationException extends RuntimeException {


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

Unfortunately I can't make it checked:

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.

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.

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.

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.

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.

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.

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?

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

elharo · 2020-10-03T13:08:21Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

@@ -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);


definitely checked here

Please check the other comment about checked exception.

elharo · 2020-10-05T21:32:07Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpResponseParser.java

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


HTTP response

elharo · 2020-10-05T21:33:50Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoMessageRequestFormatter.java

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


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

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 · 2020-10-05T21:37:32Z

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

…estSerializationException

vam-google · 2020-10-06T23:34:31Z

@elharo PTAL

miraleung · 2020-10-07T00:52:56Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoMessageRequestFormatter.java

+    implements HttpRequestFormatter<RequestT> {
+
+  private final FieldsExtractor<RequestT, String> requestBodyExtractor;
+  private final FieldsExtractor<RequestT, Map<String, List<String>>> queryParamsExtractor;


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

elharo

Is there an issue for this?
Update to master

vam-google · 2020-10-07T18:37:39Z

@elharo

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.
Merged master

vam-google · 2020-10-08T18:06:18Z

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

elharo · 2020-10-10T11:34:31Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ApiMessageHttpResponseParser.java

-          .fromJson(new InputStreamReader(httpResponseBody), responseType);
+      try {
+        return getResponseMarshaller()
+            .fromJson(new InputStreamReader(httpResponseBody), responseType);


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

elharo · 2020-10-10T11:36:43Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoMessageRequestFormatter.java

+    implements HttpRequestFormatter<RequestT> {
+
+  private final FieldsExtractor<RequestT, String> requestBodyExtractor;
+  private final FieldsExtractor<RequestT, Map<String, List<String>>> queryParamsExtractor;


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

elharo · 2020-10-10T11:38:04Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

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


requires character set

elharo · 2020-10-10T11:39:35Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ProtoRestSerializer.java

+  }
+
+  private boolean isDefaultValue(String fieldName, Object fieldValue) {
+    // TODO: Revisit this approach to ensure proper default-value handling as per design.


any reason not to do this now?

The final version most probably will be depending on optional fields in proto itself (i.e. the optiona fields from proto2 ported to proto3 requested by Ads and implemented in the recent version of protoc, governed by --experimental_allow_proto3_optional flag to protoc). This is not used yet in our generation pipeline, that is why not used here.

elharo · 2020-10-10T11:40:10Z

gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpRequestRunnableTest.java

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


Agreed. Please do this.

elharo · 2020-10-11T11:56:26Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ApiMessageHttpResponseParser.java

@@ -86,13 +89,18 @@ public ResponseT read(JsonReader in) {
  }

  @Override
-  public ResponseT parse(InputStream httpResponseBody) {
+  public ResponseT parse(InputStream httpResponseBody, Charset httpResponseBodyCharset) {


Sorry. Didn't realize this was an existing class. I's wrong as currently implemented but we can't change the signature so easily.

elharo · 2020-10-11T11:57:27Z

gax-httpjson/src/main/java/com/google/api/gax/httpjson/ApiMessageHttpResponseParser.java

+      try {
+        return getResponseMarshaller()
+            .fromJson(
+                new InputStreamReader(httpResponseBody, httpResponseBodyCharset), responseType);


specify this as UTF-8

🤖 I have created a release \*beep\* \*boop\* --- ## [1.60.0](https://github.com/googleapis/gax-java/compare/v1.59.1...v1.60.0) (2020-10-19) ### Features * REST Gapic (REGAPIC) Support ([#1177](https://github.com/googleapis/gax-java/issues/1177)) ([12b18ee](https://github.com/googleapis/gax-java/commit/12b18ee255d3fabe13bb3969df40753b29f830d5)) ### Bug Fixes * prevent npe caused by missing parentheses ([#1198](https://github.com/googleapis/gax-java/issues/1198)) ([b856351](https://github.com/googleapis/gax-java/commit/b85635123f987f9808086f14a58dd8c7418a3bd8)) ### Dependencies * upgrade grpc to 1.32.2 ([#1212](https://github.com/googleapis/gax-java/issues/1212)) ([03c4c0f](https://github.com/googleapis/gax-java/commit/03c4c0f621f439c30752122568d2a9a7703e5e16)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).

vam-google added 5 commits August 8, 2020 16:45

feat: Implement rest-gapic http transport

85bb35d

fix: fix bazel build

d6c2829

chore: Add tests to the httpjson protobuf-specific code

55b9f21

chore: Add tests to the httpjson protobuf-specific code

3c4c19d

Merge remote-tracking branch 'upstream/master' into rest-gapic

aee533a

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 8, 2020

vam-google requested review from miraleung and vchudnov-g September 8, 2020 22:05

fix license header

e02e368

miraleung suggested changes Sep 8, 2020

View reviewed changes

vam-google mentioned this pull request Sep 10, 2020

feat: REST GAPIC (REGAPIC) Support for Java googleapis/gapic-generator#3275

Merged

Address PR feedback

7c08c24

vchudnov-g suggested changes Sep 16, 2020

View reviewed changes

Address PR feedback, add javadoc

d264bc6

vam-google requested review from miraleung and vchudnov-g September 21, 2020 17:18

miraleung suggested changes Sep 21, 2020

View reviewed changes

Address PR feedback

3a982ff

vam-google requested a review from a team as a code owner September 22, 2020 21:06

vam-google added 2 commits September 22, 2020 14:08

Merge remote-tracking branch 'upstream/master' into rest-gapic

a71e5d8

merge upsream/master

07b20ba

vam-google requested a review from miraleung September 23, 2020 07:58

miraleung approved these changes Sep 25, 2020

View reviewed changes

vchudnov-g approved these changes Sep 25, 2020

View reviewed changes

elharo suggested changes Sep 25, 2020

View reviewed changes

Address PR feedback

7f72f2b

vam-google requested review from a team and elharo October 3, 2020 04:34

Merge remote-tracking branch 'upstream/master' into rest-gapic

0c0328b

elharo reviewed Oct 3, 2020

View reviewed changes

Update license year

5f257e6

vam-google requested a review from elharo October 5, 2020 21:35

elharo reviewed Oct 5, 2020

View reviewed changes

vam-google added 2 commits October 5, 2020 17:41

correct documentation typo

71474be

Add thrown exceptions into documentation, wrap gson exceptions into R…

b231f0f

…estSerializationException

vam-google requested a review from elharo October 6, 2020 01:37

miraleung approved these changes Oct 7, 2020

View reviewed changes

elharo reviewed Oct 7, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into rest-gapic

d3b13f9

vam-google requested a review from elharo October 7, 2020 19:06

Added a comment about triple nested generics

79c41a1

elharo reviewed Oct 10, 2020

View reviewed changes

Add character set (breaking change)

0dc2d89

vam-google requested a review from elharo October 10, 2020 23:38

elharo suggested changes Oct 11, 2020

View reviewed changes

Rollback HttpResponseParser.parse() surface change

169fcf2

vam-google requested a review from elharo October 12, 2020 05:25

elharo approved these changes Oct 12, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into rest-gapic

2619f63

vam-google merged commit 12b18ee into googleapis:master Oct 12, 2020

feat: REST Gapic (REGAPIC) Support #1177

feat: REST Gapic (REGAPIC) Support #1177

Conversation

vam-google commented Sep 8, 2020 • edited Loading

codecov bot commented Sep 8, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google commented Sep 12, 2020

vchudnov-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miraleung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google commented Sep 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vchudnov-g commented Sep 25, 2020

vchudnov-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google Oct 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google Oct 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google commented Sep 8, 2020 •

edited

Loading

codecov bot commented Sep 8, 2020 •

edited

Loading

vam-google Sep 18, 2020 •

edited

Loading

vam-google Oct 3, 2020 •

edited

Loading

vam-google Oct 3, 2020 •

edited

Loading

vam-google Oct 5, 2020 •

edited

Loading

vam-google Oct 5, 2020 •

edited

Loading

vam-google Oct 6, 2020 •

edited

Loading