Skip to content

feat(cache): add core cache module - #265

Open
cassiofariasmachado wants to merge 13 commits into
mainfrom
feature/add-cache-core-module
Open

cassiofariasmachado wants to merge 13 commits into
mainfrom
feature/add-cache-core-module

Conversation

@cassiofariasmachado

@cassiofariasmachado cassiofariasmachado commented Aug 10, 2026 •

Copy link
Copy Markdown
Member

Disclaimer: Do not include SAP-internal or customer-specific information in this PR (e.g. internal system URLs, customer names, tenant IDs, or confidential configurations). This is a public repository.

Description

Adds sap_cloud_sdk.core.resilience.cache — a domain-agnostic, pluggable cache layer shared across all SDK modules. The module lives under core.resilience to group it alongside future resilience primitives (e.g. retry). It is infrastructure-only: no BTP service binding, no create_client() factory.

Core design

  • CacheBackend ABC — four-method interface (get, set, delete, clear); backend-agnostic (no tenant or TTL awareness).
  • InMemoryLRUBackend — default in-process backend backed by cachetools.TTLCache; thread-safe; per-entry TTL via monotonic clock; LRU eviction at max_size; on_evict callback with reasons "lru", "ttl", and "manual".
  • CacheConfig — dataclass covering enabled, isolation_strategy, default_ttl_seconds, expiry_buffer_seconds, max_size, backend, and on_evict.
  • configure_cache() / get_cache_config() — global registry set once at startup; per-client override via CacheConfig constructor argument.
  • IsolationStrategy — three strategies: TENANT, TENANT_USER, and PROVIDER. Auto-selected from RuntimeContext (user ID present → TENANT_USER; no user ID → TENANT). Keys use a readable prefixed format:
    tenant:<tenant_id>                    → TENANT
    tenant:<tenant_id>:user:<user_id>     → TENANT_USER
    provider                              → PROVIDER
    
    Tenant ID is resolved from APP_TENANT_ID (IAS JWT) with fallback to DWC_TENANT (DWC header). User ID is read from USER_ID (IAS JWT). Callers do not pass these explicitly.
  • Cache façade (SDK-internal, not exported) — glues config, isolation, and backend; applies expiry_buffer_seconds; isolates backend exceptions.
  • module= keyword on Cache — optional name that shares a single InMemoryLRUBackend across all Cache instances 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 omit module= are unaffected.

First integration: token caching in XsuaaAuthProvider

core/protocol/http/models.py::XsuaaAuthProvider now uses the Cache façade instead of a manual dict + Lock for OAuth2 token caching. Tokens are cached with IsolationStrategy.TENANT — one token per tenant, keyed by tenant_subdomain. The cache is backed by the global CacheConfig, TTL is derived from the token expires_in value 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

  • New feature (non-breaking change that adds functionality)
  • Dependency update

How to Test

  1. Install dependencies: uv sync --all-extras --group dev
  2. Run the cache and HTTP client unit tests:
    uv run pytest tests/core/resilience/cache/ tests/core/ -v
    
  3. Run the lint and type-check suite:
    uv run ruff check src/sap_cloud_sdk/core/resilience/cache/ src/sap_cloud_sdk/core/protocol/http/
    uv run ruff format --check src/sap_cloud_sdk/core/resilience/cache/ src/sap_cloud_sdk/core/protocol/http/
    uv run ty check src/sap_cloud_sdk/core/resilience/cache/ src/sap_cloud_sdk/core/protocol/http/
    
  4. Expected result: all tests pass, all checks clean.

AI-Generated Code Disclosure

Parts of this PR were developed with AI assistance (Claude). Reviewed and validated per the SAP AI contribution guideline.

  • AI-generated code has been reviewed and tested

Checklist

  • I have read the Contributing Guidelines
  • I have verified that my changes solve the issue
  • I have added/updated automated tests to cover my changes
  • All tests pass locally
  • I have verified that my code follows the Code Guidelines
  • I have updated documentation (if applicable)
  • I have added type hints for all public APIs
  • My code does not contain sensitive information (credentials, tokens, etc.)
  • I have followed Conventional Commits for commit messages

Breaking Changes

None — purely additive new module.

Additional Notes

  • cachetools~=5.5.2 added to [project.dependencies] (MIT licence, already a transitive dependency of google-auth).
  • Module.CACHE = "cache" added to core/telemetry/module.py (alphabetical order).
  • The Cache façade is intentionally not exported from sap_cloud_sdk.core.resilience.cache; SDK modules import it via from sap_cloud_sdk.core.resilience.cache._cache import Cache.
  • Related sdk-docs PR: https://github.tools.sap/application-foundation/sdk/pull/46

@NicoleMGomes

Copy link
Copy Markdown
Contributor

Can we implement it in one module to exemplify usage?

@cassiofariasmachado
cassiofariasmachado force-pushed the feature/add-cache-core-module branch from 1727ebf to be75f1e Compare October 23, 2026 19:32
@cassiofariasmachado

Copy link
Copy Markdown
Member Author

Can we implement it in one module to exemplify usage?

Added to the HTTP Client Core module.

@cassiofariasmachado cassiofariasmachado left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: #265: feat(cache): add core cache module

Author: cassio-farias-machado Branch: feature/add-cache-core-module → main
Verdict: ⚠️ Needs Minor Work
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.md has two inaccuracies that will mislead users:

    • Line 41 — on_evict configuration table lists reason values 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 read reason 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 from client.py in this branch; the current in-tree integration is XsuaaAuthProvider token caching in core/protocol/http/models.py. Update this section to point to XsuaaAuthProvider instead.

⚠️ 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.py near the Cache class explaining why @record_metrics is not used here — the intent (metrics owned by consuming modules) is non-obvious to future contributors.

✅ Things Done Well

  • CacheBackend ABC has exactly four methods with no leaky abstractions — clear, minimal, and genuinely pluggable.
  • InMemoryLRUBackend correctly implements lazy TTL eviction with the "ttl" evict reason, which cachetools.TTLCache does not natively support.
  • Cache._make_full_key format (namespace::isolation_key::key) is readable and debuggable.
  • XsuaaAuthProvider correctly closes the stale OAuth2Session before overwriting (releases connection pool) — a subtle correctness detail that is easy to miss.
  • Config snapshot at Cache construction 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=False no-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 cassiofariasmachado left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py near the Cache class explaining that @record_metrics is intentionally absent (metrics owned by consuming modules) would help future contributors.

✅ Things Done Well

  • CacheBackend ABC has exactly four methods with no leaky abstractions — clear, minimal, and genuinely pluggable.
  • InMemoryLRUBackend correctly implements lazy TTL eviction with the "ttl" evict reason, which cachetools.TTLCache does not natively support.
  • Cache._make_full_key format (namespace::isolation_key::key) is readable and debuggable.
  • XsuaaAuthProvider correctly closes the stale OAuth2Session before overwriting (releases connection pool) — a subtle correctness detail that is easy to miss.
  • Config snapshot at Cache construction 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=False no-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_evict table corrected, and integration example updated to XsuaaAuthProvider.

NicoleMGomes
NicoleMGomes previously approved these changes Oct 29, 2026
@NicoleMGomes

Copy link
Copy Markdown
Contributor

Should we add cache as part of a bigger package called resilience? Just thinking in future we will have at least retry also.

@cassiofariasmachado

cassiofariasmachado commented Nov 5, 2026 •

Copy link
Copy Markdown
Member Author

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 sap_cloud_sdk.core.resilience.cache so it sits alongside future resilience primitives like retry. The resilience sub-package under core will be the home for that family of concerns.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants