Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bulk delete and bulk update #145

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions docs/source/crud/piccolo_crud.rst
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,42 @@ For example, to return results 11 to 20:

-------------------------------------------------------------------------------

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.

.. hint:: You can also use this method with ``UUID`` primary keys and
the usage is the same.

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

You can delete rows in bulk with any filter params. A query which
delete movies with ``name`` pass in query parameter:

.. code-block::

DELETE https://demo1.piccolo-orm.com/api/tables/movie/?name=Star

Or you can combine multiple query params for additional security.
A query to delete records with name ``Star``, but with
specific ``id`` you can pass query like this:

.. code-block::

DELETE https://demo1.piccolo-orm.com/api/tables/movie/?name=Star&__ids=1,2

.. warning:: To be able to provide a bulk delete action, we must set
``allow_bulk_delete`` to ``True``.

-------------------------------------------------------------------------------

Readable
--------

Expand Down
36 changes: 28 additions & 8 deletions piccolo_api/crud/endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ class Params:
include_readable: bool = False
page: int = 1
page_size: t.Optional[int] = None
ids: str = field(default="")
visible_fields: str = field(default="")
range_header: bool = False
range_header_name: str = field(default="")
Expand Down Expand Up @@ -155,8 +156,8 @@ def __init__(
:param read_only:
If ``True``, only the GET method is allowed.
:param allow_bulk_delete:
If ``True``, allows a delete request to the root to delete all
matching records. It is dangerous, so is disabled by default.
If True, allows a delete request to the root and delete all
matching records with values in ``__ids`` query params.
:param page_size:
The number of results shown on each page by default.
:param exclude_secrets:
Expand Down Expand Up @@ -521,7 +522,7 @@ async def root(self, request: Request) -> Response:
return await self.post_single(request, data)
elif request.method == "DELETE":
params = dict(request.query_params)
return await self.delete_all(request, params=params)
return await self.delete_bulk(request, params=params)
else:
return Response(status_code=405)

Expand All @@ -548,6 +549,9 @@ def _split_params(params: t.Dict[str, t.Any]) -> Params:

And can specify which page: {'__page': 2}.

You can specify witch records want to delete from rows:
dantownsend marked this conversation as resolved.
Show resolved Hide resolved
{'__ids': '1,2,3'}.

You can specify which fields want to display in rows:
{'__visible_fields': 'id,name'}.

Expand Down Expand Up @@ -601,6 +605,10 @@ def _split_params(params: t.Dict[str, t.Any]) -> Params:
response.page_size = page_size
continue

if key == "__ids":
response.ids = value
continue

if key == "__visible_fields":
response.visible_fields = value
continue
Expand Down Expand Up @@ -820,19 +828,31 @@ async def post_single(
return Response("Unable to save the resource.", status_code=500)

@apply_validators
async def delete_all(
async def delete_bulk(
self, request: Request, params: t.Optional[t.Dict[str, t.Any]] = None
) -> Response:
"""
Deletes all rows - query parameters are used for filtering.
Bulk deletes rows - query parameters are used for filtering.
"""
params = self._clean_data(params) if params else {}
split_params = self._split_params(params)
split_params_ids = split_params.ids.split(",")

try:
query = self._apply_filters(
self.table.delete(force=True), split_params
)
query: t.Union[
Select, Count, Objects, Delete
] = self.table.delete()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be query: Delete = self.table.delete()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately that raise mypy error (error: Incompatible types in assignment (expression has type "Union[Select, Count, Objects, Delete]", variable has type "Delete")
Because of that I added t.Union[Select, Count, Objects, Delete] type annotation.

try:
ids = [
int(item) if len(item) < len(str(uuid.uuid4())) else item
for item in split_params_ids
]
dantownsend marked this conversation as resolved.
Show resolved Hide resolved
query_ids = query.where(
self.table._meta.primary_key.is_in(ids)
)
query = self._apply_filters(query_ids, split_params)
except ValueError:
query = self._apply_filters(query, split_params)
except MalformedQuery as exception:
return Response(str(exception), status_code=400)

Expand Down
4 changes: 2 additions & 2 deletions piccolo_api/crud/validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def __init__(
delete_single: t.List[ValidatorFunction] = [],
post_single: t.List[ValidatorFunction] = [],
get_all: t.List[ValidatorFunction] = [],
delete_all: t.List[ValidatorFunction] = [],
delete_bulk: t.List[ValidatorFunction] = [],
get_references: t.List[ValidatorFunction] = [],
get_ids: t.List[ValidatorFunction] = [],
get_new: t.List[ValidatorFunction] = [],
Expand All @@ -49,7 +49,7 @@ def __init__(
self.delete_single = delete_single
self.post_single = post_single
self.get_all = get_all
self.delete_all = delete_all
self.delete_bulk = delete_bulk
self.get_references = get_references
self.get_ids = get_ids
self.get_new = get_new
Expand Down
18 changes: 18 additions & 0 deletions piccolo_api/fastapi/endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,24 @@ def modify_signature(
)
)

if http_method == HTTPMethod.delete:
parameters.extend(
[
Parameter(
name="__ids",
kind=Parameter.POSITIONAL_OR_KEYWORD,
annotation=str,
default=Query(
default=None,
description=(
"Specifies which rows you want to delete "
"in bulk (default ' ')."
),
),
),
]
)

if http_method == HTTPMethod.get:
if allow_ordering:
parameters.extend(
Expand Down
142 changes: 132 additions & 10 deletions tests/crud/test_crud_endpoints.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from enum import Enum
from unittest import TestCase

from piccolo.columns import ForeignKey, Integer, Secret, Varchar
from piccolo.columns import UUID, ForeignKey, Integer, Secret, Varchar
from piccolo.columns.readable import Readable
from piccolo.table import Table
from starlette.datastructures import QueryParams
Expand Down Expand Up @@ -33,6 +33,11 @@ class TopSecret(Table):
confidential = Secret()


class Studio(Table):
pk = UUID(primary_key=True)
name = Varchar()


class TestGetVisibleFieldsOptions(TestCase):
def test_without_joins(self):
response = get_visible_fields_options(table=Role, max_joins=0)
Expand Down Expand Up @@ -1075,9 +1080,11 @@ def test_get_404(self):
class TestBulkDelete(TestCase):
def setUp(self):
Movie.create_table(if_not_exists=True).run_sync()
Studio.create_table(if_not_exists=True).run_sync()

def tearDown(self):
Movie.alter().drop_table().run_sync()
Studio.alter().drop_table().run_sync()

def test_no_bulk_delete(self):
"""
Expand All @@ -1097,25 +1104,27 @@ def test_no_bulk_delete(self):
movie_count = Movie.count().run_sync()
self.assertEqual(movie_count, 1)

def test_bulk_delete(self):
def test_bulk_delete_pk_serial(self):
"""
Make sure that bulk deletes are only allowed is allow_bulk_delete is
True.
Make sure that bulk deletes are only allowed if ``allow_bulk_delete``
is True.
"""
client = TestClient(
PiccoloCRUD(table=Movie, read_only=False, allow_bulk_delete=True)
)

movie = Movie(name="Star Wars", rating=93)
movie.save().run_sync()
Movie.insert(
Movie(name="Star Wars", rating=93),
Movie(name="Lord of the Rings", rating=90),
).run_sync()

response = client.delete("/")
response = client.delete("/", params={"__ids": "1,2"})
self.assertEqual(response.status_code, 204)

movie_count = Movie.count().run_sync()
self.assertEqual(movie_count, 0)

def test_bulk_delete_filtering(self):
def test_bulk_delete_pk_serial_filtering(self):
"""
Make sure filtering works with bulk deletes.
"""
Expand All @@ -1128,13 +1137,126 @@ def test_bulk_delete_filtering(self):
Movie(name="Lord of the Rings", rating=90),
).run_sync()

response = client.delete("/?name=Star%20Wars")
response = client.delete(
"/", params={"__ids": "1", "name": "Star Wars"}
)
self.assertEqual(response.status_code, 204)

movies = Movie.select().run_sync()
self.assertEqual(len(movies), 1)
self.assertEqual(movies[0]["name"], "Lord of the Rings")

def test_bulk_delete_pk_serial_filtering_without_ids(self):
"""
Make sure filtering works with bulk deletes and
without ``__ids`` query params.
"""
client = TestClient(
PiccoloCRUD(table=Movie, read_only=False, allow_bulk_delete=True)
)

Movie.insert(
Movie(name="Star Wars", rating=93),
Movie(name="Lord of the Rings", rating=90),
).run_sync()

response = client.delete("/", params={"name": "Star Wars"})
self.assertEqual(response.status_code, 204)

movies = Movie.select().run_sync()
self.assertEqual(len(movies), 1)
self.assertEqual(movies[0]["name"], "Lord of the Rings")

def test_bulk_delete_pk_uuid(self):
"""
Make sure that bulk deletes are only allowed if ``allow_bulk_delete``
is True.
"""
client = TestClient(
PiccoloCRUD(table=Studio, read_only=False, allow_bulk_delete=True)
)

Studio.insert(
Studio(
pk="af5dc416-2784-4d63-87a1-c987f7ad57fc", name="Blasting Room"
),
Studio(
pk="708a7531-b1cf-4ff8-a25d-3287fad1bac4", name="JHOC Studio"
),
).run_sync()

studios = Studio.select().run_sync()
response = client.delete(
"/",
params={"__ids": f"{studios[0]['pk']},{studios[1]['pk']}"},
)
self.assertEqual(response.status_code, 204)

studio_count = Studio.count().run_sync()
self.assertEqual(studio_count, 0)

def test_bulk_delete_pk_uuid_filtering(self):
"""
Make sure filtering works with bulk deletes.
"""
client = TestClient(
PiccoloCRUD(table=Studio, read_only=False, allow_bulk_delete=True)
)

Studio.insert(
Studio(
pk="af5dc416-2784-4d63-87a1-c987f7ad57fc", name="Blasting Room"
),
Studio(
pk="708a7531-b1cf-4ff8-a25d-3287fad1bac4", name="JHOC Studio"
),
).run_sync()

studios = Studio.select().run_sync()
response = client.delete(
"/",
params={
"__ids": f"{studios[0]['pk']}",
"name": f"{studios[0]['name']}",
},
)

studios = Studio.select().run_sync()
self.assertEqual(response.status_code, 204)
self.assertEqual(len(studios), 1)
self.assertEqual(studios[0]["name"], "JHOC Studio")

def test_bulk_delete_pk_uuid_filtering_without_ids(self):
"""
Make sure filtering works with bulk deletes and
without ``__ids`` query params.
"""
client = TestClient(
PiccoloCRUD(table=Studio, read_only=False, allow_bulk_delete=True)
)

Studio.insert(
Studio(
pk="af5dc416-2784-4d63-87a1-c987f7ad57fc", name="Blasting Room"
),
Studio(
pk="708a7531-b1cf-4ff8-a25d-3287fad1bac4", name="JHOC Studio"
),
).run_sync()

studios = Studio.select().run_sync()
response = client.delete(
"/",
params={
"name": f"{studios[0]['name']}",
},
)

studios = Studio.select().run_sync()
self.assertEqual(response.status_code, 204)
self.assertEqual(len(studios), 1)
self.assertEqual(studios[0]["name"], "JHOC Studio")

def test_read_only(self):
"""
In read_only mode, no HTTP verbs should be allowed which modify data.
Expand Down Expand Up @@ -1200,7 +1322,7 @@ def test_malformed_query(self):
response = client.get("/count/", params={"foobar": "1"})
self.assertEqual(response.status_code, 400)

response = client.delete("/", params={"foobar": "1"})
response = client.delete("/", params={"__ids": "1", "foobar": "1"})
self.assertEqual(response.status_code, 400)


Expand Down
2 changes: 1 addition & 1 deletion tests/fastapi/test_fastapi_endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ def test_references(self):

def test_delete(self):
client = TestClient(app)
response = client.delete("/movies/?id=1")
response = client.delete("/movies/1/")
self.assertEqual(response.status_code, 204)
self.assertEqual(response.content, b"")

Expand Down