LCORE-298: @connection decorator and no-op conversion cache #568

tisnik · 2025-09-22T06:25:23Z

Description

LCORE-298: @connection decorator and no-op conversion cache

Type of change

Related Tickets & Documents

Related Issue #LCORE-298

Summary by CodeRabbit

New Features
- Added a “noop” cache option in configuration, providing a no‑op, always‑ready cache with no data persistence.
- Ensures a concrete cache instance is always returned for supported cache types.
Tests
- Added comprehensive tests for the noop cache behavior and cache selection.
- Added tests for automatic connection handling.
- Updated validation tests to include “noop” in allowed cache types and to improve error messaging.

coderabbitai · 2025-09-22T06:25:31Z

Walkthrough

Adds a NoopCache backend, updates the cache factory to always return a Cache (including for “noop”), expands allowed cache types and messages, introduces a connection decorator for auto-connect behavior, and adds/updates unit tests across cache, config, and utils.

Changes

Cohort / File(s)	Summary
Cache Factory & NoopCache `src/cache/cache_factory.py`, `src/cache/noop_cache.py`	Factory now returns a concrete Cache for all known types, including new NoopCache; adds NoopCache class with inert operations and input validation; adjusts return type from Cache \| None to Cache.
Config & Constants `src/constants.py`, `src/models/config.py`	Adds CACHE_TYPE_NOOP constant; expands ConversationCacheConfiguration.type Literal to include "noop"; updates error messaging expectations elsewhere.
Connection Decorator `src/utils/connection_decorator.py`	Adds connection decorator to ensure connect() is called before decorated method execution.
Tests: Cache `tests/unit/cache/__init__.py`, `tests/unit/cache/test_cache_factory.py`, `tests/unit/cache/test_noop_cache.py`	Adds/updates tests for factory handling of noop and invalid types; comprehensive tests for NoopCache behaviors and validations; adds cache tests module docstring.
Tests: Models `tests/unit/models/config/test_conversation_cache.py`	Updates expected validation message to include “noop”.
Tests: Utils `tests/unit/utils/test_connection_decorator.py`	Adds tests validating auto-connect behavior and exception propagation for the connection decorator.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant App
  participant CacheFactory
  participant NoopCache

  App->>CacheFactory: conversation_cache(config.type = "noop")
  Note over CacheFactory: Validate cache type
  CacheFactory-->>App: instance of NoopCache

  App->>NoopCache: get/insert/delete/list
  Note over NoopCache: No-op behavior, input validation only
  NoopCache-->>App: Empty results / True as applicable

sequenceDiagram
  autonumber
  participant Caller
  participant Obj as DecoratedObject
  participant Backend as Connectable

  Caller->>Obj: some_action(...)
  alt Not connected
    Obj->>Backend: connected()
    Backend-->>Obj: False
    Obj->>Backend: connect()
  else Already connected
    Obj->>Backend: connected()
    Backend-->>Obj: True
  end
  Obj->>Obj: execute original method
  Obj-->>Caller: result or exception

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

LCORE-298: Conversation cache factory #567 — Also modifies CacheFactory.conversation_cache and related config/types, overlapping with the introduction and handling of the “noop” cache type.

Poem

I hop through caches, light and free,
A Noop burrow—nothing to see!
The factory hands me keys with flair,
I stash the wind, return the air.
Connect? I’m ready, always so—
Tests nod: “All green!” Now off I go! 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title accurately and concisely describes the primary changes: adding an @connection decorator and a no-op conversion cache, and it includes the issue key LCORE-298. The diff shows the new connection_decorator.py, noop_cache.py, cache factory/model updates, and corresponding tests, which align with the title. The phrasing is specific and clear enough for a teammate scanning PRs.
Docstring Coverage	✅ Passed	Docstring coverage is 97.62% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/models/config.py (1)

492-527: Add NOOP validation to forbid backend configs when type='noop'.

Currently, NOOP bypasses checks and allows conflicting backend configs.

Apply this diff:

         match self.type:
+            case constants.CACHE_TYPE_NOOP:
+                # NOOP must not specify any backend-specific configuration
+                if any([self.memory, self.sqlite, self.postgres]):
+                    raise ValueError("No backend configuration must be provided for NOOP cache")
             case constants.CACHE_TYPE_MEMORY:
                 if self.memory is None:
                     raise ValueError("Memory cache is selected, but not configured")
                 # no other DBs configuration allowed
                 if any([self.sqlite, self.postgres]):
                     raise ValueError("Only memory cache config must be provided")

