alisaifee / limits

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

Enable lazy init for mongoDB storage + add base class for easy reuse of MongoClient #218

Closed rdar-lab closed 6 months ago

rdar-lab commented 7 months ago
  1. Fix issue with UWSGI compatibility by using lazy-init
  2. Expose a Base mongo storage that can be used (to reuse application level MongoClient)
rdar-lab commented 7 months ago

To clarify the changes:

  1. Change the init to lazy-init so it will happen once upon first usage. This should solve the issue of the monitoring thread being created using the wrong process (it will only be created post-fork).
  2. Enable a Base class for MongoDB storage—This enables you to easily register a custom implementation of the MongoDB storage. By re-implementing _init_mongo_client(), you can reuse the "MongoClient" instance. This is important because "MongoClient" actually creates an entire connection pool.
alisaifee commented 7 months ago

Would this problem not occur for the async storage?

alisaifee commented 7 months ago

For the linting errors, try running make lint-fix

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.02%. Comparing base (e84f8f5) to head (7e2e8c8).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #218 +/- ## ========================================== + Coverage 96.97% 97.02% +0.04% ========================================== Files 25 25 Lines 1256 1276 +20 Branches 134 138 +4 ========================================== + Hits 1218 1238 +20 Misses 19 19 Partials 19 19 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rdar-lab commented 6 months ago

For the linting errors, try running make lint-fix

Done

rdar-lab commented 6 months ago

Would this problem not occur for the async storage?

I wouldn't know. Did not work with the async version

rdar-lab commented 6 months ago

I was able to resolve the lint errors in the MongoDB, but only with the ignore statement:

  1. I cannot use ("") types for methods. Seems reasonable limitation, but required me to use Any
  2. Any is configured to not be supported - this seems logical, but in my use case I need to either use direct import on Mongo types or Any
  3. Using direct typing of course is wrong since the import will fail for users that do not have Mongo in their requirements.txt

Therefore the only solution I found was:

  1. Switch all methods to use "Any" (or simply do not provide the return type indication)
  2. Add ignore instructions on the file level
rdar-lab commented 6 months ago

@alisaifee Did you happen to see my latest comments?

rdar-lab commented 6 months ago

Hi @alisaifee I saw a few issues regarding lint on files I did not touch:

limits/aio/storage/base.py:30: error: Type of decorated function contains type "Any" ("_Wrapped[P, Awaitable[R], P, Coroutine[Any, Any, R]]") [misc] Found 1 error in 1 file (checked 27 source files)

I don't believe it is related to my changes. Can you verify the base version does not have the same issue?

alisaifee commented 6 months ago

Hi @alisaifee I saw a few issues regarding lint on files I did not touch:

limits/aio/storage/base.py:30: error: Type of decorated function contains type "Any" ("_Wrapped[P, Awaitable[R], P, Coroutine[Any, Any, R]]") [misc] Found 1 error in 1 file (checked 27 source files)

I don't believe it is related to my changes. Can you verify the base version does not have the same issue?

Apologies for the delay in getting to this PR. I'm looking at this at the moment - it's definitely not related to your change and exists in master (with an updated version of mypy)

alisaifee commented 6 months ago

@rdar-lab if you want to try rebasing from master I've added a temporary workaround for the typing issue.

alisaifee commented 6 months ago

I was able to resolve the lint errors in the MongoDB, but only with the ignore statement:

  1. I cannot use ("") types for methods. Seems reasonable limitation, but required me to use Any
  2. Any is configured to not be supported - this seems logical, but in my use case I need to either use direct import on Mongo types or Any
  3. Using direct typing of course is wrong since the import will fail for users that do not have Mongo in their requirements.txt

Therefore the only solution I found was:

  1. Switch all methods to use "Any" (or simply do not provide the return type indication)
  2. Add ignore instructions on the file level

For the other storage types I've done something like this - I wonder if something similar could be applied?

alisaifee commented 6 months ago

Generally this looks fine to me. If it's too tricky to resolve the type errors, I can take a crack at that post merge / as a follow up as well.

rdar-lab commented 6 months ago

@alisaifee , I rebased it. Can you run the checks again

alisaifee commented 6 months ago

@rdar-lab have you verified this branch works for the failing scenario you had?

rdar-lab commented 6 months ago

@alisaifee yes I did. I used the modified code and checked it

alisaifee commented 6 months ago

Awesome, I'll merge this and look at making a release tomorrow!

alisaifee commented 6 months ago

Just FYI: https://github.com/alisaifee/limits/commit/171b412afe7e2c09b907635b6f6cf45501e0b8d7 makes the type annotations a bit better though there is still some need for Any and type ignores.