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

add a make_cache_key parameter #159

Merged
merged 3 commits into from
May 31, 2020
Merged

Conversation

buckley-w-david
Copy link
Contributor

@buckley-w-david buckley-w-david commented Jan 29, 2020

This PR is essentially a more considered version of #76

After the additional discussion with @bzed I changed my stance on the functionality, but actually even further it seemed like what the library actually needed was a way to allow the consumer complete control over the cache key generation if they so wished.

I have added a make_cache_key parameter to the cached decorator for this purpose.

This should allow for use cases such as #71, an example of which is in the test added in this PR

library consumers can now set their own make_cache_key
attribute of the returned cache decorator, accomidating use cases such as
the one in Issue 71 without even needing to change the public API since
this attribute has been documented read/write for years.
@buckley-w-david
Copy link
Contributor Author

Hmm, CI has failed some tests that are passing locally for me, I guess I'll have to figure out why first

Copy link
Collaborator

@gergelypolonkai gergelypolonkai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the failing tests, it looks good to me. Let us know if you need help with the tests, i might be able to help tomorrow (CET)

@buckley-w-david
Copy link
Contributor Author

Thanks for the offer, but I think I'm fine to figure it out on my own, I'm able to reproduce the failures in a docker container, so I'll just debug them and then push a fix

This is technically a breaking API change, although a very small one, it
will have to be decided if that's okay.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 80.02% when pulling 8880c10 on buckley-w-david:master into b8e2fa6 on sh4nks:master.

@buckley-w-david
Copy link
Contributor Author

buckley-w-david commented Jan 29, 2020

I figured out the problem, but unfortunately the fix technically makes this a very small API change that could technically result in incompatibilities with previous versions.

The make_cache_key function previously set the use_request argument of _make_cache_key to False always, now it respects the caller setting that themselves. In almost all cases this will result in it working fine for anyone who was using make_cache_key previously.

There is however a case where this will break a previously valid use case, where the caller was using the make_cache_key attribute of the returned decorator for a view function that accepts an argument named use_request.

That is a very specific and probably unlikely scenario, but it is technically possible that this would now break existing code.

We'll have to decide if that's okay or not.

This change replaces the previous method of using the existing
make_cache_key attribute of the returned decorator since that method
required that a slight behavioral change to that method be introduced,
which technically breaks backwards compatibility
@buckley-w-david
Copy link
Contributor Author

buckley-w-david commented Feb 1, 2020

I've pushed a change so that this no longer works off of the make_cache_key attribute of the decorator and instead is simply a argument passed to the decorator.

This isn't as slick a solution as the previous one, but after thinking it over there's no real reason to break compatibility, even in a very small corner case, when it can just be done this way. Especially since if it ever did affect anyone, it would be almost impossible to debug on their part without inspecting the source of this library and really understanding what it was doing.

Also I noticed a bug in the test, so that has also been fixed.

I've edit the PR name/description to reflect this.

@buckley-w-david buckley-w-david changed the title refactor cached decorator to make it consistent with memoized add a make_cache_key parameter Feb 1, 2020
@buckley-w-david
Copy link
Contributor Author

Not sure why the tests are timing out in the first memcached test, I get no problems with them on my machine

...
tests/test_backend_cache.py::TestRedisCache::test_empty_host SKIPPED                                                             [ 46%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_get_dict PASSED                                                    [ 47%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_set_get PASSED                                                     [ 47%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_get_set PASSED                                                     [ 48%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_get_many PASSED                                                    [ 48%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_set_many PASSED                                                    [ 49%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_add PASSED                                                         [ 49%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_delete PASSED                                                      [ 50%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_delete_many PASSED                                                 [ 50%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_inc_dec PASSED                                                     [ 51%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_true_false PASSED                                                  [ 51%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_timeout PASSED                                                     [ 52%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_has PASSED                                                         [ 52%]
tests/test_backend_cache.py::TestMemcachedCache::test_compat PASSED                                                              [ 53%]
tests/test_backend_cache.py::TestMemcachedCache::test_huge_timeouts PASSED                                                       [ 53%]
tests/test_backend_cache.py::TestUWSGICache::test_generic_get_dict SKIPPED                                                       [ 54%]
...

@JimCurryWang
Copy link

Wanna ask when will flask-caching merge this PR, this feature is life-saver.

@sh4nks
Copy link
Collaborator

sh4nks commented May 31, 2020

Not sure why the tests are timing out in the first memcached test, I get no problems with them on my machine

...
tests/test_backend_cache.py::TestRedisCache::test_empty_host SKIPPED                                                             [ 46%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_get_dict PASSED                                                    [ 47%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_set_get PASSED                                                     [ 47%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_get_set PASSED                                                     [ 48%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_get_many PASSED                                                    [ 48%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_set_many PASSED                                                    [ 49%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_add PASSED                                                         [ 49%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_delete PASSED                                                      [ 50%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_delete_many PASSED                                                 [ 50%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_inc_dec PASSED                                                     [ 51%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_true_false PASSED                                                  [ 51%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_timeout PASSED                                                     [ 52%]
tests/test_backend_cache.py::TestMemcachedCache::test_generic_has PASSED                                                         [ 52%]
tests/test_backend_cache.py::TestMemcachedCache::test_compat PASSED                                                              [ 53%]
tests/test_backend_cache.py::TestMemcachedCache::test_huge_timeouts PASSED                                                       [ 53%]
tests/test_backend_cache.py::TestUWSGICache::test_generic_get_dict SKIPPED                                                       [ 54%]
...

I am having the same problem on my machine but when I started the tests a second time everything went fine. I believe it has something to do with the startup of the memcache server.

Thanks for this PR @buckley-w-david!

@sh4nks sh4nks merged commit 0b9e331 into pallets-eco:master May 31, 2020
@bzed
Copy link

bzed commented May 31, 2020

Thanks a lot @buckley-w-david @sh4nks @gergelypolonkai 👍

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants