added bulk delete option #107
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #107 +/- ##
==========================================
+ Coverage 90.94% 91.04% +0.09%
==========================================
Files 24 24
Lines 1237 1250 +13
==========================================
+ Hits 1125 1138 +13
Misses 112 112 ☔ View full report in Codecov by Sentry. |
|
@dantownsend What do you think about this? Can it be useful? |
|
@sinisaos It's definitely useful. Just need to test it locally. |
|
@dantownsend Great. Thanks. |
docs/source/crud/piccolo_crud.rst
Outdated
| @@ -14,7 +14,7 @@ Endpoints | |||
| ========== ======================= ========================================================================================================== | |||
| Path Methods Description | |||
| ========== ======================= ========================================================================================================== | |||
| / GET, POST, DELETE Get all rows, post a new row, or delete all matching rows. | |||
| / GET, POST Get all rows or post a new row. | |||
There was a problem hiding this comment.
I think we should keep DELETE here.
piccolo_api/crud/endpoints.py
Outdated
| self.table.delete().where( | ||
| self.table._meta.primary_key.is_in( | ||
| [int(item) for item in split_params_ids] | ||
| ) | ||
| ), |
There was a problem hiding this comment.
Here we should check if there are any valid ids before applying the filter, otherwise it will fail.
query = self.table.delete()
ids = [int(item) for item in split_params_ids]
if ids:
query = query.where(self.table._meta.primary_key.is_in(ids))
query = self._apply_filters(query, split_params)There was a problem hiding this comment.
Yes, you're right! I changed the delete_bulk method to work only with filter params (e.g. name param), only with __ids param and to work together for additional safety.
try:
query: t.Union[
Select, Count, Objects, Delete
] = self.table.delete()
try:
# if __ids query param is not empty
ids = [int(item) for item in split_params_ids]
query_ids = query.where(
self.table._meta.primary_key.is_in(ids)
)
query = self._apply_filters(query_ids, split_params)
except ValueError:
# if __ids query param is empty
query = self._apply_filters(query, split_params)
except MalformedQuery as exception:
return Response(str(exception), status_code=400)It's a little complicated but it works.
tests/crud/test_crud_endpoints.py
Outdated
| @@ -804,17 +804,19 @@ def test_no_bulk_delete(self): | |||
|
|
|||
| def test_bulk_delete(self): | |||
| """ | |||
| Make sure that bulk deletes are only allowed is allow_bulk_delete is | |||
| True. | |||
| Make sure that bulk deletes are only allowed is ``allow_bulk_delete`` | |||
There was a problem hiding this comment.
There's an old typo here - should be if instead of is.
docs/source/crud/piccolo_crud.rst
Outdated
| Bulk delete | ||
| ----------- | ||
|
|
||
| To specify which records you want to delete in bulk, pass a query parameter | ||
| like this ``__ids=1,2,3``, and you be able to delete all results whose ``id`` | ||
| is in the query params. | ||
|
|
||
| A query which delete movies with ``id`` pass in query parameter: | ||
|
|
||
| .. code-block:: | ||
|
|
||
| DELETE https://demo1.piccolo-orm.com/api/tables/movie/?__ids=1,2,3 | ||
|
|
||
| .. warning:: To be able to provide a bulk delete action, we must set | ||
| ``allow_bulk_delete`` to ``True``. | ||
|
|
||
| ------------------------------------------------------------------------------- | ||
|
|
There was a problem hiding this comment.
It would be good for us to expand upon these docs some more at some point. You should be able to do stuff like:
DELETE /movie/?__name=foobar
There was a problem hiding this comment.
You're right. We can write that all three methods work.
DELETE /movie/?__name=foobar (delete all movies with name foobar)
DELETE /movie/?__ids=1,2,3 (delete all movies with id 1,2,3)
DELETE /movie/?__name=foobar&__ids=1,2 (delete all movies with name foobar, but only if id is 1 or 2)
|
@sinisaos This is a great start. I've left some comments. |
|
@dantownsend I made proposed changes. Can you check it out? |
|
@sinisaos Cool, thanks for this. I was thinking about this last night. What happens when we do What you've added is a good solution, but it's part of a wider problem - being able to specify multiple values for a filter. Imagine we wanted to filter by name: I wonder if there's some way of making this work? If not we can do with the |
No. It doesn't work.
and this also doesn't work due to a validation error because we can't filter with the same query params as
I don't see any other way except to do the same as for I think |
|
@sinisaos I vaguely remember being able to pass in multiple values, but I might be confusing it with something else. Was it by doing something like: FastAPI makes it more difficult as it expects a single integer. But maybe if the type annotation is changes to I'll have a look tomorrow. It probably isn't doable, but just in case. |
|
Closed - open for too long without rejection, merge or activity. Feel free to reopen PR and use it again if code is still relevant. |
|
@sinisaos All of these PRs are still relevant. I'm sorry I haven't been able to merge them in yet. What tends to happen is a bug comes along which takes higher priority, or I don't have time to finish reviewing it. I think moving forward there needs to be a better system around code review, so good PRs don't languish. |
|
@dantownsend No worries. I know there are a lot of things that have higher priority (especially in Piccolo ORM). I closed all PRs older than 3-4 months because nothing happens in them and it makes no sense for them to be open after all code changes that have happened in the meantime. Feel free to reopen it or use it again if you find something interesting. Cheers. |
Related to #11