Handle rest_framework.throttling.SimpleRateThrottle #459

JulianMaurin · 2022-07-19T19:50:50Z

Handle rest_framework.throttling.SimpleRateThrottle

According to the issue: #382.

I can confirm that the issue comes from an incompatibility with the DRF throttling view (see: DRF code).

Once the SimpleRateThrottle.timer is patched with fake_time, executing self.timer() (equivalent to timer(self)) is working with time.time but not with fake_time:

How to reproduce

>>> import time
>>> class A:
...     timer = time.time
...     def test(self):
...             print(self.timer())
... 
>>> A().test()
1658255082.1697752
>>>
>>> from freezegun.api import fake_time
>>> class B:
...     timer = fake_time
...     def test(self):
...             print(self.timer())
... 
>>> B().test()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 4, in test
TypeError: fake_time() takes 0 positional arguments but 1 was given

About the fix

It was not that simple. Testing an API using the test client will load the module after the patch that is performed by Freezgun. To reproduce the context of late loading I introduce the tests.fake_module_lazy module (In order to add a test demonstrating the error).

Then, to fix the error I changed the signature of the freezegun.api.fake_time function, to accept any args/kwars (and so allow the self.timer usage at the origin of the issue).

I feel the change is not risky at all but I can miss something.

tests/test_class_import.py

…_time function

JulianMaurin · 2022-08-09T18:38:27Z

@staticdev, @boxed ,

Any opinion about this PR?

boxed · 2022-08-10T05:10:38Z

freezegun/api.py

@@ -170,7 +170,7 @@ def get_current_time():
    return freeze_factories[-1]()


-def fake_time():
+def fake_time(*args, **kwargs):


Why *args, **kwargs here? That seems very wrong to me.

Hello @boxed ,
Thanks for your review. May I humbly ask you to elaborate?

I think I made an effort to explain the reproduction of the bug, both in the description and in the test.
If I'm off track, can you possibly guide me to a solution that you think is more appropriate?

@JulianMaurin this is breaking the API typing on api.pyi, so users that have mypy will have problems. I also think you can add typed arguments to satisfy your needs with default arguments=None so it will also not break current applications and maintain the types well defined.

I thought this version does not break the application, in addition, fake_time function is not defined in api.pyi, but maybe I did not understand something.

Would you prefer this version (with a docstring ofc):

Suggested change

def fake_time(*args, **kwargs):

def fake_time(_ : Any):

@JulianMaurin putting an argument of type Any makes the method still untyped. Also as @boxed adding *args, **kwargs here does not make sense to me also, since to fix the bug you one need to be able to call time it with self.

You can check in the definition of builtin time.time there is NO argument whatsoever: https://github.com/python/cpython/blob/v3.9.6/Modules/timemodule.c#L86

Have to tried, instead of adding arguments, wrapping fake_time with @classmethod? Maybe it will fix this usage. If not I am sure some kind of wrapper like this will be needed.

staticdev · 2022-08-10T17:46:36Z

freezegun/api.py

@@ -170,7 +170,7 @@ def get_current_time():
    return freeze_factories[-1]()


-def fake_time():
+def fake_time(*args, **kwargs):


@JulianMaurin this is breaking the API typing on api.pyi, so users that have mypy will have problems. I also think you can add typed arguments to satisfy your needs with default arguments=None so it will also not break current applications and maintain the types well defined.

staticdev · 2022-08-10T17:48:35Z

tests/test_class_import.py

@@ -130,6 +131,14 @@ def test_fake_strftime_function():
    assert fake_strftime_function() == '2012'


+@freeze_time("2022-01-01")
+def test_fake_time_function_as_class_attribute():


It is great that you added tests, but it is also important to explain on README the new optional usage in English so more people can easily reuse your solution =)

You can also add some description of this change on CHANGELOG.

JulianMaurin

Hey @staticdev ,
Thank you for taking the time to develop.

