refactor: a huge refactor of call handling #467

alexander-fenster · 2019-04-11T18:36:54Z

⚠️⚠️⚠️⚠️ Reviewing this PR can make you mad and also can make you want to rewrite this whole library from scratch. I warned you. ⚠️⚠️⚠️⚠️
(I actually have no idea how to review the change that is this huge)

So, we are coming closer to gax v1.0, but there is no way we can make it 1.0 when it's such a mess.

In this refactor, I did several changes that did not change anything in the logic. Well, I hope so: at least both unit tests and system tests pass!

In scope of this refactor, I made the following changes to a call pipeline, from creating an API call to the call processing and returning result:

better typings. I actually made the types make sense.
renamed some classes (the worst example of a bad naming was Canceller that is actually an ongoing API call wrapper with ability to cancel).
extracted the code that handles special call types (bundling, long running, streaming, paginated calls) to their separate folders.
made a clear structure for handling calls. Each call, when initialized, can be passed a descriptor, which can be undefined (meaning that this call is just a regular normal unary call), or one of the above listed types. Based on the descriptor passed, the instance of APICaller is created (which can be one of NormalAPICaller, BundeAPICaller, LongRunningAPICaller, PagingAPICaller, or StreamingAPICaller). All descriptors implement Descriptor, all API callers implement APICaller (surprisingly, it was not the case before).
removed comments that did not make sense (it's better to not have any comment, than a wrong comment), and added some other (better) comments!

It's fun to write this but after moving things around, adding types, and fixing inheritance logic all this code finally started to make some sense for me.

This refactor brings us closer to a possibility of documenting GAX external interface, which we will need to do when we release TypeScript microgenerator (which is coming very soon).

I did not touch anything inside the gRPC logic itself (src/grpc.ts).

codecov · 2019-04-11T18:58:35Z

Codecov Report

Merging #467 into master will decrease coverage by 1.54%.
The diff coverage is 90.47%.

@@            Coverage Diff            @@
##           master    #467      +/-   ##
=========================================
- Coverage   92.74%   91.2%   -1.55%     
=========================================
  Files          26      43      +17     
  Lines        2771    2911     +140     
  Branches       72      82      +10     
=========================================
+ Hits         2570    2655      +85     
- Misses        189     243      +54     
- Partials       12      13       +1

Impacted Files	Coverage Δ
src/parserExtras.ts	`100% <ø> (ø)`	⬆️
test/routingHeader.ts	`100% <ø> (ø)`	⬆️
src/pathTemplate.ts	`98.14% <ø> (ø)`	⬆️
src/isbrowser.ts	`100% <ø> (ø)`	⬆️
src/routingHeader.ts	`100% <ø> (ø)`	⬆️
test/grpc.ts	`93.16% <ø> (ø)`	⬆️
test/warning.ts	`100% <ø> (ø)`	⬆️
test/path_template.ts	`100% <ø> (ø)`	⬆️
src/grpc.ts	`85.29% <ø> (ø)`	⬆️
test/gax.ts	`100% <ø> (ø)`	⬆️
... and 51 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 e37ee7e...b982768. Read the comment docs.

codecov · 2019-04-11T18:58:36Z

Codecov Report

Merging #467 into master will decrease coverage by 1.54%.
The diff coverage is 90.47%.

@@            Coverage Diff            @@
##           master    #467      +/-   ##
=========================================
- Coverage   92.74%   91.2%   -1.55%     
=========================================
  Files          26      43      +17     
  Lines        2771    2911     +140     
  Branches       72      82      +10     
=========================================
+ Hits         2570    2655      +85     
- Misses        189     243      +54     
- Partials       12      13       +1

Impacted Files	Coverage Δ
src/parserExtras.ts	`100% <ø> (ø)`	⬆️
test/routingHeader.ts	`100% <ø> (ø)`	⬆️
src/pathTemplate.ts	`98.14% <ø> (ø)`	⬆️
src/isbrowser.ts	`100% <ø> (ø)`	⬆️
src/routingHeader.ts	`100% <ø> (ø)`	⬆️
test/grpc.ts	`93.16% <ø> (ø)`	⬆️
test/warning.ts	`100% <ø> (ø)`	⬆️
test/path_template.ts	`100% <ø> (ø)`	⬆️
src/grpc.ts	`85.29% <ø> (ø)`	⬆️
test/gax.ts	`100% <ø> (ø)`	⬆️
... and 51 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 80c5b27...4764daa. Read the comment docs.

JustinBeckwith

Rubber Stampy

bcoe

@alexander-fenster this is looking good to me; the only real oddity that jumped out at me was the changes to licenses, at a glance from the license text I was wondering if it moved the text from Apache to BSD accidentally (or maybe this was intentional?)

Do you think this refactor will put us in a better position for some of the exiting features you've proposed for the next major version of the GAPIC generators.

bcoe · 2019-04-12T17:31:19Z

test/utils.ts

+                 }
+               };
+             }),
+             settings, descriptor) as GaxCallPromise;


It looks like the main difference in this utility is that we now return as the type GaxCallPromise, was curious why the function call changes in a bunch of tests to replace null with {}.

This is mostly caused by typings. Since we now state we return GaxCallPromise and it states it accepts (argument: {}, callOptions?: CallOptions, callback?: APICallback), null does not really fit into callOptions. I could've made callOptions?: CallOptions|null but it's just ugly and not really needed.

bcoe · 2019-04-12T17:41:15Z

test/bundling.ts

@@ -888,13 +907,13 @@ describe('bundleable', () => {
      expect(obj[0].field1).to.deep.equal([1, 2, 3]);
    }
    const apiCall = createApiCall(spy, settings);
-    apiCall({field1: [1, 2, 3], field2: 'id'}, null)
+    apiCall({field1: [1, 2, 3], field2: 'id'}, undefined)


is this change null -> undefined typing related? I read this interesting post not too long go, advising against using null in general (wondering if this was similar motivation).

This is indeed typing related, but the underlying reasoning is simple: apiCall accepts (argument: {}, callOptions?: CallOptions, callback?: APICallback), so no options would be automatically mapped to undefined, so null is not welcome here.

In general, I agree with the post you linked about not using null at all. One place where it still exists in our code is when we call a callback: callback(null, result) vs callback(err, null). It certainly can be changed to undefined but I would consider this to be a hidden breaking change (someone might check if (err === null) in their code and it will stop working properly), so I'd probably leave those nulls there for the time being.

bcoe · 2019-04-12T17:45:25Z

system-test/system.ts

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


does this switch things from an Apache to a BSD license? wondering if this is intentional.

I just realized the whole gax project is BSD-licensed, and some files I added later are Apache-licensed. Since they were no changes from external contributors in those files (I actually checked), it should be OK to relicense them as BSD to match other files in the project (I recently saw this discussion elsewhere, that's what we were told then).

bcoe · 2019-04-12T17:45:57Z

src/streamingCalls/streaming.ts

  private _isCancelCalled: boolean;
-  stream?: Duplex&{cancel: () => void};
+  stream?: CancellableStream;


this signature is way nicer.

bcoe · 2019-04-12T17:49:37Z

src/paginationCalls/pagedApiCaller.ts

+    if (settings.pageToken) {
+      argument[this.pageDescriptor.requestPageTokenField] = settings.pageToken;
+    }
+    if (settings.pageSize) {


How does this class differ from pagedIteration.ts, at a high level? wondering if we'll be in a better position to add some of the fancy iteration we've talked about.

Each special call type (paginated response, streaming, long running, or bundled) is presented by a pair of classes: *Descriptor and *APICaller. Descriptor is passed down by the client library, and it describes the type of call (for paging, it shows which response field is a next page token), and the API caller implements the functionality. Previously, both classes (named differently) were in this one file pagedIteration.ts. I just split them in separate files, renamed similarly to other descriptor and API callers, and moved to a separate folder. When I work on a new iteration, I will just make a new descriptor and API caller for paging (separate from these existing implementations) and we can support both old and new style for the time being, until all client libraries upgrade to the new one.

bcoe · 2019-04-12T17:52:39Z

src/operationsClient.ts

 export const SERVICE_ADDRESS = 'longrunning.googleapis.com';
+const version = require('../../package.json').version;


Is this trying to iterate up to the package that has included gax-nodejs as a dependency? I wonder if this could cause problems if gax-nodejs happens to be a deeper nested dependency of a library?

No, it's taking the gax version. This file, after TypeScript compilation, will be located in node_modules/google-gax/build/src (note the build/ folder) so two levels up brings us to the node_modules/google_gax/package.json.

bcoe · 2019-04-12T17:55:41Z

src/normalCalls/retries.ts

+            now = new Date();
+            delay = Math.min(delay * delayMult, maxDelay);
+            timeout = Math.min(
+                timeout! * timeoutMult!, maxTimeout!, deadline - now.getTime());


I hadn't realized you could subtract ms from a Date object, I'd usually left timeouts as ms; this is cool.

I think in this particular case deadline is a number here. But you're right, it's possible!

> d = new Date('2019-04-12T00:00:00Z') 2019-04-12T00:00:00.000Z > d - 1000 1555027199000 > new Date(d - 1000) 2019-04-11T23:59:59.000Z

bcoe · 2019-04-12T17:57:09Z

src/longRunningCalls/longrunning.ts

@@ -58,101 +51,16 @@ export interface GetOperationCallback {
  (err?: Error|null, result?: {}, metadata?: {}, rawResponse?: Operation): void;
 }

-export class LongrunningDescriptor {


I appreciate that you're breaking some of these classes out into their own files 👍

Yeah. Huge files were one of the things that I hated most about this code. I'm trying to make it manageable :)

bcoe · 2019-04-12T17:59:58Z

src/gax.ts

@@ -259,6 +263,10 @@ export class CallSettings {
      pageToken = options.pageToken;
    }

+    if ('pageSize' in options) {


once we're happy with the pagination API surface in the "GAPIC API Changes", I'd love to introduce code samples in many of our repos demonstrating pagination best practices.

Yes. What I'm going to after this PR lands is to create a separate AwesomePaginationDescriptor and AwesomePaginationAPICaller (actual names TBD) and have it alongside the existing pagination implementation. That way we can play with new things without breaking existing stuff.

callmehiphop · 2019-04-12T18:35:03Z

src/apitypes.ts

+// parameter is defined for paginated calls and stores the next page request
+// object, the third parameter stores raw (unprocessed) response object in cases
+// when it might be useful for users.
+export type RequestType = {};


Any reason why we're not using generics here?

Not sure about generics, but I think we can use protobufjs.Type instead of {} everywhere. Let me leave this for my next typings update (that will happen soon),

To answer your specific question of why we're not using generics here, it's because I did not want to make any serious changes to types in scope of this PR which is too huge to be carefully reviewed :)

alexander-fenster · 2019-04-12T20:32:19Z

@bcoe Thanks for your review! Yes, I think this refactor will make adding new features such as new pagination much simpler (at least now I understand how the special calls are handled!) As for the license change, yes, that was intentional and affected only several files I added recently (before I realized that the rest of files are BSD and not Apache licensed).

refactor: a huge refactor

020cad1

alexander-fenster requested review from bcoe, JustinBeckwith, stephenplusplus and callmehiphop April 11, 2019 18:36

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

alexander-fenster added 3 commits April 11, 2019 11:38

one test was lost, putting it back

f7333f9

no named functions

2b54c93

fix package.json to copy .js files

b982768

alexander-fenster requested a review from jkwlui April 11, 2019 19:04

JustinBeckwith approved these changes Apr 11, 2019

View reviewed changes

bcoe approved these changes Apr 12, 2019

View reviewed changes

callmehiphop reviewed Apr 12, 2019

View reviewed changes

Merge branch 'master' into huge-refactor

4764daa

alexander-fenster merged commit 3ba689b into master Apr 12, 2019

alexander-fenster deleted the huge-refactor branch April 12, 2019 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: a huge refactor of call handling #467

refactor: a huge refactor of call handling #467

alexander-fenster commented Apr 11, 2019

codecov bot commented Apr 11, 2019

codecov bot commented Apr 11, 2019 •

edited

JustinBeckwith left a comment

bcoe left a comment

bcoe Apr 12, 2019

alexander-fenster Apr 12, 2019

bcoe Apr 12, 2019

alexander-fenster Apr 12, 2019

bcoe Apr 12, 2019

alexander-fenster Apr 12, 2019

bcoe Apr 12, 2019

bcoe Apr 12, 2019

alexander-fenster Apr 12, 2019

bcoe Apr 12, 2019

alexander-fenster Apr 12, 2019

bcoe Apr 12, 2019

alexander-fenster Apr 12, 2019

bcoe Apr 12, 2019

alexander-fenster Apr 12, 2019

bcoe Apr 12, 2019

alexander-fenster Apr 12, 2019

callmehiphop Apr 12, 2019

alexander-fenster Apr 12, 2019

alexander-fenster Apr 12, 2019

alexander-fenster commented Apr 12, 2019

		export const SERVICE_ADDRESS = 'longrunning.googleapis.com';
		const version = require('../../package.json').version;

refactor: a huge refactor of call handling #467

refactor: a huge refactor of call handling #467

Conversation

alexander-fenster commented Apr 11, 2019

codecov bot commented Apr 11, 2019

Codecov Report

codecov bot commented Apr 11, 2019 • edited

Codecov Report

JustinBeckwith left a comment

Choose a reason for hiding this comment

bcoe 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

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

alexander-fenster commented Apr 12, 2019

codecov bot commented Apr 11, 2019 •

edited