novu icon indicating copy to clipboard operation
novu copied to clipboard

🐛 Bug Report: Email sent with large attachment are failing with status code 413

Open chavda-bhavik opened this issue 2 years ago • 13 comments

📜 Description

Novu email trigger fails when sending emails with attachments that are medium to large in size.

👟 Reproduction steps

  1. Set up an account in the Novu platform and create one Notification Template with one email template.
  2. Clone this repo and run yarn install.
  3. Update the following variable in router.js
const API_KEY = "";
const TO_EMAIL = "";
const TO_PHONE = "";
const NOTIFICATION_EVENT_ID = "test";
  1. Start the application by running yarn start.
  2. Register subscriber by calling the following route,
POST http://localhost:8080/register
  1. Trigger notification by calling the following route,
POST http://localhost:8080/send

You will see the following message

{
    "message": "Request failed with status code 413"
}

👍 Expected behavior

Email should be sent regardless of attachment size.

👎 Actual Behavior with Screenshots

Emails sent properly with small size attachments like 1.2 kb but sending (triggering) an email throws the error 413 (Request entity too large) when an email is sent with a large image whose size is 2.6 MB.

💻 Operating system

Linux

🤖 Node Version

v16.15.1

📃 Provide any additional context for the Bug.

No response

👀 Have you spent some time to check if this bug has been raised before?

  • [X] I checked and didn't find similar issue

🏢 Have you read the Contributing Guidelines?

chavda-bhavik avatar Sep 01 '22 04:09 chavda-bhavik

It's strange that the 20MB limit is already specified for v1/events/trigger routes at https://github.com/novuhq/novu/blob/main/apps/api/src/bootstrap.ts#L69-L70. Still, the image attachment is failing.

chavda-bhavik avatar Sep 01 '22 15:09 chavda-bhavik

may be provider level issue?

jainpawan21 avatar Sep 01 '22 16:09 jainpawan21

may be provider level issue?

Not sure about it, but I tested issue with Sendgrid and Amazon SES providers.

chavda-bhavik avatar Sep 02 '22 01:09 chavda-bhavik

@scopsy @davidsoderberg While digging into this issue I found the solution, we only need to alter the order of these set of commands and bring the default bodyParser configuration below the /v1/events/trigger routes configuration, image

As we are storing trigger payload information in the raw field of Log collection, it throws an error that Buffer size exceeds the limit, size issue

Any thoughts on it? I believe we should stop saving attachment information in the database.

chavda-bhavik avatar Sep 03 '22 12:09 chavda-bhavik

Great job on finding the source of the issue @chavda-bhavik! :) Maybe it will be a good idea to reduce the size of the file, or even remove it if not needed as you suggested.

djabarovgeorge avatar Sep 04 '22 10:09 djabarovgeorge

@chavda-bhavik @chavda-bhavik 100% let's remove the attachments from being store in our DB. Good catch! We can only store the metadata without the actual size! @chavda-bhavik do you want to open a PR for this?

scopsy avatar Sep 04 '22 14:09 scopsy

Yes, @scopsy I will create PR for it, Thank you.

chavda-bhavik avatar Sep 05 '22 01:09 chavda-bhavik

Hey @scopsy, @djabarovgeorge Buffer size exceed issue occurs while storing the payload in the Job collection as well. We can filter out actual files while storing in Log, but I believe we cannot stop file storing from the Job.

Here is the code in trigger-event.usecase.ts that makes an entry in the Job collection, https://github.com/novuhq/novu/blob/main/apps/api/src/app/events/usecases/trigger-event/trigger-event.usecase.ts#L85-L88

Any thoughts on how to tackle the Buffer size limit issue in the Job collection?

chavda-bhavik avatar Sep 06 '22 02:09 chavda-bhavik

@chavda-bhavik oh, I didn't even think about it. The correct solution would be not to store the contents of the file in the DB 100%. I think what we can do here, is to upload the file to S3 and fetch it from there when sending the actual email. In the end of the workflow we can schedule a removal of the file from S3 since we don't really need it. @davidsoderberg what do you think about this situation?

scopsy avatar Sep 07 '22 06:09 scopsy

@scopsy I think this sounds good 😄 lets schedule the removal a while after we sent the email so we can resend it if it is needed, when we have resend I mean...

davidsoderberg avatar Sep 07 '22 06:09 davidsoderberg

Hey @davidsoderberg @scopsy, I figured out how to store, retrieve, and delete attachments from S3 (via localstack). Currently, I'm looking into how Notification is being sent.

From https://github.com/novuhq/novu/blob/main/apps/api/src/app/events/usecases/trigger-event/trigger-event.usecase.ts#L30-L111, can you say How a notification is being added in the job queue, how it gets retrieved and being sent?

I want to do is to delete the actual file while storing it in Job and fill the attachment file from S3 while sending Notifications.

chavda-bhavik avatar Sep 11 '22 04:09 chavda-bhavik

I think you can do it in the send-message -email file and we add the jobs in workflow.queue.service file :)

@scopsy let me know what you think but maybe we should introduce a new queue for file management to be able to scale in the future...

davidsoderberg avatar Sep 11 '22 05:09 davidsoderberg

@davidsoderberg, as discussed previously, I think we can just use the initial task receiver to upload the files instead of adding another queue, because we might introduce problems with the ordering of the uploaded attachments.

I suggest just adding a function to upload it instead of a new job. @chavda-bhavik what do you think?

scopsy avatar Sep 12 '22 16:09 scopsy