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

LRU .values() and dict return old entries #348

Closed
nicoddemus opened this issue Oct 10, 2023 · 3 comments
Closed

LRU .values() and dict return old entries #348

nicoddemus opened this issue Oct 10, 2023 · 3 comments

Comments

@nicoddemus
Copy link
Contributor

Hi,

First of all thanks for the excellent library!

I'm seeing strange results when using LRU: after replacing an existing entry with a new value, seems like the keys() and values() methods return the old value, instead of the new one. Using __getitem__ and .get we have the expected behavior.

Here is a test showing this behavior:

def test_lru():
    from boltons.cacheutils import LRU

    cache = LRU()

    # Add an entry.
    cache['a'] = 1
    
    # Normal __getitem__ access. 
    assert cache['a'] == 1  # passes.
    # Convert to dict.
    assert dict(cache) == {'a': 1}  # passes.
    # Another way to access the only value.
    assert list(cache.values())[0] == 1  # passes.

    # Replace the existing 'a' entry with a new value.
    cache['a'] = 200

    # __getitem__ works as expected.
    assert cache['a'] == 200  # passes.

    # Both dict and accessing via values() return the old entry: 1.
    assert dict(cache) == {'a': 200}  # fails.
    assert list(dict(cache).values())[0] == 200  # fails.

This test fails in the last asserts (of course to see the 2nd failure one needs to comment the first):

λ pytest foo.py
======================== test session starts ========================
platform win32 -- Python 3.10.9, pytest-7.4.2, pluggy-1.3.0
rootdir: c:\Users\bruno\projects\boltons
configfile: pytest.ini
collected 1 item

foo.py F                                                       [100%]

============================= FAILURES ==============================
_____________________________ test_lru ______________________________

    def test_lru():
        from boltons.cacheutils import LRU

        cache = LRU()

        # Add an entry.
        cache['a'] = 1

        # Normal __getitem__ access.
        assert cache['a'] == 1  # passes.
        # Convert to dict.
        assert dict(cache) == {'a': 1}  # passes.
        # Another way to access the only value.
        assert list(cache.values())[0] == 1  # passes.

        # Replace the existing 'a' entry with a new value.
        cache['a'] = 200

        # __getitem__ works as expected.
        assert cache['a'] == 200  # passes.

        # Both dict and accessing via values() return the old entry: 1.
>       assert dict(cache) == {'a': 200}  # fails.
E       AssertionError: assert {'a': 1} == {'a': 200}
E         Differing items:
E         {'a': 1} != {'a': 200}
E         Use -v to get more diff

foo.py:23: AssertionError
====================== short test summary info ======================
FAILED foo.py::test_lru - AssertionError: assert {'a': 1} == {'a': 200}
========================= 1 failed in 0.04s =========================

At first it seems like a bug? Or perhaps this usage is not meant to be supported?

mahmoud added a commit that referenced this issue Oct 12, 2023
@mahmoud
Copy link
Owner

mahmoud commented Oct 12, 2023

Hey Bruno! Good to see you here again. Thanks for the kind words and detailed report. I confirmed I'm seeing the same behavior.

I never really had that usage pattern IRL, but I think it's a great one to support. I've got a fix on a branch that makes it work. I just pushed to #349. Still need to think about it a bit, but feel free to take a look!

mahmoud added a commit that referenced this issue Oct 30, 2023
* test and fix for #348

* quick fix for py27 ci

* another shot at a quick py27 ci fix

* caching doesn't work oob in containers, another 2.7 ci fix

* Also test LRI overwriting

Co-authored-by: Bruno Oliveira <[email protected]>

---------

Co-authored-by: Bruno Oliveira <[email protected]>
@mahmoud
Copy link
Owner

mahmoud commented Oct 31, 2023

Fix is now up in 23.1.0. Thanks again for the report/review!

@mahmoud mahmoud closed this as completed Oct 31, 2023
@nicoddemus
Copy link
Contributor Author

Awesome, thanks!

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

No branches or pull requests

2 participants