🧹 Nitpick comments (19)

tests/unit/models/config/test_conversation_cache.py (2)
47-51: Fix mismatched subtest label.

Second “SQLite cache” label should be “PostgreSQL cache” for clarity.

Apply this diff:
-    with subtests.test(msg="SQLite cache"):
+    with subtests.test(msg="PostgreSQL cache"):
         with pytest.raises(
             ValidationError, match="PostgreSQL cache is selected, but not configured"
         ):
             _ = ConversationCacheConfiguration(type=constants.CACHE_TYPE_POSTGRES)
24-31: Add a NOOP‑type happy‑path test.

Please add a test asserting that type="noop" with no backend config validates, and that type="noop" with any backend config is rejected (after implementing the model validation below).
tests/unit/utils/test_connection_decorator.py (2)
15-23: Tighten test fixture: default False and clearer naming.

Initialize as False (not None) and rename the flag to reflect some_action.

Apply this diff:
-    def __init__(self, raise_exception_from_foo: bool):
+    def __init__(self, raise_exception: bool):
         """Initialize class used to test connection decorator."""
-        self._raise_exception_from_foo = raise_exception_from_foo
-        self._connected = None
+        self._raise_exception = raise_exception
+        self._connected = False
@@
-        if self._raise_exception_from_foo:
+        if self._raise_exception:
             raise SomeActionException("some_action error!")
And update call sites:
-    c = Connectable(raise_exception_from_foo=False)
+    c = Connectable(raise_exception=False)
@@
-    c = Connectable(raise_exception_from_foo=True)
+    c = Connectable(raise_exception=True)
56-59: Assert auto-connect occurs even on exceptions.

Strengthens the decorator guarantee.

Apply this diff:
 with pytest.raises(SomeActionException, match="some_action error!"):
     # this method should autoconnect
     c.some_action()
+assert c.connected() is True
src/cache/cache_factory.py (1)
21-22: Update Returns doc to include NOOP.

Docstring still mentions only SQLite/Postgres/InMemory.

Apply this diff:
-        Returns:
-            An instance of `Cache` (either `SQLiteCache`, `PostgresCache` or `InMemoryCache`).
+        Returns:
+            An instance of `Cache` (`NoopCache`, `InMemoryCache`, `SQLiteCache`, or `PostgresCache`).
src/cache/noop_cache.py (3)
5-9: Align logging with coding guidelines.

Use logger = logging.getLogger(name) instead of a custom getter.

Apply this diff:
-from log import get_logger
+import logging
@@
-logger = get_logger("cache.noop_cache")
+logger = logging.getLogger(__name__)
14-16: Remove empty initializer.

Not needed; the base class requires none.

Apply this diff:
-    def __init__(self) -> None:
-        """Create a new instance of no-op cache."""
-
+    # No initialization required for NoopCache
28-45: Minor: call static validators via class/instance, not super().

These are static methods; self.construct_key(...) is clearer.

Apply this diff:
-        super().construct_key(user_id, conversation_id, skip_user_id_check)
+        self.construct_key(user_id, conversation_id, skip_user_id_check)
Repeat similarly in delete().
tests/unit/cache/test_cache_factory.py (3)
11-15: Simplify fixture naming (remove alias indirection).

The alias via name= creates unnecessary indirection. Prefer naming the function the same as the fixture.

Apply:
-@pytest.fixture(scope="module", name="noop_cache_config_fixture")
-def noop_cache_config():
+@pytest.fixture(scope="module")
+def noop_cache_config_fixture():
     """Fixture containing initialized instance of ConversationCacheConfiguration."""
     return ConversationCacheConfiguration(type=CACHE_TYPE_NOOP)
17-23: Construct invalid config without post-init mutation.

Mutating a Pydantic model after creation can be brittle if validate_assignment is enabled. Build an invalid instance explicitly without validation.

Apply:
-@pytest.fixture(scope="module", name="invalid_cache_type_config_fixture")
-def invalid_cache_type_config():
+@pytest.fixture(scope="module")
+def invalid_cache_type_config_fixture():
     """Fixture containing instance of ConversationCacheConfiguration with improper settings."""
-    c = ConversationCacheConfiguration()
-    c.type = "foo bar baz"
-    return c
+    # Bypass validation intentionally to feed invalid type to the factory
+    return ConversationCacheConfiguration.model_construct(type="foo bar baz")
25-31: Consider adding a None-type test.

Factory behavior when config.type is None isn’t covered.

Example:
+def test_conversation_cache_none_type():
+    cfg = ConversationCacheConfiguration()
+    with pytest.raises(ValueError, match="Invalid cache type"):
+        CacheFactory.conversation_cache(cfg)
tests/unit/cache/test_noop_cache.py (8)
21-26: Fix typo in fixture docstring.

“constucted” → “constructed”.

Apply:
-    """Fixture with constucted and initialized in memory cache object."""
+    """Fixture with constructed and initialized no-op cache object."""
28-31: Optionally assert connectivity contract.

Since connect() is a no-op with logging, also assert connected() is True to document the contract.

Apply:
 def test_connect(cache_fixture):
     """Test the behavior of connect method."""
     cache_fixture.connect()
+    assert cache_fixture.connected() is True
64-70: Remove stale comment referencing DEFAULT_USER_UID.

The test doesn’t rely on any DEFAULT_USER_UID; the comment can mislead future readers.

Apply:
-    # this UUID is different from DEFAULT_USER_UID
     assert (
         cache_fixture.get("ffffffff-ffff-ffff-ffff-ffffffffffff", CONVERSATION_ID) == []
     )
146-149: Clarify docstring (this is NoopCache, not in-memory cache).

Tighten wording.

Apply:
-    """Test if in memory cache always report ready."""
+    """Test that NoopCache always reports ready."""
151-161: Fix typos in comments and prefer precise wording.

“chararacter” → “character”.

Apply:
-    "ffffffff-ffff-ffff-ffff-fffffffffff",  # UUID-like string with missing chararacter
+    "ffffffff-ffff-ffff-ffff-fffffffffff",  # UUID-like string with one missing character
33-48: Reduce repetition via parametrization for skip_user_id_check.

The two insert_or_append tests differ only by skip_user_id_check. Parametrize instead.

Example:
-def test_insert_or_append(cache_fixture):
-    """Test the behavior of insert_or_append method."""
-    cache_fixture.insert_or_append(
-        USER_ID,
-        CONVERSATION_ID,
-        cache_entry_1,
-    )
-
-
-def test_insert_or_append_skip_user_id_check(cache_fixture):
-    """Test the behavior of insert_or_append method."""
-    skip_user_id_check = True
-    cache_fixture.insert_or_append(
-        USER_PROVIDED_USER_ID, CONVERSATION_ID, cache_entry_1, skip_user_id_check
-    )
+@pytest.mark.parametrize(
+    "user_id,skip",
+    [(USER_ID, False), (USER_PROVIDED_USER_ID, True)],
+)
+def test_insert_or_append(cache_fixture, user_id, skip):
+    """insert_or_append should accept both validated and user-provided IDs."""
+    cache_fixture.insert_or_append(user_id, CONVERSATION_ID, cache_entry_1, skip)
185-189: Add missing negative test for insert_or_append with invalid conversation ID.

We cover get/delete invalid IDs; add insert_or_append parity.

Append:
+def test_insert_or_append_improper_conversation_id(cache_fixture):
+    """insert_or_append should validate conversation ID."""
+    with pytest.raises(ValueError, match="Invalid conversation ID"):
+        cache_fixture.insert_or_append(USER_ID, "not-a-uuid", cache_entry_1)
20-26: Consider function-scope fixture explicitly.

Default is function scope; make it explicit for readability since state isolation matters for cache tests.

Apply:
-@pytest.fixture(name="cache_fixture")
+@pytest.fixture(scope="function", name="cache_fixture")
 def cache():

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1aa6803 and f5852a3.

📒 Files selected for processing (10)

src/cache/cache_factory.py (2 hunks)
src/cache/noop_cache.py (1 hunks)
src/constants.py (1 hunks)
src/models/config.py (1 hunks)
src/utils/connection_decorator.py (1 hunks)
tests/unit/cache/__init__.py (1 hunks)
tests/unit/cache/test_cache_factory.py (1 hunks)
tests/unit/cache/test_noop_cache.py (1 hunks)
tests/unit/models/config/test_conversation_cache.py (1 hunks)
tests/unit/utils/test_connection_decorator.py (1 hunks)

🧰 Additional context used

📓 Path-based instructions (9)

src/**/*.py