-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(dal, shared, api): Add rate limit DAL attributes #4758
Merged
Merged
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
748b194
feat(dal, shared, api): Add DAL fields for rate limiting
rifont 4b47cce
test(api): Add tests for rate limit fields
rifont 75febc1
test(api): Use enum for apiServiceLevel test assertion
rifont 1e06a6d
Merge branch 'next' into nv-3058-rate-limiting-dtos
rifont 15035d0
fix(dal): Use api prefix for rate limits to differentiate from other …
rifont 81ef5b7
fix(dal): Update category enum to also include api prefix
rifont d9d86d9
fix(dal): Make apiRateLimits subdocument optional
rifont 975df32
fix(dal): Add fallback unlimited tier
rifont File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 changes: 4 additions & 0 deletions
4
libs/dal/src/repositories/organization/organization.entity.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 2 additions & 0 deletions
2
libs/shared/src/entities/organization/organization.interface.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,9 @@ | ||
export type EnvironmentId = string; | ||
|
||
export enum ApiRateLimitCategoryTypeEnum { | ||
TRIGGER = 'trigger', | ||
CONFIGURATION = 'configuration', | ||
GLOBAL = 'global', | ||
} | ||
|
||
export type IApiRateLimits = Record<ApiRateLimitCategoryTypeEnum, number>; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,6 @@ | ||
export type OrganizationId = string; | ||
|
||
export enum ApiServiceLevelTypeEnum { | ||
FREE = 'free', | ||
BUSINESS = 'business', | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rifont should we exclude this entity from returning using the Exclude decorator here? So it won't be returned by default when querying for his org? To me it's more of an internal property. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that
apiServiceLevel
should be excluded from the API response for/organizations
due to it being an internal property. Though, I'm not convinced it should be excluded at the database layer - this creates extra work in the use-case layer, complicates typings, and makes debugging for missing properties less transparent and difficult.I experimented with the
ApiHideProperty
decorator from @nestjs/swagger, however, this only removes the attribute from the swagger doc.Longer term, we probably want a way to exclude certain properties from an API response, I see 2 solutions here:
I'll go with the DTO layer exclude approach in absence of the API layer exclusion for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scopsy just thinking on this some more. On the other hand, it could be useful to customers building platforms on top of Novu to have access to the
apiServiceLevel
, and theapiRateLimits
property, to display these in Admin dashboards to aid in debugging/supporting the service they are providing to their own customers via their platform.This B2B2B2C mindset for Novu as a notification infrastructure will provide the best flexibility and transparency for customers to pick and choose how they provide access to the underlying Novu platform resources with features such as rate limiting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the Swagger doc should be seen as the source of truth for API integrations, we can technically merge and proceed with this implementation and modify the behavior of this property prior to release without being a breaking change.
I'll merge this to keep proceedings with the rate-limiting implementation, but let's discuss this further.