Skip to content

fix: LRUCache.get() and __getitem__() fail for cached None values - #623

Open
koteshyelamati wants to merge 1 commit into
msiemens:masterfrom
koteshyelamati:master
Open

fix: LRUCache.get() and __getitem__() fail for cached None values#623
koteshyelamati wants to merge 1 commit into
msiemens:masterfrom
koteshyelamati:master

Conversation

@koteshyelamati

Copy link
Copy Markdown

Problem

LRUCache.get() and LRUCache.__getitem__() both break when None is a legitimate cached value.

get() bug: uses if value is not None: to detect cache hits, so if a key maps to None, it skips the LRU promotion and returns the default instead of the cached None.

__getitem__() bug: calls self.get(key) and then checks if value is None: raise KeyError, so it raises KeyError even when the key exists with value None.

cache = LRUCache(capacity=5)
cache['key'] = None

cache.get('key', 'missing')  # Returns 'missing' instead of None
cache['key']                  # Raises KeyError instead of returning None

Root cause

Both methods use the cached value itself as a sentinel to detect cache misses. This is the same class of bug that was fixed in set() by PR #597.

Fix

Replace the value-based checks with key not in self.cache membership tests, consistent with how set() was already fixed.

When a cached value is None, LRUCache.get() incorrectly returns the default
instead of the cached None, and LRUCache.__getitem__() raises KeyError even
though the key exists.

Root cause: both methods check `if value is not None` / `if value is None`
to detect cache misses, but this fails when None is a legitimate cached value.

Fix: check `if key not in self.cache` instead, mirroring the fix applied
to LRUCache.set() in PR msiemens#597.

Reproducer:
    cache = LRUCache(capacity=5)
    cache['key'] = None
    assert cache.get('key', 'default') == None  # fails before fix
    assert cache['key'] is None  # raises KeyError before fix
@msiemens

msiemens commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Thanks for the PR, @koteshyelamati! Could you add one or two tests for this?

@uadhran

uadhran commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Confirmed this still reproduces on current master (4aa5311, TinyDB 4.9.0) — not fixed by the recent docs/type PRs.

from tinydb.utils import LRUCache

c = LRUCache(capacity=5)
c['k'] = None

assert 'k' in c.cache          # key is present
print(c.get('k', 'missing'))   # 'missing'  (expected None)
# c['k'] → KeyError            (expected None)

Root cause matches the PR: get() / __getitem__() still use is not None as a hit check (tinydb/utils.py), while set() already uses membership (key in self.cache) after #596/#597.

Also worth noting: docs/changelog.rst currently says falsy values like 0 or None are handled correctly in LRUCache, but get/__getitem__ for stored None still fail — so this PR would also align the changelog claim with behavior.

Suggested tests (if helpful)

Happy to help with tests if useful — something like:

def test_lru_cache_none_value():
    cache = LRUCache(capacity=3)
    cache['key'] = None

    assert 'key' in cache.cache
    assert cache.get('key', 'missing') is None
    assert cache['key'] is None

    # LRU promotion still works for None hits
    cache['a'] = 1
    cache['b'] = 2
    cache.get('key')   # touch
    cache['c'] = 3     # would evict oldest if 'key' wasn't promoted
    assert cache.get('key') is None

(I don’t want to step on this PR — just adding verification + test ideas.)

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.

3 participants