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

chore(api, dal, shared): Rename API Rate Limiting types for consistency #4867

Closed
wants to merge 7 commits into from

Conversation

rifont
Copy link
Contributor

@rifont rifont commented Nov 17, 2023

What change does this PR introduce?

  • Aligns on API Rate Limit naming conventions for types and environment variable config options through creation of env-var namespace
    • removal of TypeEnum suffix -> Enum for enums for readability
    • refactor api rate limit category away from environment into rate limit typings
  • Extracts API Rate Limit service category typings into separate file for better modularity

Why was this change needed?

  • Preparation for clean PRs for the evaluate rate-limit and supporting use-cases

Other information (Screenshots)

N/A

Copy link

linear bot commented Nov 17, 2023

NV-3060 🏎️ Token Bucket Rate Limiting use-case

What?

Create a use-case that implements the Upstash Token bucket rate limiting algorithm.

Why? (Context)

We have chosen to use the token bucket rate limiting algorithm via the existing Upstash library because it aligns with our API access patterns and will allow us to quickly release a basic rate limiting capability.

Definition of Done

  • Use-case ready for integration into NestJS guard with interface execute(category: CategoryTypeEnum, organization: string): { success: boolean, limit: number, remaining: number, reset: string })

@@ -53,7 +53,7 @@ describe('Create Organization - /organizations (POST)', async () => {
const { body } = await session.testAgent.post('/v1/organizations').send(testOrganization).expect(201);
const dbOrganization = await organizationRepository.findById(body.data._id);

expect(dbOrganization?.apiServiceLevel).to.eq(ApiServiceLevelTypeEnum.FREE);
expect(dbOrganization?.apiServiceLevel).to.eq(ApiServiceLevelEnum.FREE);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the TypeEnum -> Enum suffix for better readability

@IsDefined()
@IsEnum(ApiRateLimitCategoryEnum)
apiRateLimitCategory: ApiRateLimitCategoryEnum;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a refactor from the following file (below in PR) that wasn't picked up correctly by git:
https://github.com/novuhq/novu/pull/4867/files#diff-bb13833ec972ae0f2f3cd90b6ac0c9f68be590818abe737ca3f26baf85516740

expect(result[mockRateLimitServiceLevel][mockRateLimitCategory]).to.equal(mockOverrideRateLimit);
delete process.env[envVarName]; // cleanup
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a refactor from the following file (below in PR) that wasn't picked up correctly by git:
https://github.com/novuhq/novu/pull/4867/files#diff-7e5c0068062244f3c27665cfd6f282caa1cdef7ab4eb223432c264999e091984.

It is renamed get-default-api-rate-limits -> get-api-rate-limit-service-maximum-config to better reflect the fact that a configuration is being read from the environment variables.

@@ -1,21 +1,21 @@
import { Injectable, InternalServerErrorException, Logger } from '@nestjs/common';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a rename from get-api-rate-limit -> get-api-rate-limit-maximum to better reflect the fact that a maximum allowed rate limit is being fetched for the given environment entity.

GLOBAL = 'global',
}

export type IApiRateLimits = Record<ApiRateLimitCategoryTypeEnum, number>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactoring this typing from the Environment repository due to the fact that the categories are specific to rate limiting configuration, therefore belong in the Rate Limit typings.

@@ -0,0 +1,13 @@
/**
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The files below are refactored for better modularity to prepare for the evaluate rate limit use-case supporting configuration typings that will be introduced soon.

@rifont rifont requested a review from LetItRock November 17, 2023 14:58
@rifont rifont self-assigned this Nov 23, 2023
@rifont
Copy link
Contributor Author

rifont commented Nov 24, 2023

Closing in favour of putting the same changes into #4844

@rifont rifont closed this Nov 24, 2023
@rifont rifont deleted the nv-3060-consistent-rate-limit-naming branch November 24, 2023 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant