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

feat(dal, shared, api): Add rate limit DAL attributes #4758

Merged
merged 8 commits into from
Nov 6, 2023

Conversation

rifont
Copy link
Contributor

@rifont rifont commented Nov 3, 2023

What change does this PR introduce?

  • @novu/shared:
    • Adds ApiServiceLevelTypeEnum
    • Adds RateLimitCategoryTypeEnum
  • @novu/dal:
    • Adds optional apiServiceLevel field to the Organization DAL
    • Adds optional rateLimits map field to the Environment DAL
  • @novu/api
    • Modifies create organization use-case to specify a apiServiceLevel: free for all new organizations

Why was this change needed?

The Organization and Environment entities require new fields to support API Request Rate Limiting.

Other information (Screenshots)

N/A

Copy link

linear bot commented Nov 3, 2023

NV-3058 🏎️ Rate Limiting DTOs

What?

  1. The Organization document must be updated to include two new fields: apiServiceLevel String field with a String value of either free or business
  2. The Environment document must be updated to include a rateLimit Subdocument, the rateLimit subdocument will comprise three Number fields: trigger, data, global
  3. Create a ApiServiceLevelTypeEnum enum in novu/shared with values of 'free' and 'business'
  4. Update OrganizationDto to include the apiServiceLevel String attribute
  5. Create the RateLimitDto
  6. Modify create Organization use-case to specify rate limit values on creation

Why? (Context)

The Organization, Environment, and RateLimit DTO will be accessed in the Rate Limiting Guard.

Definition of Done

  • New customers have an apiServiceLevel attribute Organization document
  • Existing customers can have a rateLimit subdocument assignable to the Organization document

@rifont rifont requested a review from LetItRock November 3, 2023 13:45
@rifont rifont changed the title Nv 3058 rate limiting dtos feat(dal, shared, api): Add rate limit DAL attributes Nov 3, 2023
Copy link
Contributor

@LetItRock LetItRock left a comment

Choose a reason for hiding this comment

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

🚀

export class OrganizationEntity {
_id: string;

name: string;

logo?: string;

// TODO: NV-3067 - Remove optional once all organizations have a service level
apiServiceLevel?: ApiServiceLevelTypeEnum;
Copy link
Contributor

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?

Copy link
Contributor Author

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:

  • Implicitly exclude non-decorated properties by design. Properties intended to be exposed must be decorated on a response DTO. This is a more failsafe option.
  • Explicitly exclude decorated properties. This is more intentional but requires discipline to ensure properties are decorated correctly.

I'll go with the DTO layer exclude approach in absence of the API layer exclusion for now.

Copy link
Contributor Author

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 the apiRateLimits 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.

Copy link
Contributor Author

@rifont rifont Nov 6, 2023

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.

@rifont rifont merged commit a6deb4e into next Nov 6, 2023
30 of 31 checks passed
@rifont rifont deleted the nv-3058-rate-limiting-dtos branch November 6, 2023 09:37
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.

3 participants