[LRO] Add ApiMessageOperationTransformer #683

andreamlin · 2019-02-22T05:57:36Z

This is the ApiMessage equivalent of ProtoOperationTransformers.

codecov · 2019-02-22T06:01:49Z

Codecov Report

Merging #683 into master will increase coverage by 0.21%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #683      +/-   ##
============================================
+ Coverage     75.32%   75.53%   +0.21%     
- Complexity     1030     1035       +5     
============================================
  Files           195      196       +1     
  Lines          4628     4656      +28     
  Branches        357      358       +1     
============================================
+ Hits           3486     3517      +31     
+ Misses          984      981       -3     
  Partials        158      158

Impacted Files	Coverage Δ	Complexity Δ
.../gax/httpjson/ApiMessageOperationTransformers.java	`100% <100%> (ø)`	`2 <2> (?)`
...om/google/api/gax/httpjson/HttpJsonStatusCode.java	`29.26% <0%> (+2.43%)`	`8% <0%> (+1%)`	⬆️
...java/com/google/api/gax/httpjson/EmptyMessage.java	`12.5% <0%> (+12.5%)`	`2% <0%> (+2%)`	⬆️

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 1f25467...a311056. Read the comment docs.

andreamlin · 2019-02-24T23:31:47Z

PTAL

vam-google · 2019-03-01T20:09:55Z

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

