From 94522c54a2622168ebe1d431789a404f0511f7f7 Mon Sep 17 00:00:00 2001 From: Guillermo Perez Date: Tue, 31 Mar 2026 12:32:21 +0200 Subject: [PATCH] Make request flags frozen to avoid side-effects --- Cargo.lock | 2 +- Cargo.toml | 2 +- README.md | 23 ++++- meta_memcache_socket.pyi | 67 +++++++++----- src/request_flags.rs | 124 ++++++++++++++++++++----- src/request_flags_tests.rs | 167 ++++++++++++++++++++++++++++++++++ tests/test_memcache_socket.py | 49 ++++++++++ 7 files changed, 384 insertions(+), 50 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 304d783..8465821 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -113,7 +113,7 @@ checksum = "f8ca58f447f06ed17d5fc4043ce1b10dd205e060fb3ce5b979b8ed8e59ff3f79" [[package]] name = "meta-memcache-socket" -version = "0.2.0" +version = "0.3.0" dependencies = [ "atoi", "base64", diff --git a/Cargo.toml b/Cargo.toml index cec2e94..cb7280b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "meta-memcache-socket" -version = "0.2.0" +version = "0.3.0" edition = "2024" [lib] diff --git a/README.md b/README.md index a8c5207..fa0f487 100644 --- a/README.md +++ b/README.md @@ -29,7 +29,7 @@ meta-memcache-socket-py/ │ ├── lib.rs # PyO3 module entry — exports classes, functions, constants │ ├── constants.rs # Protocol constants (response codes, set modes, NOOP, ENDL) │ ├── memcache_socket.rs # MemcacheSocket class — socket I/O, buffering, GIL management -│ ├── request_flags.rs # RequestFlags class — mutable flags for building commands +│ ├── request_flags.rs # RequestFlags class — immutable flags for building commands │ ├── response_flags.rs # ResponseFlags class — immutable flags parsed from responses │ ├── response_types.rs # Response type classes (Value, Success, Miss, NotStored, Conflict) │ ├── impl_build_cmd.rs # Command builder — key validation, base64, flag encoding @@ -175,7 +175,7 @@ flags.opaque # Optional[bytes] — echoed opaque data (O) ### RequestFlags -Mutable container for flags sent with commands. +Immutable container for flags sent with commands. ```python from meta_memcache_socket import RequestFlags @@ -205,11 +205,28 @@ flags = RequestFlags( opaque=None, # O — opaque data echoed back mode=None, # M — operation mode (set/arithmetic) ) +``` + +The flags are immutable, so they can be reused safely across threads when +calling meta commands. Internal layers migth need to mutate flags +(content id, reduce ttl, etc...) and will mutate them use replace() to create +modified copies when needed. + +If you need to change flags on a existing RequestFlags, use the `replace()` method: -flags.copy() # -> RequestFlags (deep copy) +```python +new_flags = flags.replace(return_ttl=True, cache_ttl=600) # -> RequestFlags +``` + +You can also encode the flags into a byte string for command building, showing +exactly what will be sent on the wire: + +```python flags.to_bytes() # -> bytes (encoded flag string) ``` +For debugging purposes, stringifying it shows the flags in a human-readable format. + ### Command builders Convenience functions that build meta-protocol command byte strings. diff --git a/meta_memcache_socket.pyi b/meta_memcache_socket.pyi index 95b29a0..b7e53ed 100644 --- a/meta_memcache_socket.pyi +++ b/meta_memcache_socket.pyi @@ -1,5 +1,5 @@ import socket -from typing import Any, Optional, Tuple, Union +from typing import Any, Final, Optional, Tuple, Union RESPONSE_VALUE: int # 1 - VALUE (VA) RESPONSE_SUCCESS: int # 2 - SUCCESS (OK or HD) @@ -56,26 +56,26 @@ class RequestFlags: * mode: The mode to use when storing the value in the cache. See SET_MODE_* and MA_MODE_* constants """ - no_reply: bool - return_client_flag: bool - return_cas_token: bool - return_value: bool - return_ttl: bool - return_size: bool - return_last_access: bool - return_fetched: bool - return_key: bool - no_update_lru: bool - mark_stale: bool - cache_ttl: Optional[int] - recache_ttl: Optional[int] - vivify_on_miss_ttl: Optional[int] - client_flag: Optional[int] - ma_initial_value: Optional[int] - ma_delta_value: Optional[int] - cas_token: Optional[int] - opaque: Optional[bytes] - mode: Optional[int] + no_reply: Final[bool] + return_client_flag: Final[bool] + return_cas_token: Final[bool] + return_value: Final[bool] + return_ttl: Final[bool] + return_size: Final[bool] + return_last_access: Final[bool] + return_fetched: Final[bool] + return_key: Final[bool] + no_update_lru: Final[bool] + mark_stale: Final[bool] + cache_ttl: Final[Optional[int]] + recache_ttl: Final[Optional[int]] + vivify_on_miss_ttl: Final[Optional[int]] + client_flag: Final[Optional[int]] + ma_initial_value: Final[Optional[int]] + ma_delta_value: Final[Optional[int]] + cas_token: Final[Optional[int]] + opaque: Final[Optional[bytes]] + mode: Final[Optional[int]] def __init__( self, @@ -101,7 +101,30 @@ class RequestFlags: opaque: Optional[bytes] = None, mode: Optional[int] = None, ) -> None: ... - def copy(self) -> "RequestFlags": ... + def replace( + self, + *, + no_reply: Optional[bool] = None, + return_client_flag: Optional[bool] = None, + return_cas_token: Optional[bool] = None, + return_value: Optional[bool] = None, + return_ttl: Optional[bool] = None, + return_size: Optional[bool] = None, + return_last_access: Optional[bool] = None, + return_fetched: Optional[bool] = None, + return_key: Optional[bool] = None, + no_update_lru: Optional[bool] = None, + mark_stale: Optional[bool] = None, + cache_ttl: Optional[int] = None, + recache_ttl: Optional[int] = None, + vivify_on_miss_ttl: Optional[int] = None, + client_flag: Optional[int] = None, + ma_initial_value: Optional[int] = None, + ma_delta_value: Optional[int] = None, + cas_token: Optional[int] = None, + opaque: Optional[bytes] = None, + mode: Optional[int] = None, + ) -> "RequestFlags": ... def to_bytes(self) -> bytes: ... def __str__(self) -> str: ... diff --git a/src/request_flags.rs b/src/request_flags.rs index 4f129c6..8ada4e7 100644 --- a/src/request_flags.rs +++ b/src/request_flags.rs @@ -3,48 +3,48 @@ use pyo3::types::PyBytes; use crate::{MA_MODE_INC, SET_MODE_SET}; -#[pyclass(eq, skip_from_py_object)] +#[pyclass(eq, skip_from_py_object, frozen)] #[derive(Clone, Debug, PartialEq)] pub struct RequestFlags { - #[pyo3(get, set)] + #[pyo3(get)] no_reply: bool, - #[pyo3(get, set)] + #[pyo3(get)] return_client_flag: bool, - #[pyo3(get, set)] + #[pyo3(get)] return_cas_token: bool, - #[pyo3(get, set)] + #[pyo3(get)] return_value: bool, - #[pyo3(get, set)] + #[pyo3(get)] return_ttl: bool, - #[pyo3(get, set)] + #[pyo3(get)] return_size: bool, - #[pyo3(get, set)] + #[pyo3(get)] return_last_access: bool, - #[pyo3(get, set)] + #[pyo3(get)] return_fetched: bool, - #[pyo3(get, set)] + #[pyo3(get)] return_key: bool, - #[pyo3(get, set)] + #[pyo3(get)] no_update_lru: bool, - #[pyo3(get, set)] + #[pyo3(get)] mark_stale: bool, - #[pyo3(get, set)] + #[pyo3(get)] cache_ttl: Option, - #[pyo3(get, set)] + #[pyo3(get)] recache_ttl: Option, - #[pyo3(get, set)] + #[pyo3(get)] vivify_on_miss_ttl: Option, - #[pyo3(get, set)] + #[pyo3(get)] client_flag: Option, - #[pyo3(get, set)] + #[pyo3(get)] ma_initial_value: Option, - #[pyo3(get, set)] + #[pyo3(get)] ma_delta_value: Option, - #[pyo3(get, set)] + #[pyo3(get)] cas_token: Option, - #[pyo3(get, set)] + #[pyo3(get)] opaque: Option>, - #[pyo3(get, set)] + #[pyo3(get)] mode: Option, } @@ -251,8 +251,86 @@ impl RequestFlags { } } - pub fn copy(&self) -> Self { - self.clone() + /// Return a copy of this object with the specified fields replaced. + /// + /// Only keyword arguments that are explicitly provided (non-None) override the + /// corresponding field. Fields not mentioned keep their current value. + /// + /// Note: passing `None` explicitly for an optional field (e.g. `cache_ttl=None`) + /// keeps the existing value rather than unsetting it. To unset an optional field, + /// construct a new `RequestFlags` directly. + #[allow(clippy::too_many_arguments)] + #[pyo3( + signature = ( + /, + *, + no_reply=None, + return_client_flag=None, + return_cas_token=None, + return_value=None, + return_ttl=None, + return_size=None, + return_last_access=None, + return_fetched=None, + return_key=None, + no_update_lru=None, + mark_stale=None, + cache_ttl=None, + recache_ttl=None, + vivify_on_miss_ttl=None, + client_flag=None, + ma_initial_value=None, + ma_delta_value=None, + cas_token=None, + opaque=None, + mode=None + ) + )] + pub fn replace( + &self, + no_reply: Option, + return_client_flag: Option, + return_cas_token: Option, + return_value: Option, + return_ttl: Option, + return_size: Option, + return_last_access: Option, + return_fetched: Option, + return_key: Option, + no_update_lru: Option, + mark_stale: Option, + cache_ttl: Option, + recache_ttl: Option, + vivify_on_miss_ttl: Option, + client_flag: Option, + ma_initial_value: Option, + ma_delta_value: Option, + cas_token: Option, + opaque: Option>, + mode: Option, + ) -> Self { + RequestFlags { + no_reply: no_reply.unwrap_or(self.no_reply), + return_client_flag: return_client_flag.unwrap_or(self.return_client_flag), + return_cas_token: return_cas_token.unwrap_or(self.return_cas_token), + return_value: return_value.unwrap_or(self.return_value), + return_ttl: return_ttl.unwrap_or(self.return_ttl), + return_size: return_size.unwrap_or(self.return_size), + return_last_access: return_last_access.unwrap_or(self.return_last_access), + return_fetched: return_fetched.unwrap_or(self.return_fetched), + return_key: return_key.unwrap_or(self.return_key), + no_update_lru: no_update_lru.unwrap_or(self.no_update_lru), + mark_stale: mark_stale.unwrap_or(self.mark_stale), + cache_ttl: cache_ttl.or(self.cache_ttl), + recache_ttl: recache_ttl.or(self.recache_ttl), + vivify_on_miss_ttl: vivify_on_miss_ttl.or(self.vivify_on_miss_ttl), + client_flag: client_flag.or(self.client_flag), + ma_initial_value: ma_initial_value.or(self.ma_initial_value), + ma_delta_value: ma_delta_value.or(self.ma_delta_value), + cas_token: cas_token.or(self.cas_token), + opaque: opaque.or_else(|| self.opaque.clone()), + mode: mode.or(self.mode), + } } pub fn __str__(&self) -> String { diff --git a/src/request_flags_tests.rs b/src/request_flags_tests.rs index a587360..5827650 100644 --- a/src/request_flags_tests.rs +++ b/src/request_flags_tests.rs @@ -559,4 +559,171 @@ mod tests { b" q f c v t s l h k u I T1 R2 N3 F4 J5 D6 C7 Oop ME" ); } + + // Helper: all-None replace call (no overrides) + fn replace_none(flags: &RequestFlags) -> RequestFlags { + flags.replace( + None, None, None, None, None, None, None, None, None, None, None, None, None, None, + None, None, None, None, None, None, + ) + } + + #[test] + fn test_replace_no_args_returns_equal() { + let base = RequestFlags::new( + true, + true, + false, + false, + false, + false, + false, + false, + false, + false, + false, + Some(300), + None, + None, + None, + None, + None, + None, + None, + None, + ); + assert_eq!(replace_none(&base), base); + } + + #[test] + fn test_replace_bool_flag() { + let base = default_flags(); + let updated = base.replace( + Some(true), // no_reply + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + ); + assert_eq!(push_to_vec(&updated), b" q"); + // base is unchanged + assert_eq!(push_to_vec(&base), b""); + } + + #[test] + fn test_replace_optional_field() { + let base = default_flags(); + let updated = base.replace( + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + Some(600), // cache_ttl + None, + None, + None, + None, + None, + None, + None, + None, + ); + assert_eq!(push_to_vec(&updated), b" T600"); + assert_eq!(push_to_vec(&base), b""); + } + + #[test] + fn test_replace_none_keeps_existing_optional() { + // Passing None for an optional field keeps the existing value, not unsets it + let base = RequestFlags::new( + false, false, false, false, false, false, false, false, false, false, false, + Some(300), // cache_ttl set + None, None, None, None, None, None, None, None, + ); + let updated = replace_none(&base); + assert_eq!(push_to_vec(&updated), b" T300"); + } + + #[test] + fn test_replace_multiple_fields() { + let base = RequestFlags::new( + false, true, false, true, false, false, false, false, false, false, false, Some(60), + None, None, None, None, None, None, None, None, + ); + let updated = base.replace( + Some(true), // add no_reply + None, // keep return_client_flag=true + Some(true), // add return_cas_token + None, // keep return_value=true + None, + None, + None, + None, + None, + None, + None, + None, // keep cache_ttl=60 + Some(120), // add recache_ttl + None, + None, + None, + None, + None, + None, + None, + ); + assert_eq!(push_to_vec(&updated), b" q f c v T60 R120"); + } + + #[test] + fn test_replace_opaque() { + let base = default_flags(); + let updated = base.replace( + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + None, + Some(b"abc".to_vec()), + None, + ); + assert_eq!(push_to_vec(&updated), b" Oabc"); + // base is unchanged + assert_eq!(push_to_vec(&base), b""); + } } diff --git a/tests/test_memcache_socket.py b/tests/test_memcache_socket.py index 9c44bb4..bc9423c 100644 --- a/tests/test_memcache_socket.py +++ b/tests/test_memcache_socket.py @@ -1011,3 +1011,52 @@ def test_version_matches_intenum(self): """ServerVersion IntEnum values match Rust constants.""" assert SERVER_VERSION_AWS_1_6_6 == 1 assert SERVER_VERSION_STABLE == 2 + + +class TestRequestFlagsReplace: + def test_replace_no_args_returns_equal(self): + base = RequestFlags(return_value=True, cache_ttl=300) + assert base.replace() == base + + def test_replace_bool_flag(self): + base = RequestFlags(return_value=True) + updated = base.replace(return_cas_token=True) + assert updated.return_value is True + assert updated.return_cas_token is True + # base is unchanged + assert base.return_cas_token is False + + def test_replace_optional_field(self): + base = RequestFlags(return_value=True) + updated = base.replace(cache_ttl=600) + assert updated.cache_ttl == 600 + assert updated.return_value is True + # base is unchanged + assert base.cache_ttl is None + + def test_replace_none_keeps_existing_optional(self): + base = RequestFlags(cache_ttl=300) + # explicitly passing None keeps the existing value, not unsets it + updated = base.replace(cache_ttl=None) + assert updated.cache_ttl == 300 + + def test_replace_multiple_fields(self): + base = RequestFlags(return_client_flag=True, return_value=True, cache_ttl=60) + updated = base.replace(no_reply=True, return_cas_token=True, recache_ttl=120) + assert updated.no_reply is True + assert updated.return_client_flag is True # preserved + assert updated.return_cas_token is True + assert updated.return_value is True # preserved + assert updated.cache_ttl == 60 # preserved + assert updated.recache_ttl == 120 + + def test_replace_opaque(self): + base = RequestFlags(return_value=True) + updated = base.replace(opaque=b"abc") + assert updated.opaque == b"abc" + assert base.opaque is None + + def test_fields_are_readonly(self): + flags = RequestFlags(return_value=True) + with pytest.raises(AttributeError): + flags.return_value = False # type: ignore[misc]