Best practices managing file uploads

[quasi eagle]

Hi there! First of all, I'd like to point out that I started my first job as a dev 4 months ago, and I was mostly (about 90%) frontend. I did some backend projects, but nothing massive. So this is me asking from a 2 month ish learning standpoint. Now:

At work, I was asked to create a file upload system that writes to disk, and then serves files. These files are stored in the computer where our nest js server is running, it works fine, and I haven't noticed failed uploads, or problems with images/uploads, which is great. I've been using the UseInterceptors decorator with FileInterceptors, and on the logic for it, I am writing to disk.

Then, on the service, I'm wrapping my db calls a try catch block since, if anything fails, I want to unlink/delete the files since I can't put the file interceptor logic in a prisma transaction. This is my code:

  @UseInterceptors(
    FileInterceptor('expenses', {
      storage: diskStorage({
        destination: (req, file, cb) => {
          const folderPath = getUploadDestination(file.fieldname); // 'expenses'
          cb(null, folderPath);
        },
        filename: (req, file, cb) => {
          const uniqueName = getFileName(file.originalname);
          cb(null, uniqueName);
        },
      }),
      limits: {
        fileSize: MAX_SIZE_IN_BYTES,
      },
      fileFilter: (req, file, cb) => {
        const accept = /image\/(jpeg|png)/;
        if (!accept.test(file.mimetype)) {
          return cb(
            new BadRequestException(
              `Invalid signature file type: ${file.mimetype}. Only images are allowed.`,
            ),
            false,
          );
        }
        cb(null, true);
      },
    }),
  )

Will post the service below due to limitations.

 async createExpense(vehicleName: string, dto, file?: Express.Multer.File) {
    try {
      const asset = await this.prisma.vehicleInfo.findUnique({
        where: {
          vehicleName,
        },
        select: {
          id: true,
        },
      });

      if (!asset) {
        throw new NotFoundException(ERROR_MESSAGES.ASSET_NOT_FOUND);
      }

      const vendor = dto.vendorValue
        ? await this.prisma.vendor.upsert({
            where: { value: dto.vendorValue },
            update: {}, // if the record exists, we dont do anything, it avoids the let number variable definition at the start
            create: { value: dto.vendorValue },
          })
        : null;

      const vendorId = vendor?.id ?? null;

      const created = await this.prisma.expense.create({
        data: {
          price: new Prisma.Decimal(dto.price),
          expenseType: dto.expenseType,
          vehicleInfoId: asset.id,
          description: dto.description || null,
          vendorId,
        },
        include: { vendor: true },
      });

      if (file) {
        await this.prisma.attachment.create({
          data: {
            filename: file.originalname,
            relativePath: file.path,
            uploadedAt: new Date(),
            type: 'expenses',
            expenseId: created.id,
            assetId: asset.id,
          },
        });
      }

      return { success: true, data: created };
    } catch (error) {
      if (file) {
        await cleanupUploadedFiles(file);
      }
      if (error instanceof HttpException) {
        throw error;
      }

      throw new InternalServerErrorException(
        'Something went wrong, please contact an admin.',
      );
    }
  }

The reason why I'm throwing an error below of httpexception is because as far as I understand, if the query fails, it will go to the catch block directly and override the throw of the function inside the (!asset) check.

Please keep in mind I'm basically the backend dev because we have no one else and while it works, I'm terrified that I'm doing a terrible job, my main concern if this is something that will break in a week, or not... (I was also going to wrap both create calls in a prisma transaction so that it doesn't even call the function just in case, but not sure? Anyway, sorry for the long read 😅

(Ah yes, the dto is missing a type, I am still working on this feature, I just missed that, but it's validated with class validators, for what it's worth and it works accordingly)

[native surge]

Hi @quasi eagle , not a prisma user.
You should wrap your db calls in a transaction, otherwise you might end up with corrupted data, expense was created, but attachment failed.
If saving files is your business requirement, it should live where the rest of business logic is, not in some random place.
Calling a function in a catch, might not be the best thing to do, when the function can throw an exception as well.
Hopefully you are backing up your instance often and monitoring your drive usage.

[quasi eagle]

Hi there! No worries, it's mostly related to architecture rather than prisma itself 😅

Right, the transaction is missing, this is some refactor that was not using it, the idea is to wrap it in a transaction yes.

You are right, to me it felt messy writing on the file interceptor itself rather than on the service,, it's just the example I found and understood. But thank you! I'll see if I can find a way to move it properly to the service.

As for the instance, we have a database person that is monitoring this, that is basically his only job and we have daily checkins as far as I know. Our app is not that big for what it's worth, or has too much traffic

[native surge]

Are your files also uploaded to the db? If not how would you horizontally scale?

[quasi eagle]

Our files are uploaded to the computer where we have them stored, and I store a reference basically to where the file is located in a relativePath column on the db, basically

Then they get served by checking that url

Think of it like a column called "relative path" which will be /uploads/expenses/nameofreceipt.jpg

[native surge]

What’s your plan on when something really bad happens to that computer? Do you have backups?
What if you need to run another instance of your app? It will have access only to local files.

Is storage computer the same computer where your Nest app runs?

[quasi eagle]

We do have backups in place as far as I'm aware, unfortunately I don't have accesses to that 😅 that is the information they gave me on it.

We will not be running another instance, as far I've been told as well, I asked previously.

Yes, that is the same computer. We have about 20 users on a daily basis since it's targeted to a local company, basically. It's a management app. I checked and about a week of usage was 10MB of files.

[native surge]

Ok, looks like you have an infra team to manage all of that, and you won’t be the one to blame in case of disaster 😅

[quasi eagle]

Yeah that has been set up thankfully (my boss basically owns an infra company and they have that set up, it's mostly the dev part they dont know how), we actually had a db issue where 'transaction logs' weren't cleaned up, which blocked a few of my transactions and I wasn't blamed, so I should be good haha

My other option would be to move this file creation logic outside or after the transaction, and call it then. But then I'd have the issue of well, what happens if the transaction succeeds but the writing to disk doesn't? (I really don't know if this happens often even, it's my first time dealing with file system shenanigans)

[native surge]

In your setup ( if all permissions are configured correctly and there is enough space) likelihood is low, but it will happen eventually.
You can look at https://papooch.github.io/nestjs-cls/plugins/available-plugins/transactional
otherwise you can write file to the drive as the last operation within your transaction. If write fails, db will rollback transaction.

[quasi eagle]

Thank you very much for the response! You are right, I was really overthinking this... it's a lot simpler (and probably hacky) to write as the last step of the succesful transaction, rather than assuming the db calls will work fine.

I'll take a look a the package 🙏 and I appreciate the time you took 🙂 really, it's kinda hard not having a senior to ask questions, so most of these things can go over my head 😦

[native surge]

It does feel dirty 🙂

[quasi eagle]

Yeah, I think so too 😅

I'm glad I asked honestly, I don't like making hacky workarounds as I'm too paranoid for my liking, again, really appreciated!

[native surge]

Some kind of distributed transaction mechanism is probably a right way to handle this to ensure consistency, and to not end up with orphan records in db or files on the drive

[quasi eagle]

Hm, could you elaborate on that a bit? Or is there something I can read regarding it? 🤔

[native surge]

Take a look at the link above, looks like it provides exactly what you are looking for.