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

Make the storage field in the ThrottlerStorage interface optional #1221

Closed
2 of 4 tasks
MitchellCash opened this issue Oct 27, 2022 · 2 comments · Fixed by #1565
Closed
2 of 4 tasks

Make the storage field in the ThrottlerStorage interface optional #1221

MitchellCash opened this issue Oct 27, 2022 · 2 comments · Fixed by #1565
Labels
bug Something isn't working

Comments

@MitchellCash
Copy link

MitchellCash commented Oct 27, 2022

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

Currently the storage field in the ThrottlerStorage interface is required:

export interface ThrottlerStorage {
/**
* The internal storage with all the request records.
* The key is a hashed key based on the current context and IP.
* The value of each item wil be an array of epoch times which indicate all
* the request's ttls in an ascending order.
*/
storage: Record<string, number[]>;

This makes sense for the in memory cache, but I have built my own Redis storage service which implements the ThrottlerStorage interface and I have no use for the storage field from what I can tell.

It would be nice if this field was deemed optional so I don't have unused code sitting in my ThrottlerRedisStorageService which implements ThrottlerStorage.

I am happy to make a PR for this fix, but I just wanted to confirm this field is indeed optional when using your own storage option.

Minimum reproduction code

See 'Steps to reproduce'.

Steps to reproduce

import { Injectable } from '@nestjs/common';
import { ThrottlerStorage } from '@nestjs/throttler';

@Injectable()
export class ThrottlerRedisStorageService implements ThrottlerStorage {
  // storage: Record<string, number[]>; // <-- Throws a TypeScript error when not visible.

  addRecord(key: string, ttl: number): Promise<void> {
    return Promise.resolve(undefined);
  }

  getRecord(key: string): Promise<number[]> {
    return Promise.resolve([]);
  }
}

Expected behavior

As far as I know, I do not require the storage field, so would prefer it to be optional to save an unused line of code.

Package version

3.1.0

NestJS version

9.1.4

Node.js version

16.18.0

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

No response

@MitchellCash MitchellCash added the bug Something isn't working label Oct 27, 2022
@jmcdo29
Copy link
Member

jmcdo29 commented Oct 27, 2022

Sounds like a good idea. We can probably actually just remove it from the interface, all that the guard needs to know is how to get a record and add a record

Ganesha2552 added a commit to Ganesha2552/throttler that referenced this issue Jun 16, 2023
Ability to set multiple rate limits with custom time intervals. Flexible storage options including
Redis, in-memory, and MongoDB.

BREAKING CHANGE: Multiple rate limits, Storage options

"fix nestjs#1369","fix nestjs#1221"
@Ganesha2552 Ganesha2552 mentioned this issue Jun 16, 2023
12 tasks
@jmcdo29
Copy link
Member

jmcdo29 commented Jul 6, 2023

Added to 5.0.0 in #1565

@jmcdo29 jmcdo29 linked a pull request Jul 6, 2023 that will close this issue
12 tasks
@jmcdo29 jmcdo29 mentioned this issue Jul 6, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants