LCORE-298: stub for cache implementations #571
Conversation
WalkthroughAdds three concrete cache backends (InMemoryCache, SQLiteCache, PostgresCache), wires them into cache_factory with per-backend config validation and explicit error handling, and adds unit tests covering factory resolution and improper sub-config scenarios. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant CacheFactory
participant Cache as Backend
Note over Caller,CacheFactory: Request cache instance
Caller->>CacheFactory: build_cache(config)
alt type in [MEMORY, SQLITE, POSTGRES] and sub-config present
CacheFactory-->>Caller: new Backend(config.sub)
else type is None
CacheFactory-->>Caller: ValueError("Cache type must be set")
else missing sub-config
CacheFactory-->>Caller: ValueError("Missing <backend> configuration")
end
Note over Caller,Cache: Use returned scaffolded backend
Caller->>Cache: connect()/initialize_cache()
Cache-->>Caller: ready=True
Caller->>Cache: get/insert_or_append/delete/list(...)
Note right of Cache: @connection -> key validation -> no-op results
Cache-->>Caller: empty list / True / None
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
src/cache/in_memory_cache.py (3)
9-9: Prefer module-qualified logger nameUse name for logger naming to match typical module scoping and ease filtering.
Apply this diff:
-logger = get_logger("cache.in_memory_cache") +logger = get_logger(__name__)
12-26: Annotate class attributes and track connection stateAdd explicit attribute types (per guidelines) and a simple connected state so the connection decorator meaningfully triggers connect once.
Apply this diff:
class InMemoryCache(Cache): """In-memory cache implementation.""" + cache_config: InMemoryCacheConfig + _connected: bool + def __init__(self, config: InMemoryCacheConfig) -> None: """Create a new instance of in-memory cache.""" self.cache_config = config + self._connected = False def connect(self) -> None: """Initialize connection to database.""" - logger.info("Connecting to storage") + logger.info("Connecting to in-memory storage") + self._connected = True def connected(self) -> bool: """Check if connection to cache is alive.""" - return True + return self._connected
30-46: Validate keys using the class, not super() (optional)Calling a @staticmethod via the class improves clarity over super() for static resolution.
Apply this diff:
- super().construct_key(user_id, conversation_id, skip_user_id_check) + Cache.construct_key(user_id, conversation_id, skip_user_id_check)src/cache/cache_factory.py (2)
45-46: Include NOOP in the invalid type error messageError guidance should list all supported types, including NOOP.
Apply this diff:
- f"Use '{constants.CACHE_TYPE_POSTGRES}' '{constants.CACHE_TYPE_SQLITE}' or " - f"'{constants.CACHE_TYPE_MEMORY}' options." + f"Use '{constants.CACHE_TYPE_POSTGRES}', '{constants.CACHE_TYPE_SQLITE}', " + f"'{constants.CACHE_TYPE_MEMORY}', or '{constants.CACHE_TYPE_NOOP}' options." )
27-33: Handle None type explicitly for clearer errorsIf config.type is None, raise a dedicated error rather than falling into the default case.
Apply this diff:
match config.type: case constants.CACHE_TYPE_NOOP: return NoopCache() + case None: + raise ValueError("Cache type must be set") case constants.CACHE_TYPE_MEMORY: if config.memory is not None: return InMemoryCache(config.memory) raise ValueError("Expecting configuration for in-memory cache")Also applies to: 35-41
src/cache/postgres_cache.py (3)
9-9: Prefer module-qualified logger nameUse name to bind logger to the module path.
Apply this diff:
-logger = get_logger("cache.postgres_cache") +logger = get_logger(__name__)
12-26: Annotate attributes and maintain connection lifecycle stateAdd typed attributes and flip connected state in connect/connected.
Apply this diff:
class PostgresCache(Cache): """PostgreSQL cache implementation.""" + postgres_config: PostgreSQLDatabaseConfiguration + _connected: bool + def __init__(self, config: PostgreSQLDatabaseConfiguration) -> None: """Create a new instance of PostgreSQL cache.""" self.postgres_config = config + self._connected = False def connect(self) -> None: """Initialize connection to database.""" - logger.info("Connecting to storage") + logger.info("Connecting to PostgreSQL storage") + self._connected = True def connected(self) -> bool: """Check if connection to cache is alive.""" - return True + return self._connected
44-46: Static call for staticmethod (optional)Prefer class-qualified static invocation for readability.
Apply this diff:
- super().construct_key(user_id, conversation_id, skip_user_id_check) + Cache.construct_key(user_id, conversation_id, skip_user_id_check)src/cache/sqlite_cache.py (3)
9-9: Prefer module-qualified logger nameUse name for consistent logger hierarchy.
Apply this diff:
-logger = get_logger("cache.sqlite_cache") +logger = get_logger(__name__)
12-26: Annotate attributes and track connection stateType class attributes and reflect connection state in methods.
Apply this diff:
class SQLiteCache(Cache): """Cache that uses SQLite to store cached values.""" + sqlite_config: SQLiteDatabaseConfiguration + _connected: bool + def __init__(self, config: SQLiteDatabaseConfiguration) -> None: """Create a new instance of SQLite cache.""" self.sqlite_config = config + self._connected = False def connect(self) -> None: """Initialize connection to database.""" - logger.info("Connecting to storage") + logger.info("Connecting to SQLite storage") + self._connected = True def connected(self) -> bool: """Check if connection to cache is alive.""" - return True + return self._connected
44-46: Static call for staticmethod (optional)Prefer class-qualified static invocation for readability.
Apply this diff:
- super().construct_key(user_id, conversation_id, skip_user_id_check) + Cache.construct_key(user_id, conversation_id, skip_user_id_check)tests/unit/cache/test_cache_factory.py (1)
75-119: Optional: parametrize backend success tests to reduce repetitionYou can parametrize the three “proper configuration” tests into one to DRY the assertions.
Example:
import typing as t @pytest.mark.parametrize( ("cfg", "expected_type"), [ (ConversationCacheConfiguration(type=CACHE_TYPE_MEMORY, memory=InMemoryCacheConfig(max_entries=10)), InMemoryCache), (ConversationCacheConfiguration(type=CACHE_TYPE_SQLITE, sqlite=SQLiteDatabaseConfiguration(db_path="foo")), SQLiteCache), (ConversationCacheConfiguration(type=CACHE_TYPE_POSTGRES, postgres=PostgreSQLDatabaseConfiguration(db="db", user="u", password="p")) , PostgresCache), ], ) def test_conversation_cache_backends(cfg: ConversationCacheConfiguration, expected_type: t.Type[Cache]): cache = CacheFactory.conversation_cache(cfg) assert isinstance(cache, expected_type)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/cache/cache_factory.py(2 hunks)src/cache/in_memory_cache.py(1 hunks)src/cache/postgres_cache.py(1 hunks)src/cache/sqlite_cache.py(1 hunks)tests/unit/cache/test_cache_factory.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)
Files:
src/cache/cache_factory.pysrc/cache/postgres_cache.pysrc/cache/sqlite_cache.pysrc/cache/in_memory_cache.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
src/cache/cache_factory.pysrc/cache/postgres_cache.pysrc/cache/sqlite_cache.pytests/unit/cache/test_cache_factory.pysrc/cache/in_memory_cache.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/cache/test_cache_factory.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/unit/cache/test_cache_factory.py
🧬 Code graph analysis (5)
src/cache/cache_factory.py (4)
src/cache/in_memory_cache.py (1)
InMemoryCache(12-108)src/cache/postgres_cache.py (1)
PostgresCache(12-108)src/cache/sqlite_cache.py (1)
SQLiteCache(12-108)src/models/config.py (1)
config(138-144)
src/cache/postgres_cache.py (6)
src/cache/cache.py (3)
Cache(9-110)construct_key(37-43)_check_user_id(23-28)src/models/cache_entry.py (1)
CacheEntry(6-19)src/models/config.py (2)
config(138-144)PostgreSQLDatabaseConfiguration(85-103)src/log.py (1)
get_logger(7-13)src/utils/connection_decorator.py (1)
connection(6-22)src/cache/in_memory_cache.py (6)
connect(19-21)connected(23-25)get(31-46)insert_or_append(49-66)delete(69-85)ready(102-108)
src/cache/sqlite_cache.py (5)
src/cache/cache.py (3)
Cache(9-110)construct_key(37-43)_check_user_id(23-28)src/models/cache_entry.py (1)
CacheEntry(6-19)src/models/config.py (2)
config(138-144)SQLiteDatabaseConfiguration(73-76)src/log.py (1)
get_logger(7-13)src/utils/connection_decorator.py (1)
connection(6-22)
tests/unit/cache/test_cache_factory.py (2)
src/models/config.py (5)
config(138-144)ConversationCacheConfiguration(489-527)InMemoryCacheConfig(79-82)SQLiteDatabaseConfiguration(73-76)PostgreSQLDatabaseConfiguration(85-103)src/cache/cache_factory.py (1)
CacheFactory(16-47)
src/cache/in_memory_cache.py (5)
src/cache/cache.py (3)
Cache(9-110)construct_key(37-43)_check_user_id(23-28)src/models/cache_entry.py (1)
CacheEntry(6-19)src/models/config.py (2)
config(138-144)InMemoryCacheConfig(79-82)src/log.py (1)
get_logger(7-13)src/utils/connection_decorator.py (1)
connection(6-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (1)
tests/unit/cache/test_cache_factory.py (1)
121-133: Add coverage for None type and error messagingConsider a test for config.type is None to assert the factory’s behavior (either explicit error or default). Also verify the invalid-type error lists all supported types including NOOP after the factory change.
Suggested new tests:
def test_conversation_cache_type_none(): cc = ConversationCacheConfiguration() with pytest.raises(ValueError, match="Cache type must be set"): _ = CacheFactory.conversation_cache(cc) def test_conversation_cache_invalid_type_message(): cc = ConversationCacheConfiguration() cc.type = "invalid" with pytest.raises(ValueError) as ei: _ = CacheFactory.conversation_cache(cc) assert "postgres" in str(ei.value).lower() assert "sqlite" in str(ei.value).lower() assert "memory" in str(ei.value).lower() assert "noop" in str(ei.value).lower()
tisnik
force-pushed
the
lcore-298-stub-for-cache-implementations
branch
from
38502c8 to
f536842
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/unit/cache/test_cache_factory.py (4)
103-103: Fix incorrect docstring (SQLite vs memory)Update the docstring to reflect SQLite.
- """Check if memory cache configuration is checked in cache factory.""" + """Check if SQLite cache configuration is checked in cache factory."""
59-64: Make invalid-cache-type fixture resilient to assignment validationConstruct the invalid instance directly instead of mutating after creation.
- c = ConversationCacheConfiguration() - c.type = "foo bar baz" - return c + return ConversationCacheConfiguration.model_construct(type="foo bar baz")
75-81: Optional: reduce duplication via parametrizationConsider parametrizing happy-path tests with lazy_fixture to DRY up repeated assertions.
import pytest @pytest.mark.parametrize( "cfg, expected_cls", [ (pytest.lazy_fixture("memory_cache_config_fixture"), InMemoryCache), (pytest.lazy_fixture("sqlite_cache_config_fixture"), SQLiteCache), (pytest.lazy_fixture("postgres_cache_config_fixture"), PostgresCache), (pytest.lazy_fixture("noop_cache_config_fixture"), NoopCache), ], ) def test_conversation_cache_happy_paths(cfg, expected_cls): cache = CacheFactory.conversation_cache(cfg) assert cache is not None assert isinstance(cache, expected_cls)Also applies to: 94-100, 113-119
85-90: Avoid mutating Pydantic models in tests — use model_construct to build invalid configsConfigurationBase sets extra="forbid" and does not enable validate_assignment; direct attribute mutation is fragile if assignment validation or freezing is enabled later — construct invalid instances with model_construct.
@@ - cc = ConversationCacheConfiguration( - type=CACHE_TYPE_MEMORY, memory=InMemoryCacheConfig(max_entries=10) - ) - # simulate improper configuration (can not be done directly as model checks this) - cc.memory = None + # construct invalid config without validation to exercise factory guard + cc = ConversationCacheConfiguration.model_construct( + type=CACHE_TYPE_MEMORY, + memory=None, + ) @@ - cc = ConversationCacheConfiguration( - type=CACHE_TYPE_SQLITE, sqlite=SQLiteDatabaseConfiguration(db_path="foo") - ) - # simulate improper configuration (can not be done directly as model checks this) - cc.sqlite = None + # construct invalid config without validation to exercise factory guard + cc = ConversationCacheConfiguration.model_construct( + type=CACHE_TYPE_SQLITE, + sqlite=None, + ) @@ - cc = ConversationCacheConfiguration( - type=CACHE_TYPE_POSTGRES, - postgres=PostgreSQLDatabaseConfiguration(db="db", user="u", password="p"), - ) - # simulate improper configuration (can not be done directly as model checks this) - cc.postgres = None + # construct invalid config without validation to exercise factory guard + cc = ConversationCacheConfiguration.model_construct( + type=CACHE_TYPE_POSTGRES, + postgres=None, + ) @@ - cc = ConversationCacheConfiguration(type=CACHE_TYPE_NOOP) - # simulate improper configuration (can not be done directly as model checks this) - cc.type = None + # construct invalid config with no type set + cc = ConversationCacheConfiguration.model_construct(type=None)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cache/cache_factory.py(2 hunks)tests/unit/cache/test_cache_factory.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cache/cache_factory.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All modules start with descriptive module-level docstrings explaining purpose
Use logger = logging.getLogger(name) for module logging after import logging
Define type aliases at module level for clarity
All functions require docstrings with brief descriptions
Provide complete type annotations for all function parameters and return types
Use typing_extensions.Self in model validators where appropriate
Use modern union syntax (str | int) and Optional[T] or T | None consistently
Function names use snake_case with descriptive, action-oriented prefixes (get_, validate_, check_)
Avoid in-place parameter modification; return new data structures instead of mutating arguments
Use appropriate logging levels: debug, info, warning, error with clear messages
All classes require descriptive docstrings explaining purpose
Class names use PascalCase with conventional suffixes (Configuration, Error/Exception, Resolver, Interface)
Abstract base classes should use abc.ABC and @AbstractMethod for interfaces
Provide complete type annotations for all class attributes
Follow Google Python docstring style for modules, classes, and functions, including Args, Returns, Raises, Attributes sections as needed
Files:
tests/unit/cache/test_cache_factory.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests
Do not use unittest in tests; pytest is the standard
Files:
tests/unit/cache/test_cache_factory.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest-mock to create AsyncMock objects for async interactions in tests
Use the shared auth mock constant: MOCK_AUTH = ("mock_user_id", "mock_username", False, "mock_token") in tests
Files:
tests/unit/cache/test_cache_factory.py
🧬 Code graph analysis (1)
tests/unit/cache/test_cache_factory.py (5)
src/models/config.py (5)
config(138-144)ConversationCacheConfiguration(489-527)InMemoryCacheConfig(79-82)SQLiteDatabaseConfiguration(73-76)PostgreSQLDatabaseConfiguration(85-103)src/cache/cache_factory.py (2)
CacheFactory(16-49)conversation_cache(20-49)src/cache/in_memory_cache.py (1)
InMemoryCache(12-108)src/cache/sqlite_cache.py (1)
SQLiteCache(12-108)src/cache/postgres_cache.py (1)
PostgresCache(12-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (1)
tests/unit/cache/test_cache_factory.py (1)
75-81: Good coverage for factory happy pathsHappy-path tests for memory, SQLite, and Postgres backends look correct and align with CacheFactory behavior.
Also applies to: 94-100, 113-119
1 participant
Description
LCORE-298: stub for cache implementations
Type of change
Related Tickets & Documents
Summary by CodeRabbit
New Features
Bug Fixes
Tests