Re-implementing the HTTP Client Abstraction by hiranya911 · Pull Request #280 · firebase/firebase-admin-node

hiranya911 · 2018-05-24T23:43:41Z

The existing HTTP client abstractions (HttpRequestHandler and SignedApiRequestHandler) have a number of caveats:

Do not support HTTP endpoints (HTTPS only)
Do not expose response headers
Complex and often confusing error handling semantics (e.g. even low-level network errors are reported as errors with HTTP status codes)
Support for handling different payload types is broken (e.g. cannot handle anything other than application/json, text/html and text/plain)
Not easy to extend or test (public API takes 5 arguments already)

I ran into pretty much all the above problems while prototyping the IAM support for token minting.

To resolve these problems I propose a new set of HTTP client abstractions (HttpClient and AuthorizedHttpClient). My original plan was to use the axios library under the hood, but its support for handling timeouts needs a bit of work. So I came up with an implementation which borrows from both axios and our already existing HttpRequestHandler class. The resulting implementation:

Supports both HTTPS and HTTP
Exposes response headers alongside status and payload
Simplifies error handling -- HTTP errors are reported by throwing an HttpError which provides access to the response object. All other low-level errors result in a FirebaseAppError.
Supports any payload type
Fairly easy to test; new arguments can be added by extending the HttpRequestConfig type

This PR implements the new abstractions and provides unit tests. I will update the rest of the codebase to use them in a series of future PRs (work already underway in a separate branch).

hiranya911 · 2018-05-25T00:21:43Z

Related to #274

bojeil-google · 2018-05-31T07:18:35Z

I haven't started review yet, but why do we want to support HTTP requests? I am hoping that you are not planning to use this with unencrypted connections.

hiranya911 · 2018-05-31T16:30:28Z

I want to be able to use this to access things like the local Metadata service, which is only available over HTTP: https://github.com/firebase/firebase-admin-node/blob/master/src/auth/credential.ts#L338

bojeil-google · 2018-06-01T22:36:17Z

src/utils/api-request.ts

+  public readonly headers: any;
+  public readonly text: string;
+
+  private readonly data_: any;


private properties should not use trailing underscores (typescript style guide).

bojeil-google · 2018-06-01T22:37:15Z

src/utils/api-request.ts

+  public readonly text: string;
+
+  private readonly data_: any;
+  private readonly request_: string;


private properties should not use trailing underscores (typescript style guide).

bojeil-google · 2018-06-01T22:46:46Z

src/utils/api-request.ts

