Repository navigation
feat(cache): add core cache module - #265
cassiofariasmachado wants to merge 14 commits into
Conversation
|
Can we implement it in one module to exemplify usage? |
1727ebf to
be75f1e
Compare
Added to the HTTP Client Core module. |
cassiofariasmachado
left a comment
There was a problem hiding this comment.
PR Review: #265: feat(cache): add core cache module
Author: cassio-farias-machado Branch: feature/add-cache-core-module → main
Verdict:
Summary: Solid, well-structured new module; two documentation inaccuracies and a missing AI-disclosure must be fixed before merge.
A: Process & Compliance
| # | Criterion | Status | Finding |
|---|---|---|---|
| A1 | PR template complete | New module — "e2e evidence attached" checkbox not ticked and no screen recording or BTP logs referenced in the body or comments. | |
| A2 | Conventional Commits | ✅ | PR title feat(cache): add core cache module is valid. commit-validation CI job passes. |
| A3 | Issue linked | No Closes #N / Fixes #N in the PR body. Acceptable if there is no tracked issue. |
|
| A4 | AI-generated code disclosure | ❌ | The diff (large new module, consistent style, full test coverage) strongly suggests AI assistance. No disclosure referencing the SAP AI contribution guideline is present in the PR body. Required by CONTRIBUTING.md. |
B: Security & Sensitive Data
| # | Criterion | Status | Finding |
|---|---|---|---|
| B1 | No sensitive data in code | ✅ | No credentials, hardcoded tenant IDs, or internal URLs. |
| B2 | No sensitive data in PR body | ✅ | No account-specific data. |
C: Code Quality
| # | Criterion | Status | Finding |
|---|---|---|---|
| C1 | CI checks passing | All required jobs pass. SDK review label present fails — expected until this review applies the label. |
|
| C2 | Version bump | ➖ | No manual version change detected. |
| C3 | Type hints | ✅ | All public methods fully annotated. py.typed present. |
| C4 | No hardcoded values | ✅ | Module-level constants used (_TOKEN_NAMESPACE, _TOKEN_EXPIRY_BUFFER_SECONDS, etc.). |
| C5 | Import organization | ✅ | All top-level imports, PEP 8 order, no lazy imports. |
| C6 | Naming conventions | ✅ | Enum values SCREAMING_SNAKE_CASE, alphabetical order, private symbols prefixed _. |
| C7 | No unused code | ✅ | No dead code introduced. |
| C8 | No unjustified new dependencies | ✅ | cachetools is a minimal, established library. |
| C9 | Proto code freshness | ➖ | No .proto files changed. |
D: API & Design
| # | Criterion | Status | Finding |
|---|---|---|---|
| D1 | API future-proofing | ✅ | CacheConfig dataclass, IsolationStrategy enum, CacheBackend ABC. Clean design. |
| D2 | Public API hygiene | ✅ | Cache is correctly kept internal (not exported from __init__.py). Only CacheConfig, CacheBackend, InMemoryLRUBackend, IsolationStrategy, configure_cache, get_cache_config, and exception types are public. |
| D3 | Breaking changes marked | ➖ | No public API changed or removed. |
| D4 | Pagination & tenant filtering | ➖ | Not a list/query operation. |
| D5 | Telemetry instrumentation | Module.CACHE was added to core/telemetry/module.py (good), but no @record_metrics decorator is used anywhere in sap_cloud_sdk/cache/. For an infrastructure module that other SDK modules consume, this is acceptable — but the decision should be documented. Consider adding a comment in _cache.py explaining why metrics are not recorded here (e.g., "metrics are recorded by the consuming module, not the cache layer"). |
|
| D6 | Multi-tenancy support | ✅ | TENANT, TENANT_USER, PROVIDER isolation with automatic RuntimeContext reading. Tenant ID resolved from APP_TENANT_ID with DWC_TENANT fallback. |
E: Tests & Documentation
| # | Criterion | Status | Finding |
|---|---|---|---|
| E1 | Tests added/updated | ✅ | Comprehensive unit tests for Cache, InMemoryLRUBackend, _isolation, _config, and _backend. Test names follow test_<functionality>_<condition>_<expected_result> convention. XsuaaAuthProvider changes tested via existing test suite. |
| E2 | Documentation quality | ❌ | Two inaccuracies in user-guide.md (details below). |
| E2a | Multi-tenancy documentation | ✅ | ## Multi-tenancy section at line 149 contains all four required fields. |
| E3 | Module structure compliance | ✅ | __init__.py, exceptions.py, py.typed, user-guide.md all present. Structure matches the SDK convention. |
❌ Blocking Issues
-
[A4] No AI-generated code disclosure in PR body. Add a note referencing the SAP AI contribution guideline and tick the corresponding checkbox in the PR template.
-
[E2]
user-guide.mdhas two inaccuracies that will mislead users:- Line 41 —
on_evictconfiguration table listsreasonvalues as"lru"or"manual"only. The"ttl"reason is implemented in_lru_backend.py(fired for lazily-expired entries on access) and is documented correctly in the prose section at line 92 — but the table itself is wrong. Update the table cell to readreason is "lru","ttl", or"manual". - Line 121 — Integration example section references CSRF token handling in
HttpClient(csrf_enabled=True) as the real in-tree usage. That feature was removed fromclient.pyin this branch; the current in-tree integration isXsuaaAuthProvidertoken caching incore/protocol/http/models.py. Update this section to point toXsuaaAuthProviderinstead.
- Line 41 —
⚠️ Non-Blocking Suggestions
-
[A1] The "e2e evidence" checkbox is unchecked. If this module was manually tested against a real BTP service (even locally), attach logs or a brief test summary in the PR body or a comment. If e2e testing is deferred (e.g., pending a consuming module), note that explicitly.
-
[D5] Consider adding a short comment in
_cache.pynear theCacheclass explaining why@record_metricsis not used here — the intent (metrics owned by consuming modules) is non-obvious to future contributors.
✅ Things Done Well
CacheBackendABC has exactly four methods with no leaky abstractions — clear, minimal, and genuinely pluggable.InMemoryLRUBackendcorrectly implements lazy TTL eviction with the"ttl"evict reason, whichcachetools.TTLCachedoes not natively support.Cache._make_full_keyformat (namespace::isolation_key::key) is readable and debuggable.XsuaaAuthProvidercorrectly closes the staleOAuth2Sessionbefore overwriting (releases connection pool) — a subtle correctness detail that is easy to miss.- Config snapshot at
Cacheconstruction time prevents silent global config changes from affecting already-constructed clients. The behaviour is documented in the user guide. - Test coverage is thorough: isolation across tenants and users,
enabled=Falseno-ops, expiry buffer math, TTL clamping to 1, and backend exception swallowing all have dedicated cases. - The Redis deferral note in the Custom Backend section is clear and does not over-promise.
cassiofariasmachado
left a comment
There was a problem hiding this comment.
PR Review: #265: feat(cache): add core cache module
Author: cassio-farias-machado Branch: feature/add-cache-core-module → main
Verdict: ✅ Ready to Merge
Summary: All previously blocking issues resolved; documentation fixes and AI disclosure are in place.
A: Process & Compliance
| # | Criterion | Status | Finding |
|---|---|---|---|
| A1 | PR template complete | New module — "e2e evidence attached" checkbox not ticked and no BTP logs or screen recording referenced. Acceptable deferral for an infrastructure module with no direct BTP service dependency. | |
| A2 | Conventional Commits | ✅ | PR title feat(cache): add core cache module is valid. Commit Validation CI job passes. |
| A3 | Issue linked | No Closes #N in the PR body. Acceptable — no tracked issue exists. |
|
| A4 | AI-generated code disclosure | ✅ | ## AI-Generated Code Disclosure section added, SAP AI guideline linked, checkbox ticked. |
B: Security & Sensitive Data
| # | Criterion | Status | Finding |
|---|---|---|---|
| B1 | No sensitive data in code | ✅ | No credentials, hardcoded tenant IDs, or internal URLs. |
| B2 | No sensitive data in PR body | ✅ | No account-specific data. |
C: Code Quality
| # | Criterion | Status | Finding |
|---|---|---|---|
| C1 | CI checks passing | ✅ | All required jobs pass. Integration Tests failure is a timeout in agent_memory — unrelated to this PR and a known environmental flake. |
| C2 | Version bump | ➖ | No manual version change detected. |
| C3 | Type hints | ✅ | All public methods fully annotated. py.typed present. |
| C4 | No hardcoded values | ✅ | Module-level constants used (_TOKEN_NAMESPACE, _TOKEN_EXPIRY_BUFFER_SECONDS, etc.). |
| C5 | Import organization | ✅ | All top-level imports, PEP 8 order, no lazy imports. |
| C6 | Naming conventions | ✅ | Enum values SCREAMING_SNAKE_CASE, alphabetical order, private symbols prefixed _. |
| C7 | No unused code | ✅ | No dead code introduced. |
| C8 | No unjustified new dependencies | ✅ | cachetools is minimal, MIT-licensed, and already a transitive dependency. |
| C9 | Proto code freshness | ➖ | No .proto files changed. |
D: API & Design
| # | Criterion | Status | Finding |
|---|---|---|---|
| D1 | API future-proofing | ✅ | CacheConfig dataclass, IsolationStrategy enum, CacheBackend ABC. Clean design. |
| D2 | Public API hygiene | ✅ | Cache is correctly kept internal. Only CacheConfig, CacheBackend, InMemoryLRUBackend, IsolationStrategy, configure_cache, get_cache_config, and exception types are public. |
| D3 | Breaking changes marked | ➖ | No public API changed or removed. |
| D4 | Pagination & tenant filtering | ➖ | Not a list/query operation. |
| D5 | Telemetry instrumentation | Module.CACHE added to core/telemetry/module.py, but no @record_metrics used in the cache module itself. For an infrastructure layer this is intentional — metrics belong to consuming modules. The decision is non-obvious and worth a one-line comment in _cache.py, but not a blocker. |
|
| D6 | Multi-tenancy support | ✅ | TENANT, TENANT_USER, PROVIDER isolation with automatic RuntimeContext reading. Tenant ID resolved from APP_TENANT_ID with DWC_TENANT fallback. |
E: Tests & Documentation
| # | Criterion | Status | Finding |
|---|---|---|---|
| E1 | Tests added/updated | ✅ | Comprehensive unit tests for Cache, InMemoryLRUBackend, _isolation, _config, and _backend. Test names follow test_<functionality>_<condition>_<expected_result> convention. |
| E2 | Documentation quality | ✅ | user-guide.md fixes applied: on_evict table now lists "lru", "ttl", and "manual"; integration example updated to reference XsuaaAuthProvider. |
| E2a | Multi-tenancy documentation | ✅ | ## Multi-tenancy section present with all four required fields. |
| E3 | Module structure compliance | ✅ | __init__.py, exceptions.py, py.typed, user-guide.md all present. Structure matches the SDK convention. |
⚠️ Non-Blocking Suggestions
- [A1] Consider noting explicitly in the PR body that e2e coverage is deferred until a consuming module (e.g. destination) integrates the cache layer.
- [D5] A one-line comment in
_cache.pynear theCacheclass explaining that@record_metricsis intentionally absent (metrics owned by consuming modules) would help future contributors.
✅ Things Done Well
CacheBackendABC has exactly four methods with no leaky abstractions — clear, minimal, and genuinely pluggable.InMemoryLRUBackendcorrectly implements lazy TTL eviction with the"ttl"evict reason, whichcachetools.TTLCachedoes not natively support.Cache._make_full_keyformat (namespace::isolation_key::key) is readable and debuggable.XsuaaAuthProvidercorrectly closes the staleOAuth2Sessionbefore overwriting (releases connection pool) — a subtle correctness detail that is easy to miss.- Config snapshot at
Cacheconstruction time prevents silent global config changes from affecting already-constructed clients. The behaviour is documented in the user guide. - Test coverage is thorough: isolation across tenants and users,
enabled=Falseno-ops, expiry buffer math, TTL clamping to 1, and backend exception swallowing all have dedicated cases. - The Redis deferral note in the Custom Backend section is clear and does not over-promise.
- All three blocking issues from the previous review pass: AI disclosure added,
on_evicttable corrected, and integration example updated toXsuaaAuthProvider.
|
Should we add cache as part of a bigger package called resilience? Just thinking in future we will have at least retry also. |
Great point! We moved the cache module to |
…-module token collisions
Description
Adds
sap_cloud_sdk.core.resilience.cache— a domain-agnostic, pluggable cache layer shared across all SDK modules. The module lives undercore.resilienceto group it alongside future resilience primitives (e.g. retry). It is infrastructure-only: no BTP service binding, nocreate_client()factory.Core design
CacheBackendABC — four-method interface (get,set,delete,clear); backend-agnostic (no tenant or TTL awareness).InMemoryLRUBackend— default in-process backend backed bycachetools.TTLCache; thread-safe; per-entry TTL via monotonic clock; LRU eviction atmax_size;on_evictcallback with reasons"lru","ttl", and"manual".CacheConfig— dataclass coveringenabled,isolation_strategy,default_ttl_seconds,expiry_buffer_seconds,max_size,backend, andon_evict.configure_cache()/get_cache_config()— global registry set once at startup; per-client override viaCacheConfigconstructor argument.IsolationStrategy— three strategies:TENANT,TENANT_USER, andPROVIDER. Auto-selected fromRuntimeContext(user ID present →TENANT_USER; no user ID →TENANT). Keys use a readable prefixed format:APP_TENANT_ID(IAS JWT) with fallback toDWC_TENANT(DWC header). User ID is read fromUSER_ID(IAS JWT). Callers do not pass these explicitly.Cachefaçade (SDK-internal, not exported) — glues config, isolation, and backend; appliesexpiry_buffer_seconds; isolates backend exceptions.module=keyword onCache— optional name that shares a singleInMemoryLRUBackendacross allCacheinstances with the same name. Solves the per-request-client pattern (e.g. Agent Gateway) where a new client is created per request and would otherwise get a fresh, empty backend each time. Existing callers that omitmodule=are unaffected.First integration: token caching in
XsuaaAuthProvidercore/protocol/http/models.py::XsuaaAuthProvidernow uses theCachefaçade instead of a manualdict + Lockfor OAuth2 token caching. Tokens are cached withIsolationStrategy.TENANT— one token per tenant, keyed bytenant_subdomain. The cache is backed by the globalCacheConfig, TTL is derived from the tokenexpires_invalue minus a 60-second buffer, and the old session is closed before overwriting to release its connection pool.module="core.protocol.http"is set so the shared backend survives across per-request client instances.Related Issue
N/A
Type of Change
How to Test
uv sync --all-extras --group devAI-Generated Code Disclosure
Parts of this PR were developed with AI assistance (Claude). Reviewed and validated per the SAP AI contribution guideline.
Checklist
Breaking Changes
None — purely additive new module.
Additional Notes
cachetools~=5.5.2added to[project.dependencies](MIT licence, already a transitive dependency ofgoogle-auth).Module.CACHE = "cache"added tocore/telemetry/module.py(alphabetical order).Cachefaçade is intentionally not exported fromsap_cloud_sdk.core.resilience.cache; SDK modules import it viafrom sap_cloud_sdk.core.resilience.cache._cache import Cache.