+      if (!operationSnapshot.getResponse().getClass().isAssignableFrom(responseTClass)) {
+        throw ApiExceptionFactory.createException(
+            new Throwable(
+                String.format(


Minor: this is inconsistent with how other error meessages are constructed (L59 uses string concatenation). Please use one style (pick any (String concatenation is technically faster, String.format is more readable)) for all of them.

i choose readability!

vam-google · 2019-03-01T20:18:07Z

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

+    /** Unwraps an OperationSnapshot and returns the contained method response message. */
+    @SuppressWarnings("unchecked")
+    public ResponseT apply(OperationSnapshot operationSnapshot) {
+      if (!operationSnapshot.getErrorCode().getCode().equals(Code.OK)) {


Same issue applies to ProtoOperationTransformers. It looks like we are interested if the call was successful or no. This treats only 200 as success, but any 2xx code can be considered success HTTP Status Codes.

Though, this is unlikely that we can get anything but 200 here for success in practical cases (maybe 201 also). Please add at least a comment about "potential need to handle other error codes". If that ever happens (other than 200 success status code returned) it may save time for those who will be fixing it.

TIL
See the new diff in HttpJsonStatusCode.java to squash all 2xx codes into the OK 200 code.

Please add a comment here about potential need to handle more than only 200.

vam-google · 2019-03-01T20:37:19Z

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

+    }
+
+    /** Unwraps an OperationSnapshot and returns the contained method response message. */
+    @SuppressWarnings("unchecked")


Please move this annotation to where it applies if possible (i assume it was line 80). Reducing the scope of warning suppression helps to prevent false suppressions (for cases, when that was actually an error).

vam-google · 2019-03-01T20:38:00Z

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

+            operationSnapshot.getErrorCode(),
+            false);
+      }
+      if (!operationSnapshot.getResponse().getClass().isAssignableFrom(responseTClass)) {


Is operationSnapshot.getResponse() returning a Java proxy class? Or in other words, why isAssignableFrom() instead of isInstance(). isAssignableFrom() is heavier.

I don't know why isInstance() doesn't work; the error says

... its response type com.google.api.gax.httpjson.EmptyMessage cannot be cast to com.google.api.gax.httpjson.EmptyMessage.

But isAssignableFrom() does work for this use case.

This is very suspicious, the error message says it cannot cast class to itself. Something is wrong here.

i know! i looked on stackoverflow and some answers say that this might happen if you have different classloaders? idk

vam-google · 2019-03-01T20:39:19Z

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

+    }
+  }
+
+  @SuppressWarnings("unchecked")


Please move the annotation to where it applies, if possible.

vam-google · 2019-03-01T20:42:31Z

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

+
+    /** Unwraps an OperationSnapshot and returns the contained operation metadata message. */
+    @Override
+    public MetadataT apply(OperationSnapshot operationSnapshot) {


It seems like this is same logic as in the response transformer (the "response type" and "metadata type" in error message string can be a parameter). Please consider putting this in helper method and reuse it (static or make transformers extend same class and put this method in the parent class).

Done; implemented in transformEntityFromOperationSnapshot()

vam-google · 2019-03-01T20:45:05Z

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

+    DONE
+  }
+
+  private static class MetadataMessage<ResponseT extends ApiMessage> implements ApiMessage {


Can these classes be replaced with Mockito mocks (we should avoid creating mocks manually where possible, because they are usually much longer and require lots of boilerplate code, which has to be updated if interface changes (here the ApiMessage)). If it is not possible to use mocks instead, please rename them to Fake* or Mock* or something like that, so they do not appear as false positive results in code search (when searching for real stuff).

Naming them Fake*. Because the test logic relies on calling class type methods like instanceof, I wasn't sure how to mock that with mockito.

andreamlin · 2019-03-01T23:57:41Z

PTAL

vam-google

Sorry for the 2xx confusion. Please revert that change.

vam-google · 2019-03-04T23:53:09Z

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


  static StatusCode.Code httpStatusToStatusCode(int httpStatus, String errorMessage) {
    String causeMessage = Strings.nullToEmpty(errorMessage).toUpperCase();
+    if (httpStatus >= 200 && httpStatus < 300) {


Please revert this. Sorry for confusion I did not mean that any 2xx status code should be treated equally (at long last they are different codes for a reason). For our particular case (LRO), most probably only 200 and 201 would fit. In general handling only 200 is much safer than treating all 2xx equally. Also we are loosing information here: all 2xx httpStatus codes are mapped into 200 (this is a potential time bomb).

andreamlin · 2019-03-05T00:01:34Z

Reverted status code change.
~~PTAL~~ I missed a comment!

vam-google

LGTM with a comment.

vam-google · 2019-03-05T00:11:32Z

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

+    /** Unwraps an OperationSnapshot and returns the contained method response message. */
+    @SuppressWarnings("unchecked")
+    public ResponseT apply(OperationSnapshot operationSnapshot) {
+      if (!operationSnapshot.getErrorCode().getCode().equals(Code.OK)) {


Please add a comment here about potential need to handle more than only 200.

andreamlin changed the title ~~Add ApiMessageOperationTransformer~~ [LRO] Add ApiMessageOperationTransformer Feb 22, 2019

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 22, 2019

andreamlin requested a review from vam-google February 24, 2019 23:31

vam-google reviewed Mar 1, 2019

View reviewed changes

andreamlin added 10 commits March 1, 2019 15:48

add diff

d9b7641

2019

67482da

string.format exception message

2ab22dd

make 200-299 codes 200

f2abc31

move SuppressWarnings to the line that triggers it

d3230f5

again move suppresswarnings

681bc98

helper method transformEntityFromOperationSnapshot

40ecd2e

renaming

d1f3bb1

formatting

a19e0dd

rebase on master

b5cec71

andreamlin force-pushed the just_apimessageoptransformers branch from c43c468 to b5cec71 Compare March 1, 2019 23:50

yoshi-automation added the 🚨 This issue needs some love. label Mar 4, 2019

vam-google reviewed Mar 4, 2019

View reviewed changes

revert status code

a53600a

revert again

ac2fd7c

vam-google approved these changes Mar 5, 2019

View reviewed changes

comment for 2xx

a311056

andreamlin merged commit d47e073 into googleapis:master Mar 5, 2019

andreamlin deleted the just_apimessageoptransformers branch March 5, 2019 00:18

andreamlin mentioned this pull request Mar 5, 2019

Release gax-java v1.42.0 #689

Merged

[LRO] Add ApiMessageOperationTransformer #683

[LRO] Add ApiMessageOperationTransformer #683

Uh oh!

Conversation

andreamlin commented Feb 22, 2019

Uh oh!

codecov bot commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

andreamlin commented Feb 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreamlin commented Mar 1, 2019

Uh oh!

vam-google left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreamlin commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vam-google left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Feb 22, 2019 •

edited

Loading

vam-google left a comment •

edited

Loading

andreamlin commented Mar 5, 2019 •

edited

Loading