From 2e9e72d2606e395db6dd67f695229e21ca34fa2a Mon Sep 17 00:00:00 2001 From: allen0099 Date: Sat, 26 Sep 2026 11:07:56 +0000 Subject: [PATCH] fix(backends): recognise counters the same way on every backend counter_value() parsed any entry whose content was a number, so on the memory backend and the base fallback increment() on a cached response with body "42" returned 43 and overwrote it, while Redis and Memcached raised CacheXError. It now also requires COUNTER_FINGERPRINT. A counter written with set(key, counter_entry(n)) was stored as a JSON document on Redis and Memcached, which INCR rejects. encode_entry now stores exactly what counter_entry builds as a bare integer. The codec read any value int() accepts as a counter, including " 7" and "1_0". parse_counter accepts only an optional minus sign and ASCII digits, plus the trailing spaces Memcached pads a value with when DECR shortens it. Closes #111 --- CHANGELOG.md | 10 ++ docs/BACKENDS.md | 4 +- fastapi_cachex/backends/codec.py | 20 ++-- fastapi_cachex/types.py | 29 +++-- tests/backends/test_codec.py | 31 +++++- tests/backends/test_counter_contract.py | 134 ++++++++++++++++++++++++ 6 files changed, 211 insertions(+), 17 deletions(-) create mode 100644 tests/backends/test_counter_contract.py diff --git a/CHANGELOG.md b/CHANGELOG.md index d085818..b56c2e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,16 @@ Note that 0.3.3 was never released; 0.3.4 follows 0.3.2. UTF-8; the parameter is removed in 0.4.0 (#126). ([#122](https://github.com/allen0099/FastAPI-CacheX/issues/122)) +- **Counters are recognised the same way on every backend.** On the memory + backend and the base-class fallback, `increment()` on a cached response + whose body was a number, such as `42`, returned 43 and overwrote the + response. It now raises `CacheXError`, as Redis and Memcached already did. A + counter written with `set(key, counter_entry(n))` can now be incremented on + Redis and Memcached too, because it is stored as a bare integer. Stored + values such as `" 7"` or `"1_0"`, which `int()` accepts, are no longer + read as counters. + ([#111](https://github.com/allen0099/FastAPI-CacheX/issues/111)) + ## [0.3.7] - 2026-09-25 ### Added diff --git a/docs/BACKENDS.md b/docs/BACKENDS.md index 1fde999..02d3f4d 100644 --- a/docs/BACKENDS.md +++ b/docs/BACKENDS.md @@ -156,7 +156,9 @@ if await backend.set_if_absent(f"stream:{user_id}", owner, ttl=300): counter is visible through `get()` as a `CacheEntry` with fingerprint `COUNTER_FINGERPRINT` and the decimal value as content, so `delete`/`clear*` and the monitoring routes treat it like any other entry. Incrementing a key - that holds a cached response raises `CacheXError`. + that holds anything else raises `CacheXError` on every backend, even a cached + response whose body is a number. A counter written with + `set(key, counter_entry(n))` can be incremented on every backend. - `get_and_delete(key) -> CacheEntry | None` — Memory pops under its lock, Redis uses `GETDEL` (server 6.2+) and Memcached returns the value only when its own `DELETE` won. `StateManager.consume_state`, `StateManager.delete_state`, diff --git a/fastapi_cachex/backends/codec.py b/fastapi_cachex/backends/codec.py index 6539494..ba88598 100644 --- a/fastapi_cachex/backends/codec.py +++ b/fastapi_cachex/backends/codec.py @@ -4,9 +4,11 @@ it is installed and the standard library ``json`` module otherwise. """ +from fastapi_cachex.types import COUNTER_FINGERPRINT from fastapi_cachex.types import DEFAULT_STATUS_CODE from fastapi_cachex.types import CacheEntry from fastapi_cachex.types import counter_entry +from fastapi_cachex.types import parse_counter try: import orjson as json @@ -27,7 +29,15 @@ def encode_entry(entry: CacheEntry) -> bytes: The raw content bytes are passed through ``latin-1`` so that arbitrary bytes round-trip through JSON text. + + A counter entry (exactly what ``counter_entry`` builds) is stored as a bare + integer instead, the form the server-side ``INCR`` family works on, so a + counter written with ``set()`` can still be passed to ``increment()``. """ + if entry.fingerprint == COUNTER_FINGERPRINT: + value = parse_counter(entry.content) + if value is not None and entry == counter_entry(value): + return entry.content serialized: str | bytes = json.dumps( { "fingerprint": entry.fingerprint, @@ -41,14 +51,6 @@ def encode_entry(entry: CacheEntry) -> bytes: return serialized if isinstance(serialized, bytes) else serialized.encode("utf-8") -def _as_counter(raw: str | bytes) -> int | None: - """The integer a bare counter value holds, or ``None`` for anything else.""" - try: - return int(raw) - except ValueError: - return None - - def decode_entry(raw: str | bytes | None) -> CacheEntry | None: """Rebuild a ``CacheEntry`` from a stored value. @@ -63,7 +65,7 @@ def decode_entry(raw: str | bytes | None) -> CacheEntry | None: """ if raw is None: return None - counter = _as_counter(raw) + counter = parse_counter(raw) if counter is not None: return counter_entry(counter) try: diff --git a/fastapi_cachex/types.py b/fastapi_cachex/types.py index 5fb6d74..815e407 100644 --- a/fastapi_cachex/types.py +++ b/fastapi_cachex/types.py @@ -1,5 +1,6 @@ """Type definitions and type aliases for FastAPI-CacheX.""" +import re from collections.abc import Callable from dataclasses import dataclass @@ -59,15 +60,31 @@ def counter_entry(value: int) -> CacheEntry: return CacheEntry(fingerprint=COUNTER_FINGERPRINT, content=str(value).encode()) +# Trailing spaces are allowed because Memcached pads a value that DECR made +# shorter instead of resizing it (DECR on "10" leaves "9 "). +_COUNTER_PATTERN = re.compile(rb"-?[0-9]+ *") + + +def parse_counter(raw: str | bytes) -> int | None: + """The integer ``raw`` spells as a plain decimal, or ``None`` otherwise. + + Stricter than ``int()``: leading whitespace, underscores, a ``+`` sign and + non-ASCII digits are rejected. Only the shapes the server-side ``INCR`` + family of Redis and Memcached leaves behind are accepted. + """ + data = raw.encode() if isinstance(raw, str) else raw + return int(data) if _COUNTER_PATTERN.fullmatch(data) else None + + def counter_value(entry: CacheEntry) -> int: """Read the integer a counter entry holds. Raises: - CacheXError: If the content is not a decimal integer, i.e. the key - holds a cached response rather than a counter. + CacheXError: If the entry is not a counter, e.g. the key holds a + cached response, even one whose body is a number. """ - try: - return int(entry.content) - except ValueError as e: + value = parse_counter(entry.content) + if entry.fingerprint != COUNTER_FINGERPRINT or value is None: msg = "Cache key holds a value that is not a counter" - raise CacheXError(msg) from e + raise CacheXError(msg) + return value diff --git a/tests/backends/test_codec.py b/tests/backends/test_codec.py index 4a385dc..07720f7 100644 --- a/tests/backends/test_codec.py +++ b/tests/backends/test_codec.py @@ -55,10 +55,39 @@ def test_decode_entry_treats_malformed_documents_as_a_miss(raw): assert codec.decode_entry(raw) is None -@pytest.mark.parametrize("raw", ["7", b"7", b"7 ", " 7\n"]) +@pytest.mark.parametrize("raw", ["7", b"7", b"7 ", "7 "]) def test_decode_entry_reads_a_bare_integer_as_a_counter(raw): + """Trailing spaces are Memcached's padding after a shrinking DECR.""" assert codec.decode_entry(raw) == counter_entry(7) +@pytest.mark.parametrize("raw", [b" 7", b" 7 ", "7\n", b"1_0", b"+7", chr(0x0667), b""]) +def test_decode_entry_rejects_what_int_accepts_but_no_server_writes(raw): + """``int()`` reads all of these; none of them is a counter (#111).""" + assert codec.decode_entry(raw) is None + + +def test_encode_entry_stores_a_counter_as_a_bare_integer(): + """So ``increment()`` works on a counter written with ``set()`` (#111).""" + assert codec.encode_entry(counter_entry(-5)) == b"-5" + assert codec.decode_entry(codec.encode_entry(counter_entry(-5))) == counter_entry( + -5 + ) + + +@pytest.mark.parametrize( + "entry", + [ + CacheEntry(fingerprint="etag", content=b"42"), + CacheEntry(fingerprint="counter", content=b"42", media_type="text/plain"), + CacheEntry(fingerprint="counter", content=b"042"), + CacheEntry(fingerprint="counter", content=b"abc"), + ], +) +def test_encode_entry_keeps_anything_else_as_a_document(entry): + """Only exactly what ``counter_entry`` builds is stored bare.""" + assert codec.decode_entry(codec.encode_entry(entry)) == entry + + def test_decode_entry_reads_a_negative_counter(): assert codec.decode_entry("-2") == counter_entry(-2) diff --git a/tests/backends/test_counter_contract.py b/tests/backends/test_counter_contract.py new file mode 100644 index 0000000..8b88a64 --- /dev/null +++ b/tests/backends/test_counter_contract.py @@ -0,0 +1,134 @@ +"""A counter is recognised the same way on every backend (#111). + +The memory backend and the base fallback parsed any entry whose body was a +number as a counter, so `increment()` on a cached response with body `42` +returned 43 and overwrote it, while Redis and Memcached raised. And a counter +written with `set(key, counter_entry(n))` could be incremented only on the +memory backend, because the network backends stored it as a JSON document. +""" + +from collections.abc import AsyncIterator +from collections.abc import Callable + +import pytest +import pytest_asyncio + +from fastapi_cachex.backends import AsyncRedisCacheBackend +from fastapi_cachex.backends import MemcachedBackend +from fastapi_cachex.backends.base import BaseCacheBackend +from fastapi_cachex.backends.memory import MemoryBackend +from fastapi_cachex.exceptions import CacheXError +from fastapi_cachex.types import COUNTER_FINGERPRINT +from fastapi_cachex.types import CacheEntry +from fastapi_cachex.types import counter_entry +from fastapi_cachex.types import counter_value +from fastapi_cachex.types import parse_counter +from tests.backends.test_base import DictBackend +from tests.live_servers import MEMCACHED_SERVER +from tests.live_servers import REDIS_HOST +from tests.live_servers import REDIS_PORT +from tests.live_servers import memcached_skip_reason +from tests.live_servers import redis_skip_reason + + +def _redis() -> BaseCacheBackend: + return AsyncRedisCacheBackend( + host=REDIS_HOST, port=REDIS_PORT, key_prefix="cachex-test-counter:" + ) + + +def _memcached() -> BaseCacheBackend: + return MemcachedBackend(servers=[MEMCACHED_SERVER]) + + +FACTORIES: dict[ + str, tuple[Callable[[], BaseCacheBackend], Callable[[], str | None]] +] = { + "memory": (MemoryBackend, lambda: None), + "fallback": (DictBackend, lambda: None), + "redis": (_redis, redis_skip_reason), + "memcached": (_memcached, memcached_skip_reason), +} +KEYS = ("counter-contract-page", "counter-contract-hits") + + +@pytest_asyncio.fixture(params=list(FACTORIES)) +async def backend(request: pytest.FixtureRequest) -> AsyncIterator[BaseCacheBackend]: + factory, skip_reason = FACTORIES[request.param] + reason = skip_reason() + if reason is not None: + pytest.skip(reason) + instance = factory() + await instance.delete_many(list(KEYS)) + yield instance + await instance.delete_many(list(KEYS)) + + +@pytest.mark.asyncio +async def test_increment_rejects_a_cached_response_with_a_numeric_body( + backend: BaseCacheBackend, +) -> None: + page = CacheEntry(fingerprint="etag", content=b"42") + await backend.set(KEYS[0], page) + + with pytest.raises(CacheXError, match="not a counter"): + await backend.increment(KEYS[0]) + + assert await backend.get(KEYS[0]) == page + + +@pytest.mark.asyncio +async def test_increment_continues_a_counter_written_with_set( + backend: BaseCacheBackend, +) -> None: + await backend.set(KEYS[1], counter_entry(5)) + + assert await backend.increment(KEYS[1]) == 6 + assert await backend.increment(KEYS[1], -2) == 4 # Memcached stops at 0 + assert await backend.get(KEYS[1]) == counter_entry(4) + + +@pytest.mark.asyncio +async def test_a_counter_that_shrinks_reads_back_as_a_counter( + backend: BaseCacheBackend, +) -> None: + """Memcached pads the value DECR shortens ("10" -> "9 ").""" + await backend.increment(KEYS[1], 10) + await backend.increment(KEYS[1], -1) + + assert await backend.get(KEYS[1]) == counter_entry(9) + + +@pytest.mark.parametrize( + ("raw", "expected"), + [ + ("7", 7), + (b"-7", -7), + (b"9 ", 9), + (b" 7", None), + (b"7\n", None), + (b"1_0", None), + (b"+7", None), + (chr(0x0667), None), # ARABIC-INDIC DIGIT SEVEN, which int() accepts + (b"", None), + (b"-", None), + ], +) +def test_parse_counter_accepts_only_what_the_servers_write( + raw: str | bytes, expected: int | None +) -> None: + assert parse_counter(raw) == expected + + +@pytest.mark.parametrize( + "entry", + [ + CacheEntry(fingerprint="etag", content=b"42"), + CacheEntry(fingerprint=COUNTER_FINGERPRINT, content=b"1_0"), + ], +) +def test_counter_value_requires_the_counter_fingerprint_and_a_plain_integer( + entry: CacheEntry, +) -> None: + with pytest.raises(CacheXError, match="not a counter"): + counter_value(entry)