Add multi_delete() to delete multiple keys in one call - #1084
ChrisJr404 wants to merge 1 commit into
Conversation
Confidence Score: 3/5The PR should not merge until existing custom backends remain instantiable and memory batches correctly account for keys storing None. The new abstract primitive creates an immediate compatibility failure for documented BaseCache extensions, while the memory implementation can remove a valid key yet return the wrong count and retain stale expiration state. Files Needing Attention: aiocache/base.py, aiocache/backends/memory.py Reviews (1): Last reviewed commit: "Add multi_delete to delete multiple keys..." | Re-trigger Greptile |
| @abstractmethod | ||
| async def _multi_delete(self, keys, _conn=None): | ||
| raise NotImplementedError() |
There was a problem hiding this comment.
Abstract primitive breaks custom backends
When an existing custom BaseCache subclass lacks _multi_delete, making the new primitive abstract prevents that backend from being instantiated and raises TypeError, even when the application never calls multi_delete.
| @abstractmethod | |
| async def _multi_delete(self, keys, _conn=None): | |
| raise NotImplementedError() | |
| async def _multi_delete(self, keys, _conn=None): | |
| deleted = [await self._delete(key, _conn=_conn) for key in keys] | |
| return sum(deleted) |
Knowledge Base Used:
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| async def _multi_delete(self, keys, _conn=None): | ||
| return sum(self.__delete(key) for key in keys) |
There was a problem hiding this comment.
None values corrupt deletion accounting
When a memory-cache key stores literal None, _multi_delete removes the key but reports zero deletions because __delete treats the popped value as a miss; when that key has a TTL, its expiration handle also remains registered until it fires.
Knowledge Base Used:
There was a problem hiding this comment.
Isn't that a bug in __delete()? i.e. Should be fixed separately, not related to this PR.
There was a problem hiding this comment.
You're right. The None value issue is a pre-existing bug in __delete() — it predates this PR and affects _delete() equally. _multi_delete is correctly delegating to the same __delete method, so it's consistent with the existing behavior.
This PR shouldn't be held responsible for fixing a separate bug. Happy to withdraw that part of the review.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
This adds
multi_delete()so you can delete a batch of keys in one call, rounding out the multi_get/multi_set pair. Closes #517.Each backend does the sensible thing: valkey issues a single DEL with all the keys, memcached fires the deletes concurrently, and memory just pops them. Like
delete(), it returns the number of keys that were actually removed, and it honours the namespace.Added unit tests for each backend plus behavioural tests in the acceptance suite, along with a changelog entry and docs.