Implementing FCM sendAll() API#453

hiranya911 · 2019-02-07T21:56:06Z

Implementing the following new API:

sendAll(messages: Message[], dryRun?: boolean): Promise<BatchResponse>

This implements a part of go/fcm-multicast. I've also implemented the following refactorings in order to keep the size of the messaging.ts module in check:

Moved all the type definitions and their util functions to a new messaging-types.ts module.
Moved the error handling/parsing logic to a new messaging-errors.ts module.

There's more room for refactoring here, but I would rather do that in separate PRs.

src/messaging/messaging-api-request.ts

src/messaging/messaging-errors.ts

bojeil-google

I mostly have nits and some recommendations for providing samples and splitting some code to make it easier to follow. It would make the review easier. I haven't covered the entire PR yet.

src/messaging/batch-request.ts

src/utils/api-request.ts

bojeil-google · 2019-02-27T23:50:49Z

src/utils/api-request.ts

+  constructor(resp: LowLevelResponse) {
+    this.status = resp.status;
+    this.headers = resp.headers;
+    this.multipart = resp.multipart;


should you validate it is an actually multipart response?

This class is not exported, and we control the only place where it gets instantiated. So it should be fine.

src/utils/api-request.ts

bojeil-google · 2019-02-28T01:51:33Z

src/utils/api-request.ts

-      respStream.on('data', (chunk: Buffer) => {
-        responseBuffer.push(chunk);
-      });
+      const boundary = getMultipartBoundary(res.headers);


sendRequest has become too large, is there a way we can split it to be more manageable? or create smaller functions to handle different parts of the logic?

It's not easy due to the use of new Promise() but I think we can do some improvements. However, I'd rather do that as a separate PR.

src/utils/api-request.ts

src/messaging/messaging.ts

bojeil-google · 2019-02-28T02:03:10Z

src/utils/api-request.ts

+  const headerLines: string[] = responseText.substring(0, endOfHeaderPos).split('\r\n');
+
+  const statusLine: string = headerLines[0];
+  const status: string = statusLine.trim().split(/\s/)[1];


Validate that the array has >=2 elements and before that headerLines has at least 1 element.

slice(1) returns an empty array when length is less than 2. So it's safe to iterate over it without a length check. Similarly, headerLines is the result of a split() call, which always returns an array with at least 1 element.

I only see that you are using split here and not slice:
statusLine.trim().split(/\s/)[1]

We use split and slice for headerLines. For statusLine I added a check to cover the case of a malformed header. Although chances of it happening should be negligibly small.

hiranya911 · 2019-03-04T21:32:54Z

Thanks @bojeil-google. I've responded to your comments. PTAL.

src/messaging/batch-request.ts

bojeil-google · 2019-03-11T18:33:34Z

src/utils/api-request.ts

+  const headerLines: string[] = responseText.substring(0, endOfHeaderPos).split('\r\n');
+
+  const statusLine: string = headerLines[0];
+  const status: string = statusLine.trim().split(/\s/)[1];


I only see that you are using split here and not slice:
statusLine.trim().split(/\s/)[1]

src/utils/api-request.ts

test/unit/messaging/batch-requests.spec.ts

bojeil-google · 2019-03-11T19:13:04Z

test/unit/messaging/batch-requests.spec.ts

+});
+
+function parseHttpRequest(text: string | Buffer): any {
+  const httpMessageParser = require('http-message-parser');


Suggestion: I think we should minimize depending on low usage npm modules. In this case, this has ~2k weekly downloads. We have been bitten by some dependency on an npm module that was abandoned in the past.

We only use this in a couple of test cases. I can try to replace it with something else in a future PR.

bojeil-google · 2019-03-11T19:28:03Z

test/unit/messaging/messaging.spec.ts

+      ).should.eventually.be.rejected.and.have.property('code', 'app/invalid-credential');
+    });
+
+    function checkSendResponseSuccess(response: SendResponse, messageId: string) {


nit: define helper functions on top of file.

Done. Moved to the top of the enclosing block. These helpers are scoped to a specific test case.

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

hiranya911 · 2019-03-11T20:37:29Z

Thanks @bojeil-google. I've addressed most of your comments.

* Implementing sendMulticast() API * Added integration test * Fixed a comment * Readability improvement in the copy operation

… hkj-fcm-batch

…de into hkj-fcm-batch

hiranya911 added 14 commits February 5, 2019 11:13

Initial implementation for batch send

c634e1f

Unit tests for BatchRequestClient

0ea4631

Finished sendAll() implementation

26932e1

Fixed some lint errors

e063f4f

Fixed messaging test imports

e68bf31

Adding more tests

ff24eee

Increased test coverage

d3cc374

Updated tests

2b54313

Implemented multipart parsing with dicer for performance

2f96e3f

Increased test coverage for HttpClient

e65b8d1

Added a test case for zlib

ae23539

Removed http-message-parser frm required dependencies

35c7131

Added some documentation

3c3c8d1

Updated comments

3168d0e

hiranya911 requested a review from bojeil-google February 7, 2019 21:56

hiranya911 assigned bojeil-google Feb 7, 2019

hiranya911 requested a review from chong-shao February 7, 2019 21:56

hiranya911 assigned chong-shao Feb 7, 2019

Trigger CI

b9a80af

chong-shao reviewed Feb 11, 2019

View reviewed changes

src/messaging/messaging-api-request.ts Outdated Show resolved Hide resolved

src/messaging/messaging-errors.ts Outdated Show resolved Hide resolved

Fixed some typos; Reduced batch size limit to 100

434bfe5

chong-shao approved these changes Feb 19, 2019

View reviewed changes

Merge branch 'master' into hkj-fcm-batch

80c44ba

hiranya911 unassigned chong-shao Feb 25, 2019

bojeil-google suggested changes Feb 28, 2019

View reviewed changes

More documentation and clean up

64eb6e6

bojeil-google suggested changes Mar 11, 2019

View reviewed changes

bojeil-google assigned hiranya911 and unassigned bojeil-google Mar 11, 2019

hiranya911 added 3 commits March 11, 2019 13:19

Updated docs; Other code review feedback

3addf55

Merge branch 'master' into hkj-fcm-batch

b705eb5

Handling malformed responses in parseHttpResponse()

e701d4f

hiranya911 assigned bojeil-google and unassigned hiranya911 Mar 11, 2019

hiranya911 mentioned this pull request Mar 11, 2019

Implementing the sendMulticast() API for FCM #473

Merged

bojeil-google approved these changes Mar 12, 2019

View reviewed changes

bojeil-google assigned hiranya911 and unassigned bojeil-google Mar 12, 2019

hiranya911 added 3 commits March 12, 2019 13:16

Implementing the sendMulticast() API for FCM (#473)

997ff94

* Implementing sendMulticast() API * Added integration test * Fixed a comment * Readability improvement in the copy operation

Merge branch 'master' of github.com:firebase/firebase-admin-node into…

824d5bb

… hkj-fcm-batch

Merge branch 'hkj-fcm-batch' of github.com:firebase/firebase-admin-no…

146c7d6

…de into hkj-fcm-batch

hiranya911 merged commit b5f9bf8 into master Mar 14, 2019

hiranya911 deleted the hkj-fcm-batch branch March 14, 2019 00:07

ZachOrr mentioned this pull request May 1, 2019

Add messaging send_all and send_multicast functions firebase/firebase-admin-python#283

Merged

Jeremip11 mentioned this pull request Mar 8, 2021

[Snyk] Security upgrade @google-cloud/firestore from 0.15.2 to 0.19.0 Jeremip11/firebase-admin-node#8

Open

Jeremip11 mentioned this pull request May 13, 2024

[Snyk] Security upgrade @google-cloud/firestore from 0.15.2 to 0.19.0 Jeremip11/firebase-admin-node#26

Open

Conversation

hiranya911 commented Feb 7, 2019

Uh oh!

Uh oh!

Uh oh!

bojeil-google left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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 Mar 4, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

hiranya911 commented Mar 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants