LCORE-892: updated conversation cache loading logic + added missing tests for conversation cache settings

[tisnik]

Description

LCORE-892: updated conversation cache loading logic + added missing tests for conversation cache settings

Type of change

• Refactor
• New feature
• Bug fix
• CVE fix
• Optimization
• Documentation Update
• Configuration Update
• Bump-up service version
• Bump-up dependent library
• Bump-up library or tool used for development (does not change the final image)
• CI configuration change
• Konflux configuration change
• Unit tests improvement
• Integration tests improvement
• End to end tests improvement

Related Tickets & Documents

• Related Issue #LCORE-892

Summary by CodeRabbit

• Bug Fixes

  • Accessing the conversation cache now errors clearly if the app configuration hasn't been loaded, preventing silent failures.

• New Features

  • Conversation cache always provides a usable cache instance and supports configurable storage backends (SQLite and in-memory) via configuration.

• Tests

  • Added tests validating cache backend selection and the new error behavior when configuration is missing.

[coderabbitai]

Walkthrough

Reworked the conversation_cache accessor to always return a Cache or raise a LogicError if configuration is not loaded. Added unit tests that load YAML specifying conversation_cache as sqlite or memory and assert the runtime cache instance type.

Changes

Cohort / File(s)	Summary
Configuration change src/configuration.py	conversation_cache signature changed to return Cache (no longer optional). Added precondition: if self._configuration is None raise LogicError. Retained lazy init of _conversation_cache when configuration is present.
Unit tests for cache types tests/unit/test_configuration.py	Added tests importing SQLiteCache and InMemoryCache. Added tests loading YAML with conversation_cache set to sqlite and memory, asserting config fields and that returned runtime cache instances are SQLiteCache or InMemoryCache. Added check that accessing cfg.conversation_cache raises the same logic error when configuration is not loaded.

Sequence Diagram(s)

sequenceDiagram
    participant C as Caller
    participant AppCfg as AppConfig
    participant Conf as _configuration
    participant Cache as _conversation_cache

    rect rgb(235,245,255)
    C->>AppCfg: access conversation_cache()
    AppCfg->>Conf: is _configuration loaded?
    alt not loaded
        AppCfg-->>C: raise LogicError
    else loaded
        AppCfg->>Cache: if exists return
        alt cache exists
            Cache-->>AppCfg: Cache instance
            AppCfg-->>C: return Cache
        else create
            AppCfg->>Conf: read conversation_cache config
            AppCfg->>Cache: instantiate Cache (SQLiteCache/InMemoryCache)
            Cache-->>AppCfg: new Cache instance
            AppCfg-->>C: return Cache
        end
    end
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Inspect the new precondition raising path in src/configuration.py.
• Verify lazy initialization and thread-safety (if applicable) of _conversation_cache.
• Validate unit tests correctly assert YAML-to-runtime-type mapping and error path.

Possibly related PRs

• LCORE-298: Conversation cache factory #567 — Modifies AppConfig's conversation cache accessor and related cache initialization patterns (strongly related).
• LCORE-298: SQLite conversation cache #576 — Implements the SQLite-backed Cache backend that the new tests expect to be instantiated.
• LCORE-298: configuration for conversation cache #563 — Adds the ConversationCache model / proxy used by the configuration accessor.

Poem

🐰 I hopped through config, nose to the ground,
Found caches that once hid without sound.
Now if config's missing, I loudly declare—
"LogicError! Load me first, if you dare!"
SQLite or memory, both ready to bound. 🥕

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 pull request title "LCORE-892: updated conversation cache loading logic + added missing tests for conversation cache settings" directly corresponds to the two primary changes in the changeset. The first part accurately reflects the modifications to src/configuration.py where the conversation_cache method signature changed and its loading logic was updated with stricter preconditions. The second part correctly describes the new unit tests added to tests/unit/test_configuration.py for conversation cache settings. The title is concise, specific, and clearly communicates the main objectives without being vague or misleading.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

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

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3f6a806 and 23465bb.

📒 Files selected for processing (2)

• src/configuration.py (1 hunks)
• tests/unit/test_configuration.py (3 hunks)

🧰 Additional context used

📓 Path-based instructions (6)

src/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)

Files:

• src/configuration.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/configuration.py
• tests/unit/test_configuration.py

src/{models/config.py,configuration.py}

📄 CodeRabbit inference engine (CLAUDE.md)

src/{models/config.py,configuration.py}: All configuration uses Pydantic models extending ConfigurationBase
Configuration base models must set model_config with extra="forbid" to reject unknown fields

Files:

• src/configuration.py

src/{models/**/*.py,configuration.py}

📄 CodeRabbit inference engine (CLAUDE.md)

src/{models/**/*.py,configuration.py}: Use @field_validator and @model_validator for custom validation in Pydantic models
Use precise type hints in configuration (e.g., Optional[FilePath], PositiveInt, SecretStr)

Files:

• src/configuration.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/test_configuration.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/test_configuration.py

🧬 Code graph analysis (2)

src/configuration.py (2)

src/cache/cache_factory.py (1)

• conversation_cache (20-49)

src/cache/cache.py (1)

• Cache (10-129)

tests/unit/test_configuration.py (3)

src/cache/sqlite_cache.py (1)

• SQLiteCache (19-414)

src/cache/in_memory_cache.py (1)

• InMemoryCache (13-130)

src/configuration.py (4)

• conversation_cache (147-155)
• AppConfig (36-155)
• load_configuration (52-58)
• conversation_cache_configuration (133-137)

⏰ 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). (3)

• GitHub Check: build-pr
• GitHub Check: e2e_tests (ci)
• GitHub Check: e2e_tests (azure)

🔇 Additional comments (6)

tests/unit/test_configuration.py (4)

9-10: LGTM: Imports support new cache type verification tests.

The imports for SQLiteCache and InMemoryCache are necessary for the isinstance checks in the new test functions, ensuring the correct cache implementation is instantiated based on configuration.

---

81-83: LGTM: Consistent fail-fast validation for conversation_cache property.

The test correctly verifies that accessing conversation_cache before loading configuration raises a LogicError, maintaining consistency with the other configuration properties tested above.

---

602-638: LGTM: Comprehensive SQLite cache configuration test.

The test thoroughly validates:

• YAML parsing of SQLite conversation cache configuration
• Correct configuration field population and mutual exclusivity (sqlite set, postgres/memory None)
• Runtime instantiation of SQLiteCache via the factory

The use of :memory: for the database path is appropriate for a unit test, avoiding filesystem dependencies.

---

641-677: LGTM: Comprehensive in-memory cache configuration test.

The test thoroughly validates:

• YAML parsing of in-memory conversation cache configuration
• Correct configuration field population and mutual exclusivity (memory set, sqlite/postgres None)
• Runtime instantiation of InMemoryCache via the factory

The test structure mirrors test_configuration_with_sqlite_conversation_cache, providing consistent coverage across cache implementations.

src/configuration.py (2)

147-147: LGTM: Return type annotation correctly updated to Cache.

The return type annotation has been corrected from Cache | None to Cache, accurately reflecting that this method either returns a Cache instance or raises a LogicError. This addresses the previous review feedback and improves type safety.

---

149-155: LGTM: Fail-fast behavior with lazy initialization.

The implementation correctly:

• Validates configuration is loaded before accessing the cache (lines 149-150)
• Lazily initializes _conversation_cache only once (lines 151-154)
• Returns a Cache instance that CacheFactory.conversation_cache guarantees to provide

The fail-fast pattern is consistent with other configuration properties in this class.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

tests/unit/test_configuration.py (2)

602-638: Improve docstring specificity.

The test correctly validates SQLite conversation cache initialization from YAML configuration and verifies the runtime instance type. However, the docstring is generic and doesn't indicate this test specifically covers SQLite cache configuration.

Apply this diff to improve the docstring:

-    """Test loading configuration from YAML file with conversation cache configuration."""
+    """Test loading configuration from YAML file with SQLite conversation cache."""

---

641-677: Improve docstring specificity and consider reducing duplication.

The test correctly validates in-memory conversation cache initialization and instance type verification. The docstring should be more specific, and there's significant YAML duplication with the SQLite test above.

Apply this diff to improve the docstring:

-    """Test loading configuration from YAML file with conversation cache configuration."""
+    """Test loading configuration from YAML file with in-memory conversation cache."""

Optionally, consider using pytest.mark.parametrize to reduce duplication between this test and test_configuration_with_sqlite_conversation_cache, though the current approach is acceptable for test clarity.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9906579 and 3f6a806.

📒 Files selected for processing (2)

• src/configuration.py (1 hunks)
• tests/unit/test_configuration.py (3 hunks)

🧰 Additional context used

📓 Path-based instructions (6)

src/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use absolute imports for internal modules (e.g., from auth import get_auth_dependency)

Files:

• src/configuration.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/configuration.py
• tests/unit/test_configuration.py

src/{models/config.py,configuration.py}

📄 CodeRabbit inference engine (CLAUDE.md)

src/{models/config.py,configuration.py}: All configuration uses Pydantic models extending ConfigurationBase
Configuration base models must set model_config with extra="forbid" to reject unknown fields

Files:

• src/configuration.py

src/{models/**/*.py,configuration.py}

📄 CodeRabbit inference engine (CLAUDE.md)

src/{models/**/*.py,configuration.py}: Use @field_validator and @model_validator for custom validation in Pydantic models
Use precise type hints in configuration (e.g., Optional[FilePath], PositiveInt, SecretStr)

Files:

• src/configuration.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/test_configuration.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/test_configuration.py

🧬 Code graph analysis (1)

tests/unit/test_configuration.py (3)

src/cache/sqlite_cache.py (1)

• SQLiteCache (19-414)

src/cache/in_memory_cache.py (1)

• InMemoryCache (13-130)

src/configuration.py (1)

• conversation_cache (147-155)

🔇 Additional comments (2)

tests/unit/test_configuration.py (2)

9-10: LGTM!

Necessary imports for the new test assertions verifying cache instance types.

---

81-83: Good addition for comprehensive coverage.

This test correctly verifies the new fail-fast behavior when accessing conversation_cache before configuration is loaded, ensuring consistency with other configuration properties.