+      return this.data_;
+    }
+    try {
+      return JSON.parse(this.text);


Is this try/catch needed again? You already test this in the constructor. You may as well throw an error directly.

bojeil-google · 2018-06-01T22:48:46Z

src/utils/api-request.ts

+  url: string;
+  headers?: {[key: string]: string};
+  data?: string | object | Buffer;
+  timeout?: number;


Why is there no port number (in case a non-default one is used)? It seems like this has to be provided in the URL. Maybe document that.

bojeil-google · 2018-06-01T22:50:36Z

src/utils/api-request.ts

+  readonly data: any;
+}
+
+class DefaultHttpResponse implements HttpResponse {


Javadocs for these new structures would be nice to have.

bojeil-google · 2018-06-01T23:41:11Z

src/utils/api-request.ts

+      respStream.on('end', () => {
+        const responseData = Buffer.concat(responseBuffer).toString();
+        response.data = responseData;
+        settle(resolve, reject, response);


Can you rename to something less abstract?

Renamed to finalizeRequest

bojeil-google · 2018-06-02T00:14:41Z

src/utils/api-request.ts

+}
+
+function enhanceError(
+  error,


error: Error,

That's not possible since we add new fields to the error object in the method body (triggers TS compilation error).

bojeil-google · 2018-06-02T00:21:02Z

test/unit/utils.ts

  return 'access_token_' + _.random(999999999);
 }
+
+export function responseFrom(data: any, status: number = 200, headers: any = {}): HttpResponse {


Add javadoc. Also where is this used?

Done.

Not used anywhere at the moment. But it will be used once we start mocking interactions in other unit tests. I have separate PRs coming up for that.

bojeil-google · 2018-06-02T00:25:03Z

test/unit/utils/api-request.spec.ts

+    });
+  });
+
+  it('should be fulfilled for a 2xx response with a json payload', () => {


This looks exactly like the test above it.

bojeil-google · 2018-06-02T00:26:39Z

test/unit/utils/api-request.spec.ts

+    const reqData = {request: 'data'};
+    const respData = {success: true};
+    const scope = nock('https://' + mockHost, {
+        reqheaders: {


indent 2 spaces instead of 4

hiranya911 · 2018-06-06T21:29:39Z

Made the suggested changes. Over to @bojeil-google for another look.

bojeil-google

LGTM. Just have a bunch of javadoc nits to be consistent with the rest of the repo.

bojeil-google · 2018-06-08T02:40:04Z

test/unit/utils.ts

+/**
+ * Creates a mock HTTP response from the given data and parameters.
+ *
+ * @param data Data to be included in the response body.


@param {*} data ...

bojeil-google · 2018-06-08T02:40:28Z

test/unit/utils.ts

+ * Creates a mock HTTP response from the given data and parameters.
+ *
+ * @param data Data to be included in the response body.
+ * @param status HTTP status code (defaults to 200).


@param {number=} status...

bojeil-google · 2018-06-08T02:40:57Z

test/unit/utils.ts

+ *
+ * @param data Data to be included in the response body.
+ * @param status HTTP status code (defaults to 200).
+ * @param headers HTTP headers to be included in the ersponse.


@param {*=} headers...

bojeil-google · 2018-06-08T02:48:32Z

src/utils/api-request.ts

+    this.request = `${resp.config.method} ${resp.config.url}`;
  }

  get data(): any {


Javadoc missing

This seems to get inherited from the parent interface. At least on VSCode, hovering over the getter shows the documentation from the interface.

bojeil-google · 2018-06-08T02:48:52Z

src/utils/api-request.ts

-  response?: LowLevelResponse;
-}
-
 function sendRequest(config: HttpRequestConfig): Promise<LowLevelResponse> {


Javadoc missing.

bojeil-google · 2018-06-08T02:49:00Z

src/utils/api-request.ts

 }

-function settle(resolve, reject, response: LowLevelResponse) {
+function finalizeRequest(resolve, reject, response: LowLevelResponse) {


Javadoc missing.

Done. I'm only adding a description to these un-exported helper functions.

bojeil-google · 2018-06-08T02:49:48Z

src/utils/api-request.ts

+  private readonly parseError: Error;
+  private readonly request: string;

  constructor(resp: LowLevelResponse) {


To be consistent, we have been adding javadocs in other files. Let's add one here.

hiranya911 · 2018-06-08T18:52:26Z

Updated the documentation as recommended.

hiranya911 added 7 commits May 20, 2018 12:45

Started implementing Axios based HTTP client

33766e9

Added some docs; Further cleaned up API and tests

da938d6

Implemented retry on network error

56c8d12

Removing axios dependency

780c512

Merge branch 'master' into hkj-axios

a24d6c3

Renamed response type

9c50fa9

Minor updates

40b62ee

hiranya911 requested review from bojeil-google and jshcrowthe May 25, 2018 00:48

hiranya911 assigned bojeil-google and jshcrowthe May 25, 2018

Removing unused requires

7750e43

bojeil-google suggested changes Jun 2, 2018

View reviewed changes

Added documentation and other improvements

66532d6

bojeil-google suggested changes Jun 8, 2018

View reviewed changes

Updated documentation

4e47858

bojeil-google approved these changes Jun 8, 2018

View reviewed changes

hiranya911 assigned hiranya911 and unassigned jshcrowthe and bojeil-google Jun 8, 2018

hiranya911 merged commit 40df8cb into master Jun 8, 2018

hiranya911 deleted the hkj-axios branch June 8, 2018 21:12

hiranya911 mentioned this pull request Jun 11, 2018

Migrating the auth module to use the new HTTP Client #291

Merged

hiranya911 mentioned this pull request Jul 6, 2018

Using the new HttpClient in messaging module #301

Merged

hiranya911 mentioned this pull request Jul 25, 2018

Using the new HttpClient API in Credentials #318

Merged

This was referenced Jul 30, 2018

Using the new HttpClient API in instanceId module #321

Merged

Removing the old HttpRequestHandler APIs #324

Merged

hiranya911 mentioned this pull request Aug 21, 2018

FR: [Messaging] Add option to retry sending or expose Retry-After header #43

Closed

Conversation

hiranya911 commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hiranya911 commented May 25, 2018

Uh oh!

bojeil-google commented May 31, 2018

Uh oh!

hiranya911 commented May 31, 2018

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!

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!

hiranya911 commented Jun 6, 2018

Uh oh!

bojeil-google left a comment

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

hiranya911 commented May 24, 2018 •

edited

Loading