Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PB-1429]: Feat/max storage days limit #278

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

apsantiso
Copy link
Collaborator

@apsantiso apsantiso commented Mar 4, 2024

This PR enforces the limit for the storage days in the trash.

What it includes:

  1. Do not list expired trashed items in get trashed items endpoint.
  2. Add a cron job to expire files every X defined time.

Some considerations:

  • Cron jobs with multiple instances running: nestjs cron jobs are in memory, every instance is going to try to run the cron job. There are some ways to handle this, having a table in a shared db so the first instance that tries to run the job writes to the DB with the current timestamp (it sounds kind of a race condition I don't like it) or using redis.

    However, for our case, maybe it is too much trouble to handle this simple cron job that eventually is going to process a small quantity of files. It runs every few minutes, we are not going to have a lot of expired trashed files in such a small time frame.

  • Folders use deleteAt and files use updatedAt. Should we index both? Should we update "updatedAt" for both when they are trashed?

  • Why is there a raw query inside the query for listing trashed/expired items? Actually, I tried to use the ORM at its max extend without writing raw SQL (they are too troublesome to maintain 😴 and people sometimes don't check them before changing models). This was my original query:

    const files = await this.fileModel.findAll({
      attributes: ['status', 'plainName', 'updatedAt', 'fileId', 'uuid'],
      where: {
        status: 'TRASHED',
        updatedAt: {
          [Op.lt]: Sequelize.literal(
            `NOW() - "user->tier->limits"."value"::integer * INTERVAL '1 day'`,
          ),
        },
      },
      include: {
        model: UserModel,
        attributes: ['uuid', 'id', 'tierId'],
        as: 'user',
        include: [
          {
            model: TierModel,
            attributes: ['id', 'label'],
            include: [
              {
                model: Limitmodel,
                where: { label: LimitLabels.MaxTrashStorageDays },
              },
            ],
          },
        ],
      },
      limit,
    });

However, Sequelize seems to have a known bug when you try to nest models with many to many relations sometimes. Basically, the query works if I do not add the limit at the end to just get a X number of records OR if I do not ask for the limitModel while limiting the results. It creates a sort of subquery and introduces the limit = 1000 inside that subquery and it fails.

Of course, I do not want to get the entire database of expired trashed items, so that's the reason of the raw query being there.

@apsantiso apsantiso changed the title Feat/max storage days limit [PB-1429]: Feat/max storage days limit Mar 4, 2024
@apsantiso apsantiso self-assigned this Mar 4, 2024
@apsantiso apsantiso added the enhancement New feature or request label Mar 4, 2024
Copy link

sonarqubecloud bot commented Mar 4, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
42.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@apsantiso apsantiso marked this pull request as ready for review March 4, 2024 13:44
@apsantiso apsantiso requested a review from sg-gs March 4, 2024 13:44
@apsantiso apsantiso marked this pull request as draft March 5, 2024 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant