feat(wip): initial bulk subscriber create #3938

ainouzgali · 2023-08-08T14:22:13Z

What change does this PR introduce?

For larger organizations allow creating subscribers in bulk.
Introduce a subscriber bulk API to create subscriber in batches of up to 500 entries.

Why was this change needed?

Other information (Screenshots)

linear · 2023-08-08T14:22:18Z

NV-2191 📡 Bulk subscriber creation over the API

Why?

For larger organizations allow creating subscribers in bulk

Definition of Done

Introduce a subscriber bulk API to create subscriber in batches of up to 500 entries.
Implement this functionality in the novu/node package

ainouzgali · 2023-08-08T14:23:39Z

libs/dal/src/repositories/base-repository.ts

@@ -163,8 +163,8 @@ export class BaseRepository<T_DBModel, T_MappedEntity, T_Enforcement = object> {
    return await Promise.all(promises);
  }

-  async bulkWrite(bulkOperations: any) {
-    await this.MongooseModel.bulkWrite(bulkOperations);
+  async bulkWrite(bulkOperations: any, ordered = false): Promise<any> {


Mongoose doesn't export the Bulk types. Not sure how we can still type this

Let's check if it is a version thing and we need to upgrade the Mongoose version. Found some comments mentioning that with the same problem.

I see that they don't fix that also in the latest :( super weird...

libs/dal/src/repositories/subscriber/subscriber.repository.ts

ainouzgali · 2023-08-08T14:26:23Z

libs/dal/src/repositories/subscriber/subscriber.repository.ts

+      if (!e.writeErrors) {
+        throw new DalException(e.message);
+      }


If the error is not from write errors related to the bulk execution, throw the error.
Otherwise, we want to parse that into the response and not throw error

libs/dal/src/repositories/subscriber/subscriber.repository.ts

…er-creation-over-the-api

p-fernandez · 2023-08-08T14:48:29Z

apps/api/src/app/subscribers/e2e/bulk-create-subscribers.e2e.ts

+  it('should throw an error when sending more than 500 subscribers', async function () {
+    const payload = {


I am missing a test where we show how we handle if any of the subscribers fail in the creation/update and its response. Or if the user is not providing enough information to do the creation/update.

I can't think of a way to fake a failed update during the write executions... If the user is not providing enough information, it will be caught by the nested validator

Ok, so assuming the nested validator is tested already to solve the other failure case we could provide proper uploads and fake the failure in the DAL service, for example, with Sinon. So in:

bulkResponse = await this.bulkWrite(bulkWriteOps);

we would need to fake the response so for example from 3 subscribers, 2 go well and 1 fails. Therefore we can test the rest of the logic for the wrong case.
The approach would be similar to what is done in the Node.js SDK tests.

Anyway, if you need to move on other tasks let's not spend too much time on it. 👍🏻

apps/api/src/app/subscribers/e2e/bulk-create-subscribers.e2e.ts

p-fernandez · 2023-08-08T15:17:39Z

apps/api/src/app/subscribers/subscribers.controller.ts

+  @Post('/bulk')
+  @ExternalApiAccessible()
+  @UseGuards(JwtAuthGuard)
+  @ApiNoContentResponse()


I don't think this is an endpoint that would benefit of a 204 response.
From the cases we discussed will probably be a situation where:

We return a 200 if all the subscribers are created or updated.

We could return a 207 if we have created and failed (though it is not a very common status code as it was oriented to WebDAV protocol: https://www.rfc-editor.org/rfc/rfc4918#section-13).

Of course 400 validation responses.

There is a last twist of this that is to provide the status code of the success of the operation per subscriber in the response while returning a 200. Second code snippet: https://evertpot.com/http/207-multi-status

p-fernandez · 2023-08-08T15:19:58Z

libs/dal/src/repositories/base-repository.ts

@@ -163,8 +163,8 @@ export class BaseRepository<T_DBModel, T_MappedEntity, T_Enforcement = object> {
    return await Promise.all(promises);
  }

-  async bulkWrite(bulkOperations: any) {
-    await this.MongooseModel.bulkWrite(bulkOperations);
+  async bulkWrite(bulkOperations: any, ordered = false): Promise<any> {


Let's check if it is a version thing and we need to upgrade the Mongoose version. Found some comments mentioning that with the same problem.

libs/dal/src/repositories/subscriber/subscriber.repository.ts

p-fernandez · 2023-08-08T15:27:57Z

libs/dal/src/repositories/subscriber/subscriber.repository.ts

+    const insertedSubscribers = created.map((inserted) => {
+      indexes.push(inserted.index);
+
+      return { subscriberId: subscribers[inserted.index]?.subscriberId };
+    });
+
+    let failed = [];
+    if (writeErrors.length > 0) {
+      failed = writeErrors.map((error) => {
+        indexes.push(error.err.index);
+
+        return {
+          message: error.err.errmsg,
+          subscriberId: error.err.op?.subscriberId,
+        };
+      });
+    }
+
+    const updatedSubscribers = subscribers
+      .filter((subId, index) => !indexes.includes(index))
+      .map((subscriber) => {


When we have a bit more clear the direction here let's clean up the different pieces in separate functions so we can isolate the actions for the different type of affected subscribers so we improve the readability of the code.
We can have a dedicated function for the created, other for the updated and other for the failed.

libs/dal/src/repositories/subscriber/subscriber.repository.ts

LetItRock · 2023-08-11T12:23:04Z

libs/dal/src/repositories/subscriber/subscriber.repository.ts

+      bulkResponse = e.result;
+    }
+
+    const { upserted: created, writeErrors } = bulkResponse.result;


The result property is deprecated, it's recommended to use:

/** Returns an array of all inserted ids */ getInsertedIds(): Document[]; /** Returns an array of all upserted ids */ getUpsertedIds(): Document[]; /** Retrieve all write errors */ getWriteErrors(): WriteError[];

and it will simplify the logic a little ;)

LetItRock · 2023-08-11T13:13:40Z

libs/dal/src/repositories/base-repository.ts

@@ -163,8 +163,8 @@ export class BaseRepository<T_DBModel, T_MappedEntity, T_Enforcement = object> {
    return await Promise.all(promises);
  }

-  async bulkWrite(bulkOperations: any) {
-    await this.MongooseModel.bulkWrite(bulkOperations);
+  async bulkWrite(bulkOperations: any, ordered = false): Promise<any> {


I see that they don't fix that also in the latest :( super weird...

…er-creation-over-the-api

p-fernandez · 2023-08-14T09:33:27Z

apps/api/src/app/events/usecases/map-trigger-recipients/map-trigger-recipients.use-case.ts

@@ -27,7 +27,7 @@ interface ILogTopicSubscribersPayload {
 }

 const isNotTopic = (recipient: TriggerRecipientSubscriber): recipient is TriggerRecipientSubscriber =>
-  typeof recipient === 'string' || recipient?.type !== TriggerRecipientsTypeEnum.TOPIC;
+  typeof recipient === 'string' || !recipient.hasOwnProperty('type');


What's the reason behind changing this? We might not be using the type for a different situation of a Topic, but we wanted to leave that way open for future solutions. I can totally see us allowing to trigger an event to all the subscribers of a TenantId (becoming TriggerRecipientsTypeEnum.TENANT) as related to the work we are doing on that side.

@p-fernandez Because I removed the unused 'record' fields from subscriber type (it was never used or saved anywhere), so in the context of this function - which subscriber is of type TriggerRecipientSubscriber - type script no longer accepts that there could be type property for subscriber. It wasn't exactly right even before, as it accepted it by 'mistake'

We could refactor the function to be of different type/ different check when we need to and get to a specific case. Wdyt? makes sense?

I can't totally understand the reason. isNotTopic is being used here:

novu/apps/api/src/app/events/usecases/map-trigger-recipients/map-trigger-recipients.use-case.ts

Line 132 in b8cb9da

private findSubscribers(recipients: TriggerRecipients): ISubscribersDefine[] {

If we do this change if we introduce new types of recipients as I mentioned or if someone is using this construction for subscribers (as it is available here:

novu/libs/shared/src/types/events/index.ts

Line 41 in b8cb9da

SUBSCRIBER = 'Subscriber',

) it will fail the mapping of the users.
If I am still missing the point you are making let's review this in private to not clutter this PR. 👍🏻

p-fernandez

🌟
Brilliant work. The endpoint is nicely implemented and the API response is well thought.
Just one question regarding one change that I am not sure about it.

feat(wip): initial bulk subscriber create

d851735

ainouzgali marked this pull request as draft August 8, 2023 14:22

github-actions bot added @novu/api @novu/dal labels Aug 8, 2023

ainouzgali commented Aug 8, 2023

View reviewed changes

ainouzgali added 2 commits August 8, 2023 17:29

Merge remote-tracking branch 'origin/next' into nv-2191-bulk-subscrib…

db98784

…er-creation-over-the-api

fix: cspell error

c2833e0

p-fernandez reviewed Aug 8, 2023

View reviewed changes

feat: add types

9540bf0

github-actions bot added the @novu/shared label Aug 10, 2023

ainouzgali added 3 commits August 10, 2023 13:28

feat: non existent id field

4d90606

refactor: minor naming changes

ee40561

Merge branch 'next' into nv-2191-bulk-subscriber-creation-over-the-api

a4e3396

ainouzgali marked this pull request as ready for review August 10, 2023 11:13

ainouzgali self-assigned this Aug 10, 2023

ainouzgali requested a review from p-fernandez August 10, 2023 11:39

LetItRock reviewed Aug 11, 2023

View reviewed changes

ainouzgali added 2 commits August 13, 2023 11:05

Merge remote-tracking branch 'origin/next' into nv-2191-bulk-subscrib…

0037df9

…er-creation-over-the-api

refactor: use get functions instead of result field of bulk

60635bf

ainouzgali requested a review from LetItRock August 14, 2023 07:35

p-fernandez reviewed Aug 14, 2023

View reviewed changes

p-fernandez approved these changes Aug 14, 2023

View reviewed changes

ainouzgali added 3 commits August 15, 2023 13:32

refactor: better filter for not topic

f3c9d45

refactor: better filter for not topic

bc78075

Merge branch 'next' into nv-2191-bulk-subscriber-creation-over-the-api

986152e

ainouzgali added this pull request to the merge queue Aug 15, 2023

Merged via the queue into next with commit 84f8c4b Aug 15, 2023

ainouzgali deleted the nv-2191-bulk-subscriber-creation-over-the-api branch August 15, 2023 12:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(wip): initial bulk subscriber create #3938

feat(wip): initial bulk subscriber create #3938

ainouzgali commented Aug 8, 2023

linear bot commented Aug 8, 2023

Why?

Definition of Done

ainouzgali Aug 8, 2023

p-fernandez Aug 8, 2023

LetItRock Aug 11, 2023

ainouzgali Aug 8, 2023

p-fernandez Aug 8, 2023

ainouzgali Aug 10, 2023

p-fernandez Aug 14, 2023 •

edited

Loading

p-fernandez Aug 8, 2023

p-fernandez Aug 8, 2023

p-fernandez Aug 8, 2023

LetItRock Aug 11, 2023

LetItRock Aug 11, 2023

p-fernandez Aug 14, 2023

ainouzgali Aug 14, 2023

p-fernandez Aug 14, 2023

p-fernandez left a comment

		it('should throw an error when sending more than 500 subscribers', async function () {
		const payload = {

feat(wip): initial bulk subscriber create #3938

feat(wip): initial bulk subscriber create #3938

Conversation

ainouzgali commented Aug 8, 2023

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

linear bot commented Aug 8, 2023

Why?

Definition of Done

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

p-fernandez Aug 14, 2023 • 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

p-fernandez left a comment

Choose a reason for hiding this comment

p-fernandez Aug 14, 2023 •

edited

Loading