Wrap corrupt cache reads in CacheCorruptError#62
Conversation
FileCache#read leaks JSON::ParserError and KeyError to callers when the cache file is malformed (truncated, hand-edited, partial write). The original message gives no hint that the file is FixtureKit's cache or that the fix is to delete and regenerate. Wrap both error classes in a new FixtureKit::CacheCorruptError that includes the cache path and the original error class/message, plus a short note pointing at the remediation. This is a strict superset of the previous behavior — a rescue on the new error class still catches malformed cache files, and StandardError catches everything as before. No retry, no auto-regeneration. Disk-vs-memory consistency bugs should be loud, not silently papered over.
ngan
left a comment
ngan
left a comment
There was a problem hiding this comment.
Thanks for the PR, left a comment.
| rescue JSON::ParserError, KeyError => e | ||
| raise FixtureKit::CacheCorruptError, | ||
| "FixtureKit cache file at #{path} is corrupt or malformed (#{e.class}: #{e.message}). " \ | ||
| "Delete it and re-run to regenerate." |
There was a problem hiding this comment.
Can you localize these rescues so it's not rescuing errors across many statements doing very different things? Maybe move the JSON.parse(...) into its own method and do the rescue there. To DRY up the error message, you can bake it into the error class and you'd just pass in the error and the path.
There was a problem hiding this comment.
Done. I pulled the JSON parse and key validation into a private #parse so the rescue only wraps that step. One nice side effect: coder_for's KeyError (an unregistered coder) is no longer caught and mislabeled as a corrupt file. That's a configuration error, so it now propagates as itself.
For the message, I baked it into the class but went with a CacheCorruptError.for(path, error) factory instead of a custom initialize. Same ergonomics you suggested (the call site just passes the path and the error), and it keeps the standard raise CacheCorruptError, "..." constructor working. Also added the class to the reference docs.
Scope the rescue to JSON parsing and required-key validation in a new private #parse, so decode-stage errors (e.g. an unregistered coder's KeyError from #coder_for) are no longer mislabeled as a corrupt cache file. Move the message into CacheCorruptError via a .for(path, cause) factory, which keeps the standard raise idiom intact. Document CacheCorruptError in the reference. Addresses review feedback from @ngan on Gusto#62.
2 participants
Summary
FileCache#readleaksJSON::ParserErrorandKeyErrordirectly to callers when the cache file on disk is malformed (truncated, hand-edited, partially written). The resulting error gives no hint that the file is FixtureKit's cache, what path it lives at, or how to recover.Common scenarios where this surfaces:
Fix
Wrap
JSON::ParserErrorandKeyErrorraised insideFileCache#readin a newFixtureKit::CacheCorruptErrorwhose message includes the cache path, the original error class and message, and a short remediation hint ("Delete it and re-run to regenerate.").This is a strict superset of the previous behavior. A rescue on the new error class still catches malformed cache files, and
StandardErrorcatches everything as before.Design notes
Errno::ENOENTis intentionally not wrapped. Missing-file semantics differ from corrupt-file semantics, andCache#loadalready checksexists?first and raisesCacheMissingErrorfor the missing case.CacheMissingErrorinlib/fixture_kit.rb.Related
Follow-up from review feedback on #61.
Test plan
bundle exec rspec spec/unit/file_cache_spec.rb— 11/11 pass (including 2 new specs for invalid JSON and missing keys)bundle exec rspec(full suite) — 176/176 pass