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

code objects analysis replaced with inspect #487

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Trizalio
Copy link

multi_cached works incorrectly with decorated functions, called with keys_from_attr value passed as positional argument, example:

import asyncio
import functools
from typing import List, Dict

from aiocache import multi_cached


async def foo(ids: List[int]) -> Dict[int, int]:
    return {id_: id_ ** 2 for id_ in ids}


def simple_decorator(f):
    @functools.wraps(f)
    async def wrapper(*args, **kwargs):
        return await f(*args, **kwargs)
    return wrapper


cached_foo = multi_cached("ids", namespace="main")(foo)
cached_wrapped_foo = multi_cached("ids", namespace="main")(simple_decorator(foo))


async def amain():
    await cached_foo(ids=[1])  # ok
    await cached_foo([1])  # ok
    await cached_wrapped_foo(ids=[1])  # ok
    await cached_wrapped_foo([1])  # EXCEPTION

if __name__ == "__main__":
    asyncio.run(amain())

This problem's reason is analysis of code object of function passed to multi_cached decorator.
To properly get arguments from function decorated with functools.wraps, it's required to get signature of inner function. Which can be accessed by __wrapped__ attribute, or by inspect.signature function, which goes recursively through __wrapped__ attributes till core function.
I suggest to used inspect.signature instead of direct code obejects inteaction.

@codecov
Copy link

codecov bot commented Apr 16, 2020

Codecov Report

Merging #487 into master will decrease coverage by 0.60%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #487      +/-   ##
==========================================
- Coverage   99.13%   98.52%   -0.61%     
==========================================
  Files          13       11       -2     
  Lines        1043      952      -91     
  Branches      116      104      -12     
==========================================
- Hits         1034      938      -96     
  Misses          9        9              
- Partials        0        5       +5     
Impacted Files Coverage Δ
aiocache/decorators.py 94.03% <100.00%> (-5.97%) ⬇️
aiocache/serializers/serializers.py 95.08% <0.00%> (-0.24%) ⬇️
aiocache/base.py 100.00% <0.00%> (ø)
aiocache/factory.py 100.00% <0.00%> (ø)
aiocache/backends/redis.py 100.00% <0.00%> (ø)
aiocache/backends/memory.py 100.00% <0.00%> (ø)
aiocache/backends/memcached.py 100.00% <0.00%> (ø)
aiocache/__init__.py
aiocache/plugins.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a966f84...d51ae83. Read the comment docs.

@Trizalio
Copy link
Author

Trizalio commented Apr 16, 2020

i'm quite new in coverage checks, can someone tell me, how is serializers.py file affected by my commit?

@argaen
Copy link
Member

argaen commented Oct 19, 2020

Some tests are failing so it can be because of this as a side effect. Don't check coverage until the build passes

@CLAassistant

This comment has been minimized.

@Dreamsorcerer
Copy link
Member

Hey, sorry for the delay, if you can merge master and get this running again, then we can look at merging this.

If I understand correctly, the test is only affected by function signatures, so you should also move the test to tests/ut/test_decorators.py and use a mock_cache instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants