http: extract JSONFileStore primitive from FileTokenStore#160
Merged
Conversation
The planeed auth-plugin work needs to cache plugin authentication responses on disk with the same guarantees the OAuth token cache already provides: atomic write so a kill mid-write never leaves a corrupt file, POSIX 0700/0600 hardening, corruption-tolerant reads, and key sanitization. Rather than copy that machinery into a second file-backed store, lift it into a generic JSONFileStore[T] in sap/http/cache.py. FileTokenStore becomes a thin specialization that inherits from JSONFileStore[Token] and the TokenStore ABC and only supplies Token (de)serialization. The future plugin-response cache will follow the same ~10-line pattern over a PluginResponse payload, with a single source of truth for the storage contract. JSONFileStore is listed first in FileTokenStore's MRO so its concrete get/set/delete satisfy the abstract methods declared on TokenStore; otherwise ABCMeta resolves get/set/delete to the abstract versions and refuses to instantiate the class. Tests for the platform/permission helpers and the generic store behaviour move to test_sap_http_cache.py, exercised against a tiny _Sample payload type. test_sap_http_token_cache.py keeps the Token-specific tests and redirects os.replace patches to sap.http.cache, which is now the call site. Naming sap/http/cache.py was misleading: HTTP "cache" reads as the HTTP-response cache (Cache-Control, ETags) rather than what this module actually provides — a generic file-backed typed JSON store. The class inside is JSONFileStore, so json_store.py names the contents directly and removes the false signal for future maintainers. Both consumers of the primitive (Token, planned plugin auth response) happen to be auth-related, but the primitive itself is just typed JSON on disk. A use-case-named module like auth_cache.py would have hidden that and re-imported the same confusion the rename is meant to remove. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR extracts a generic file-backed JSON cache primitive ( ChangesShared JSON Store Abstraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/unit/test_sap_http_token_cache.py (1)
53-56: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace loop-based patch registration with explicit sequential patch setup.
The loop in module test setup conflicts with the repo test-style rule. Please register each patcher explicitly.
As per coding guidelines "Write tests sequentially without loops".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/test_sap_http_token_cache.py` around lines 53 - 56, The loop that registers patchers (iterating over targets, creating patcher = patch(target, return_value=return_value), calling patcher.start(), and appending to _module_patchers) must be replaced with explicit, sequential patch registration: remove the for-loop and instead create and start a separate patcher for each target entry and append each to _module_patchers (e.g., create patcher1 = patch(...); patcher1.start(); _module_patchers.append(patcher1); then patcher2 = patch(...); etc.), keeping the same target names and return values used in the original targets list so the test_setup uses explicit, non-looped patcher variables.
🧹 Nitpick comments (1)
sap/http/token_cache.py (1)
41-50: ⚡ Quick winSort
__all__to satisfy Ruff (RUF022).This is a quick cleanup to keep static analysis green and reduce noisy diffs later.
Suggested fix
__all__ = [ - 'Token', - 'TokenStore', 'FileTokenStore', - 'get_token_store', + 'Token', + 'TokenStore', '_default_cache_dir', '_harden_dir', '_harden_file', '_sanitize', + 'get_token_store', ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sap/http/token_cache.py` around lines 41 - 50, The __all__ export list is unsorted and triggers Ruff RUF022; sort the entries in the __all__ list alphabetically (e.g., arrange 'FileTokenStore', 'Token', 'TokenStore', '_harden_dir', '_harden_file', '_sanitize', '_default_cache_dir', 'get_token_store' into proper lexicographic order) so the module-level __all__ is deterministic and satisfies the linter; update the __all__ list near the top of token_cache.py (the list containing 'Token', 'TokenStore', 'FileTokenStore', 'get_token_store', '_default_cache_dir', '_harden_dir', '_harden_file', '_sanitize').
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sap/http/json_store.py`:
- Around line 108-121: The helpers _harden_dir and _harden_file currently catch
and silently ignore OSError; update each except block to either log the
exception at a debug level or (per guidelines) add an explicit inline comment
explaining why swallowing the error is acceptable (e.g., best‑effort hardening
on POSIX only, non‑fatal if chmod fails, no sensitive data at risk), so future
readers know the behavior is intentional; keep the try/except structure but
include the explanatory comment (or a debug log via the module logger) inside
the except.
- Around line 57-60: The get() cache-read currently only treats
OSError/ValueError/KeyError from self._deserialize(...) as a corrupt-cache miss;
include TypeError as well so schema-mismatch errors are caught and treated as a
missing cache entry. Update the except clause in the get() implementation that
wraps self._deserialize(...) to also catch TypeError (i.e., except (OSError,
ValueError, KeyError, TypeError):) so _deserialize() raising TypeError due to
schema mismatch will not propagate.
- Around line 69-73: The current write uses a fixed temporary filename (tmp =
path.with_suffix(path.suffix + '.tmp') / tmp.write_text(...)) which causes
cross-process collisions; change the write logic in the function that calls
path.with_suffix, tmp.write_text, _harden_file and os.replace to create a unique
per-write temp file (e.g. incorporate a UUID/pid/timestamp or use tempfile to
create a unique name in the same directory), write the serialized content to
that unique temp path, call _harden_file on that unique temp, and then
atomically os.replace the unique temp into path to avoid other writers trampling
each other.
In `@test/unit/test_sap_http_json_store.py`:
- Around line 43-46: The test currently builds patches in a loop over targets
(using variables targets, patch, patcher.start(), and _module_patchers) which
violates the "no loops in tests" rule; replace the for-loop with explicit
sequential patch registrations by calling patch(...) for each target
individually, starting each patcher and appending it to _module_patchers (e.g.,
create patcherA = patch(<first-target>, return_value=<first-value>);
patcherA.start(); _module_patchers.append(patcherA); then repeat for the second
target, etc.), ensuring each patch is registered in its own statement rather
than via iteration.
---
Outside diff comments:
In `@test/unit/test_sap_http_token_cache.py`:
- Around line 53-56: The loop that registers patchers (iterating over targets,
creating patcher = patch(target, return_value=return_value), calling
patcher.start(), and appending to _module_patchers) must be replaced with
explicit, sequential patch registration: remove the for-loop and instead create
and start a separate patcher for each target entry and append each to
_module_patchers (e.g., create patcher1 = patch(...); patcher1.start();
_module_patchers.append(patcher1); then patcher2 = patch(...); etc.), keeping
the same target names and return values used in the original targets list so the
test_setup uses explicit, non-looped patcher variables.
---
Nitpick comments:
In `@sap/http/token_cache.py`:
- Around line 41-50: The __all__ export list is unsorted and triggers Ruff
RUF022; sort the entries in the __all__ list alphabetically (e.g., arrange
'FileTokenStore', 'Token', 'TokenStore', '_harden_dir', '_harden_file',
'_sanitize', '_default_cache_dir', 'get_token_store' into proper lexicographic
order) so the module-level __all__ is deterministic and satisfies the linter;
update the __all__ list near the top of token_cache.py (the list containing
'Token', 'TokenStore', 'FileTokenStore', 'get_token_store',
'_default_cache_dir', '_harden_dir', '_harden_file', '_sanitize').
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3c57b27-0c6c-481e-b1ab-c983989da703
📒 Files selected for processing (4)
sap/http/json_store.pysap/http/token_cache.pytest/unit/test_sap_http_json_store.pytest/unit/test_sap_http_token_cache.py
Three robustness improvements to the cache primitive: 1. Unique per-write tmp filenames. The fixed '<key>.json.tmp' name meant two processes (or threads) writing the same key would step on each other's in-flight tmp file, with whoever lost the race promoting garbage. Each set() now writes to '<key>.json.<uuid>.tmp' and only that unique path goes through os.replace, so concurrent writers no longer collide. On serialize/IO failure the unique tmp is unlinked so we do not accumulate stale .tmp garbage. 2. get() also catches TypeError from _deserialize. The old except clause handled OSError/ValueError/KeyError, which covered IO failures, invalid JSON, and missing keys. It missed schema drift where the cached JSON deserializes into a different shape entirely (e.g. a list instead of a mapping), making subsequent dict-style access raise TypeError. Treat that as a corrupt-cache miss too rather than letting it propagate. 3. _harden_dir / _harden_file now carry inline comments explaining why swallowing OSError is intentional (best-effort POSIX hardening on read-only / sandboxed / restricted filesystems; the user-scoped cache root is the actual access boundary). Per project guideline against silently swallowing caught exceptions without a why. Test changes: - New tests for the unique-tmp-per-write contract and the failure-path cleanup of the unique tmp. - New TypeError test in TestJSONFileStoreGet covering the schema-drift case. - setUpModule patch registration in both test files unrolled into explicit per-target statements (no for-loop), per the project rule against loops in tests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The planeed auth-plugin work needs to cache plugin authentication responses on disk with the same guarantees the OAuth token cache already provides: atomic write so a kill mid-write never leaves a corrupt file, POSIX 0700/0600 hardening, corruption-tolerant reads, and key sanitization.
Rather than copy that machinery into a second file-backed store, lift it into a generic JSONFileStore[T] in sap/http/cache.py. FileTokenStore becomes a thin specialization that inherits from JSONFileStore[Token] and the TokenStore ABC and only supplies Token (de)serialization. The future plugin-response cache will follow the same ~10-line pattern over a PluginResponse payload, with a single source of truth for the storage contract.
JSONFileStore is listed first in FileTokenStore's MRO so its concrete get/set/delete satisfy the abstract methods declared on TokenStore; otherwise ABCMeta resolves get/set/delete to the abstract versions and refuses to instantiate the class.
Tests for the platform/permission helpers and the generic store behaviour move to test_sap_http_cache.py, exercised against a tiny _Sample payload type. test_sap_http_token_cache.py keeps the Token-specific tests and redirects os.replace patches to sap.http.cache, which is now the call site.
Naming sap/http/cache.py was misleading: HTTP "cache" reads as the HTTP-response cache (Cache-Control, ETags) rather than what this module actually provides — a generic file-backed typed JSON store. The class inside is JSONFileStore, so json_store.py names the contents directly and removes the false signal for future maintainers.
Both consumers of the primitive (Token, planned plugin auth response) happen to be auth-related, but the primitive itself is just typed JSON on disk. A use-case-named module like auth_cache.py would have hidden that and re-imported the same confusion the rename is meant to remove.