Repository navigation
Fix: del of a missing key was silent; getitem masked backend errors as KeyError - #3
Merged
Conversation
… longer KeyErrors DynamoDB DeleteItem succeeds silently on an absent key (NoSuchKey is an S3 code), so #2's NoSuchKey mapping never fired: del store[missing] did nothing. Delete now asks for ReturnValues=ALL_OLD and raises NoSuchKeyError when nothing was deleted. __getitem__ no longer turns every exception (throttling, auth, missing table) into NoSuchKeyError, which made .get() and the items view that #2 added report present keys as absent; only a missing Item or a key the schema rejects (ValidationException) is a KeyError now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Post-merge refute review of #2.
Defect 1, missing-key delete. #2 maps a
NoSuchKeyerror code toNoSuchKeyErrorin both__delitem__s. DynamoDB never raises that.NoSuchKeyis an S3 code, and DynamoDB's DeleteItem succeeds silently on an absent key. Sodel store['missing']still does nothing and does not raiseKeyErroras the MutableMapping contract requires. It is nowdelete_item(..., ReturnValues='ALL_OLD'), which raisesNoSuchKeyErrorwhen no oldAttributescome back. That costs no extra request.Defect 2, getitem masking. #2's new
ItemsView.__contains__goes through.get. All three__getitem__s turned every exception intoNoSuchKeyError, including throttling, bad credentials and a missing table. So.get(k)and(k, v) in store.items()reported a present key as absent whenever the backend hiccuped. That is the same masking #2 set out to remove from delete. Now only a response withoutItem, or a key the schema rejects (ValidationException, e.g. wrong type or empty string, which is what the old code answered as absent), is aKeyError. Everything else propagates.Tests (in
tests/test_offline.py, with the FakeTable now mimicking boto3'sReturnValues):delof a missing key raisesKeyErrorfor base and partition stores; a throttled GetItem propagates from[],.getand the items view; ValidationException stays aKeyError; missing-keyget/inbehave the same as before. 4 of the new tests fail on master. All 19 pass here, plus the helper doctests.Note:
k in storeitself isdol.Collection.__contains__, a full scan. That is pre-existing and not changed here.Self-reviewed only (the dispatching run disallowed sub-agents). No live DynamoDB used.
🤖 Generated with Claude Code