Fix: del of a missing key was silent; getitem masked backend errors as KeyError

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):

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)

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"]