diff --git a/dynamodol/base.py b/dynamodol/base.py index 202b5ea..352bcd8 100644 --- a/dynamodol/base.py +++ b/dynamodol/base.py @@ -48,6 +48,65 @@ def is_no_such_key_error(error: BaseException) -> bool: return type(error).__name__ in NO_SUCH_KEY_ERROR_CODES +#: Backend error codes that mean "this key cannot name an item" (wrong type, empty +#: string, wrong arity for the key schema): such a key is absent, not a backend failure. +KEY_CANNOT_NAME_AN_ITEM_ERROR_CODES = frozenset({"ValidationException"}) + + +def get_item_or_raise(table, key, k, *, error_cls=NoSuchKeyError, **get_item_kwargs): + """``table.get_item(Key=key)``'s ``Item``, raising ``error_cls`` only if it is absent. + + Backend failures (throttling, credentials, a missing table) propagate unchanged. + A key the table's schema rejects outright is reported absent, as before. + """ + try: + response = table.get_item(Key=key, **get_item_kwargs) + except Exception as e: + code = (getattr(e, "response", None) or {}).get("Error", {}).get("Code") + if code in KEY_CANNOT_NAME_AN_ITEM_ERROR_CODES: + raise error_cls(f"Key not found: {k}") from e + raise + return _item_or_raise(response, k, error_cls=error_cls) + + +def _item_or_raise(get_item_response, k, *, error_cls=NoSuchKeyError): + """The ``Item`` of a ``get_item`` response, or ``NoSuchKeyError`` if there is none. + + DynamoDB answers a lookup of an absent key with a response that simply has no + ``Item`` -- that, and only that, is "missing". A throttled request, bad + credentials or a missing table are backend errors and must surface as such, not + as a ``KeyError`` that makes ``k in store`` and ``store.get(k)`` quietly say + "absent". + + >>> _item_or_raise({"Item": {"key": "k1"}}, "k1") + {'key': 'k1'} + >>> _item_or_raise({}, "k1") + Traceback (most recent call last): + ... + dynamodol.base.NoSuchKeyError: 'Key not found: k1' + """ + if "Item" not in get_item_response: + raise error_cls(f"Key not found: {k}") + return get_item_response["Item"] + + +def raise_if_nothing_was_deleted(delete_item_response, k, *, error_cls=NoSuchKeyError): + """Raise ``NoSuchKeyError`` if a ``ReturnValues='ALL_OLD'`` delete removed nothing. + + DynamoDB's ``DeleteItem`` succeeds silently on an absent key -- it never reports a + ``NoSuchKey`` code (that is S3's) -- so the only sign the key was missing is that + no old ``Attributes`` came back. ``del store[missing]`` must raise ``KeyError``. + + >>> raise_if_nothing_was_deleted({"Attributes": {"key": "k1"}}, "k1") + >>> raise_if_nothing_was_deleted({}, "k1") + Traceback (most recent call last): + ... + dynamodol.base.NoSuchKeyError: 'Key not found: k1' + """ + if "Attributes" not in (delete_item_response or {}): + raise error_cls(f"Key not found: {k}") + + DFLT_TABLE_NAME = "dynamodol" DFLT_KEY_FIELDS = ("key",) DFLT_DATA_FIELDS = ("value",) @@ -270,11 +329,10 @@ def __getitem__(self, k): ) _k = (k,) _k = {att: key for att, key in zip(self.key_fields, _k)} - response = self.table.get_item(Key=_k, **self._values_expression) - item = response["Item"] - return self.format_get_item(item) - except Exception as e: - raise NoSuchKeyError(f"Key not found: {k}") + except (ValueError, TypeError) as e: + raise NoSuchKeyError(f"Key not found: {k}") from e + item = get_item_or_raise(self.table, _k, k, **self._values_expression) + return self.format_get_item(item) def iter_items(self): response = self.table.scan(**self._keys_values_expression) @@ -375,11 +433,12 @@ def __delitem__(self, k): ) k = (k,) key = {att: key for att, key in zip(self.key_fields, k)} - self.table.delete_item(Key=key) + response = self.table.delete_item(Key=key, ReturnValues="ALL_OLD") except Exception as e: if is_no_such_key_error(e): raise NoSuchKeyError(f"Key not found: {k}") from e raise + raise_if_nothing_was_deleted(response, k) def set_db_defaults(new_defaults: dict): diff --git a/dynamodol/partition_query.py b/dynamodol/partition_query.py index 02355c9..cc74e31 100644 --- a/dynamodol/partition_query.py +++ b/dynamodol/partition_query.py @@ -9,6 +9,8 @@ DynamoDbBasePersister, db_defaults, is_no_such_key_error, + get_item_or_raise, + raise_if_nothing_was_deleted, ) @@ -229,13 +231,9 @@ def format_get_key(self, item): return item[self.sort_key] def __getitem__(self, k): - try: - key = {self.partition_key: self.partition, self.sort_key: k} - response = self.table.get_item(Key=key) - item = response["Item"] - return self.format_get_item(item) - except Exception as e: - raise NoSuchKeyError(f"Key not found: {k}") + key = {self.partition_key: self.partition, self.sort_key: k} + item = get_item_or_raise(self.table, key, k, error_cls=NoSuchKeyError) + return self.format_get_item(item) @dataclass @@ -267,11 +265,10 @@ def format_get_key(self, item): def __getitem__(self, k): try: key = {self.partition_key: self.partition, self.sort_key: self.prefix + k} - response = self.table.get_item(Key=key) - item = response["Item"] - return self.format_get_item(item) - except Exception as e: - raise NoSuchKeyError(f"Key not found: {k}") + except TypeError as e: # a non-str key cannot carry the prefix + raise NoSuchKeyError(f"Key not found: {k}") from e + item = get_item_or_raise(self.table, key, k, error_cls=NoSuchKeyError) + return self.format_get_item(item) class DynamoDbPartitionPersister(DynamoDbBasePersister, DynamoDbPartitionReader): @@ -289,8 +286,9 @@ def __setitem__(self, k, v): def __delitem__(self, k): key = {self.partition_key: self.partition, self.sort_key: k} try: - self.table.delete_item(Key=key) + response = self.table.delete_item(Key=key, ReturnValues="ALL_OLD") except Exception as e: if is_no_such_key_error(e): raise NoSuchKeyError(f"Key not found: {k}") from e raise + raise_if_nothing_was_deleted(response, k, error_cls=NoSuchKeyError) diff --git a/tests/test_offline.py b/tests/test_offline.py index 2179e2e..806cf6f 100644 --- a/tests/test_offline.py +++ b/tests/test_offline.py @@ -35,15 +35,18 @@ class FakeTable: ``delete_item`` so backend failures can be exercised without a live table. """ - def __init__(self, items=(), *, delete_error=None): + def __init__(self, items=(), *, delete_error=None, get_error=None): self.items = [dict(item) for item in items] self.delete_error = delete_error + self.get_error = get_error @staticmethod def _matches(item, key): return all(item.get(k) == v for k, v in key.items()) def get_item(self, Key, **kwargs): + if self.get_error is not None: + raise self.get_error for item in self.items: if self._matches(item, Key): return {"Item": dict(item)} @@ -61,10 +64,16 @@ def put_item(self, Item): self.items = [item for item in self.items if not self._matches(item, Item)] self.items.append(dict(Item)) - def delete_item(self, Key): + def delete_item(self, Key, ReturnValues="NONE"): + """Like boto3: an absent key is deleted silently; ``ALL_OLD`` returns the + deleted item's ``Attributes`` (and nothing when there was none).""" if self.delete_error is not None: raise self.delete_error + old = [item for item in self.items if self._matches(item, Key)] self.items = [item for item in self.items if not self._matches(item, Key)] + if ReturnValues == "ALL_OLD" and old: + return {"Attributes": dict(old[0])} + return {} class FakeDb: @@ -84,8 +93,8 @@ def Table(self, table_name): return self._table -def _mk_persister(items=(), *, delete_error=None): - table = FakeTable(items, delete_error=delete_error) +def _mk_persister(items=(), *, delete_error=None, get_error=None): + table = FakeTable(items, delete_error=delete_error, get_error=get_error) store = DynamoDbBasePersister( db=FakeDb(table), table_name=TABLE_NAME, @@ -95,8 +104,8 @@ def _mk_persister(items=(), *, delete_error=None): return store, table -def _mk_partition_persister(items=(), *, delete_error=None): - table = FakeTable(items, delete_error=delete_error) +def _mk_partition_persister(items=(), *, delete_error=None, get_error=None): + table = FakeTable(items, delete_error=delete_error, get_error=get_error) store = DynamoDbPartitionPersister( db=FakeDb(table), table_name=TABLE_NAME, @@ -178,3 +187,56 @@ def test_delitem_removes_the_item(mk_store, key): store, table = mk_store(items) del store[key] assert table.items == [] + + +# --- a missing key is a KeyError; a backend failure is not ------------------- + +_BOTH_STORES = pytest.mark.parametrize( + "mk_store,items,key", + [ + (_mk_persister, ONE_ITEM, "k1"), + (_mk_partition_persister, ONE_PARTITIONED_ITEM, "s1"), + ], +) + + +@_BOTH_STORES +def test_delitem_of_a_missing_key_raises_key_error(mk_store, items, key): + """DynamoDB's DeleteItem succeeds silently on an absent key (``NoSuchKey`` is an + S3 code, never a DynamoDB one), so ``del store[missing]`` used to do nothing.""" + store, table = mk_store(items) + with pytest.raises(KeyError): + del store["zzz"] + assert len(table.items) == 1 # and the present item is untouched + + +@_BOTH_STORES +def test_getitem_propagates_backend_errors(mk_store, items, key): + """A throttled or unauthorised read used to become a KeyError, so ``.get`` and + the items view reported a present key as absent.""" + error = _client_error("ProvisionedThroughputExceededException", "GetItem") + store, _ = mk_store(items, get_error=error) + with pytest.raises(ClientError): + store[key] + with pytest.raises(ClientError): + store.get(key) + with pytest.raises(ClientError): + (key, "A") in store.items() + + +@_BOTH_STORES +def test_a_key_the_schema_rejects_is_absent(mk_store, items, key): + error = _client_error("ValidationException", "GetItem") + store, _ = mk_store(items, get_error=error) + with pytest.raises(KeyError): + store[key] + assert store.get(key, "DFLT") == "DFLT" + + +@_BOTH_STORES +def test_missing_key_lookups_stay_key_errors(mk_store, items, key): + store, _ = mk_store(items) + assert "zzz" not in store + assert store.get("zzz", "DFLT") == "DFLT" + with pytest.raises(KeyError): + store["zzz"]