I will add an entry to the changelog if we continue this PR, thanks for the suggestion 🙇

However I don't see what should be added to the README, can you be more precise?

It would be nice to fix this bug, but if you think this is not the right way at all, tell me directly and I will close the PR and we will all save time.

JulianMaurin · 2022-08-11T06:31:38Z

freezegun/api.py

@@ -170,7 +170,7 @@ def get_current_time():
    return freeze_factories[-1]()


-def fake_time():
+def fake_time(*args, **kwargs):


I thought this version does not break the application, in addition, fake_time function is not defined in api.pyi, but maybe I did not understand something.

Would you prefer this version (with a docstring ofc):

Suggested change

def fake_time(*args, **kwargs):

def fake_time(_ : Any):

staticdev · 2022-08-11T14:07:19Z

Hey @staticdev , Thank you for taking the time to develop.

I will add an entry to the changelog if we continue this PR, thanks for the suggestion bow

However I don't see what should be added to the README, can you be more precise?

It would be nice to fix this bug, but if you think this is not the right way at all, tell me directly and I will close the PR and we will all save time.

@JulianMaurin I mean, you can add the usage in a class like you described in the description of your pull request.

I did not say you are doing it wrong, you are in the right path.

JulianMaurin added 2 commits July 19, 2022 21:25

test(rest_framework.throttling.SimpleRateThrottle): reproduce the error

f92dd37

fix(fake_time): accept any args/kwargs

4313a88

JulianMaurin commented Jul 19, 2022

View reviewed changes

tests/test_class_import.py Outdated Show resolved Hide resolved

refacto(test_class_import): factorize tests introducting the _date_to…

9d2b92f

…_time function

boxed reviewed Aug 10, 2022

View reviewed changes

staticdev suggested changes Aug 10, 2022

View reviewed changes

JulianMaurin commented Aug 11, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle rest_framework.throttling.SimpleRateThrottle #459

Handle rest_framework.throttling.SimpleRateThrottle #459

JulianMaurin commented Jul 19, 2022 •

edited

JulianMaurin commented Aug 9, 2022

boxed Aug 10, 2022

JulianMaurin Aug 10, 2022

staticdev Aug 10, 2022

JulianMaurin Aug 11, 2022 •

edited

staticdev Aug 11, 2022

staticdev Aug 10, 2022

staticdev Aug 10, 2022

staticdev Aug 10, 2022

JulianMaurin left a comment

JulianMaurin Aug 11, 2022 •

edited

staticdev commented Aug 11, 2022 •

edited

Handle rest_framework.throttling.SimpleRateThrottle #459

Are you sure you want to change the base?

Handle rest_framework.throttling.SimpleRateThrottle #459

Conversation

JulianMaurin commented Jul 19, 2022 • edited

Handle rest_framework.throttling.SimpleRateThrottle

How to reproduce

About the fix

JulianMaurin commented Aug 9, 2022

boxed Aug 10, 2022

Choose a reason for hiding this comment

JulianMaurin Aug 10, 2022

Choose a reason for hiding this comment

staticdev Aug 10, 2022

Choose a reason for hiding this comment

JulianMaurin Aug 11, 2022 • edited

Choose a reason for hiding this comment

staticdev Aug 11, 2022

Choose a reason for hiding this comment

staticdev Aug 10, 2022

Choose a reason for hiding this comment

staticdev Aug 10, 2022

Choose a reason for hiding this comment

staticdev Aug 10, 2022

Choose a reason for hiding this comment

JulianMaurin left a comment

Choose a reason for hiding this comment

JulianMaurin Aug 11, 2022 • edited

Choose a reason for hiding this comment

staticdev commented Aug 11, 2022 • edited

JulianMaurin commented Jul 19, 2022 •

edited

JulianMaurin Aug 11, 2022 •

edited

JulianMaurin Aug 11, 2022 •

edited

staticdev commented Aug 11, 2022 •

edited