search

alisaifee / limits

Rate limiting using various strategies and storage backends such as redis & memcached
https://limits.readthedocs.org
MIT License
410 stars 57 forks source link

Add namespace prefix to Redis keys #183

Closed hasier closed 10 months ago

hasier commented 11 months ago

Picking up the feedback for https://github.com/alisaifee/limits/pull/173 and based on the current solution for etcd, I added a PREFIX to the Redis storages and used it to generate its keys, therefore allowing for a more targeted cleanup of values in reset().

I opted for generating the keys right before they were used against the storage, but we may as well move the key generation right at the beginning of each of the entrypoints, i.e. all the public functions. Happy to hear your feedback.

Besides, I guess this might be considered a breaking change? Not sure if you'd like to do something specific about that or you are just happy to add that as a note in the release notes.

hasier commented 10 months ago

@alisaifee I appreciate it is a somewhat meaty PR and it takes time to review, any chance you can run tests for now to ensure at least CI passes?

hasier commented 10 months ago

@alisaifee sorry I had a dumb typo in a sync test 🤦 Could you re-run again when you have some time, please? 🙏

hasier commented 10 months ago

@alisaifee any chance you can look at this one? I think it'd be a very interesting change to merge as it brings the Redis storage in line with the rest and makes simpler to cleanup the data related to each of the individual namespaces (and I could really benefit of this one myself!). Thanks in advance for your time!

alisaifee commented 10 months ago

First up, very sorry for all the delays in handling this change request.

I'm a bit confused about what the goal of this change is. Effectively the code is refactored to be a bit more explicit about the prefix, however it still doesn't allow the user to reset limits with a custom namespace. They could of course override the class variable PREFIX - but that's not something that should be advertised

hasier commented 10 months ago

First up, very sorry for all the delays in handling this change request.

I'm a bit confused about what the goal of this change is. Effectively the code is refactored to be a bit more explicit about the prefix, however it still doesn't allow the user to reset limits with a custom namespace. They could of course override the class variable PREFIX - but that's not something that should be advertised

No worries @alisaifee! Thanks for looking into the changes. I think I took a slightly different path to the specific reset() problematic in the original issue, apologies for the confusion. I would say these changes mainly achieve a couple of things:

Regarding the reset() itself, looking at it again I find it difficult to fit the whole namespace thing within the storage itself, and therefore these changes. And I fully agree changing the PREFIX variable is not the way to go, this is just the tool to restrict the library keys into its own scope, as other storages already do and I explained above. The namespace seems more related to the RateLimitItem, and that is not passed down to the storages, they arrive as a merged key string (example usage here). If we wanted to make that available at such a low level I guess we should either include the concept of namespace in the storage init (although semantically it feels like it'd be restricting that storage acting on just one namespace and to me feels odd), or we pass down the namespace separated from the key as a new argument, and therefore introduce a breaking change. I would say we can better tackle the design and solution of this issue separately.

Hopefully this makes sense!

alisaifee commented 10 months ago

Yes, this makes sense. Thanks for breaking it down in detail and my apologies again for the delays in reviewing the PR.

I'll aim for making a release with this change today!