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.

Sign up for GitHub

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

Avoid empty set on multi_cached #490

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Avoid empty set on multi_cached #490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion aiocache/decorators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ async def decorator(
result = await f(*new_args, **kwargs)
result.update(partial)

if cache_write:
if result and cache_write:
Copy link
Member

Choose a reason for hiding this comment

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

As a user... with this (not that before it worked as expected) it means that the execution will be executed every time the results are empty, is that the behavior you would expect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, in my case I already had something like:

if not params:
    return {}

So it wouldn't make any difference. But I can understand that in some cases you might want to do some logic even if the params are empty because everything is cached already.

I would be against putting a new configuration to control this (too many already), so there are 2 options:

  1. Always call the method even if the params are empty. If you don't want to do anything a shortcut can be added at the beginning of the method to just return, so the behaviour can be controlled.
  2. Never call the method when the params are empty. In this case if I wanted to do something in the method (like logging) I would not have any workaround.

I would go for option 1 then.

Copy link
Member

Choose a reason for hiding this comment

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

this looks similar to #452. Maybe the 3rd option is what @paulo-raca mentions in the issue of having an "empty" token and write this into Redis (and others). Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would work too, since in that case we would be writing the "guard" value internally instead of the empty dict that causes the error.

Copy link
Member

Choose a reason for hiding this comment

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

I think that discussion relates more to the other decorators, where the expected return value may be None.

For multi_cached though, we should clearly define what an empty dict signifies. I suspect this change is fine and it can be treated as a 'no-cache' for the result, indicating that something failed temporarily and a result couldn't be retrieved. Alternatively, it could simply be an error and a function is expected to always return values for the keys requested. Or it could be treated as a normal result that should be cached.

I'm not sure what the use cases are for this though.

if aiocache_wait_for_write:
await self.set_in_cache(result, f, args, kwargs)
else:
Expand Down
13 changes: 12 additions & 1 deletion tests/acceptance/test_decorators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import asyncio
import pytest
import random

from unittest import mock

from aiocache import cached, cached_stampede, multi_cached
Expand Down Expand Up @@ -110,6 +109,18 @@ async def test_multi_cached(self, cache):
for key in default_keys:
assert await cache.get(key) is not None

@pytest.mark.asyncio
async def test_multi_cached_with_empty_dict(self, mocker, cache):
mocker.spy(cache, "set")

@multi_cached("keys")
async def fn(keys):
return {}

await fn([pytest.KEY])
assert await cache.exists(pytest.KEY) is False
assert cache.set.call_count == 0

@pytest.mark.asyncio
async def test_keys_without_kwarg(self, cache):
@multi_cached("keys")
Expand Down