Skip to content
This repository has been archived by the owner on Nov 23, 2022. It is now read-only.

Ability to use function to determine request key for use in non-standard execution contexts #41

Open
smiley-uriux opened this issue Nov 23, 2020 · 2 comments
Labels
enhancement New feature or request should be checked Something in the library seems not right

Comments

@smiley-uriux
Copy link

I'm currently using this library within a service that includes both http handlers and custom rabbitmq RPC handlers. Because I register this module's interceptor globally, my RPC handlers are triggering errors because this library assumes that only one type of transport will ever be used and forces you to choose one using the "for" option. (The specific error comes from assuming the returned context request object will have an IP property). I can obviously stop using the interceptor globally and decorate my individual methods, however I'd really love to be able to use this rate-limiter for both transports.

I can imagine handling this in one of two ways.

  1. Continue assuming that only one transport type will be used, but actually check the type using context.getType(). A type of rpc probably shouldn't be processed by the interceptor when "fastify" or "express" are being configured. This along with the new PR that allows registering multiple rate limiters provides a decent enough workaround to handle my use case.

  2. Expose a "custom" option for the for parameter that calls a user defined function that gets passed the execution context (and probably a default handler that can be called to restore original behavior) and returns a key to be used for that request. This way you can inject custom logic and handle rpc/ws/http transports however you like and let the developer worry about how to resolve the key. The added bonus is that if somebody wants to use some brand new http server that has a custom request format, they'll still be able to use this library to handle rate-limiting and won't bother you to add support for yet another transport variant.

Personally I'd prefer option 2 since I think its more future proof but option 1 would be nice too and definitely save some headaches for people not expecting this interceptor to apply to non-http controller methods. Would consider putting the time into a PR if I knew it would be accepted and published without too much delay.

@onur-ozkan
Copy link
Owner

@smiley-uriux Did you try the latest alpha version 2.6.1-alpha?

@smiley-uriux
Copy link
Author

Thanks for getting back.

Sure did, the alpha version surfaced a different error, but on the same line. Looks like whatever is published as alpha on npm isn't reflected in the repo. Something about accessing "limiterKey" on undefined (this.spesificOptions). I think a super quick fix until more thought can be put into a complete solution would be to just bail in the intercept method if context.getType() returns 'rpc' or 'ws' since those are obviously not going to work with the current code.

@onur-ozkan onur-ozkan added enhancement New feature or request should be checked Something in the library seems not right labels Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request should be checked Something in the library seems not right
Projects
None yet
Development

No branches or pull requests

2 participants