Bulk delete and bulk update

[sinisaos]

New PR related to #11, but also added support for UUID primary keys.

[codecov-commenter]

Codecov Report

Merging #145 (200076d) into master (92ea0ac) will increase coverage by 0.20%.
The diff coverage is 98.14%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #145      +/-   ##
==========================================
+ Coverage   92.55%   92.75%   +0.20%     
==========================================
  Files          33       33              
  Lines        2027     2070      +43     
==========================================
+ Hits         1876     1920      +44     
+ Misses        151      150       -1     

Impacted Files	Coverage Δ
piccolo_api/crud/endpoints.py	95.09% <97.56%> (+0.32%)	⬆️
piccolo_api/crud/validators.py	90.56% <100.00%> (+0.18%)	⬆️
piccolo_api/fastapi/endpoints.py	100.00% <100.00%> (+0.80%)	⬆️

[dantownsend]

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

[sinisaos]

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.

[sinisaos]

@dantownsend I also added a bulk update option. With these changes we can do the proper batch actions also in Piccolo Admin (we need to change some code, it remains to be seen) and in the audit_logs app so we can have batch results as you mentioned. Of course there is also one problem with uuid primary key. As they are quite long (36 characters) we should reduce the page size in Piccolo Admin (e.g. 5,10,25,50) where 50 (or less) is the maximum number of lines because the maximum URL length is 2048. This would reduce the possibility of side effects and URL length exceeding.

UPDATE
Piccolo Admin - it works great with some code changes
Audit logs - it works great with some code changes

I have a one question. Why do we support filter parameters in the delete_bulk method? I know that you would need two queries (first GET to filter rows and then DELETE to delete in bulk) but without that it would be much simpler. Something like this

@apply_validators
async def delete_bulk(
    self, request: Request, rows_ids: str
) -> Response:
    """
    Bulk deletes of rows whose primary keys are in the ``rows_ids``
    query param.
    """
    # Serial or UUID primary keys enabled in query params
    value_type = self.table._meta.primary_key.value_type
    # an exception must be added if the primary key of the table does not 
    # exist in split_rows_ids
    split_rows_ids = rows_ids.split(",")
    ids = [value_type(item) for item in split_rows_ids]

    await self.table.delete().where(
            self.table._meta.primary_key.is_in(ids)
        ).run()
    return Response(status_code=204)

[dantownsend]

@sinisaos The filters are useful in bulk delete, because imagine you wanted to delete all rows where draft=False. You can do that with the filters. It's mostly if Piccolo API is being used standalone - I don't think we use that feature in Piccolo Admin.

The changes look good. I'm a bit behind of reviewing PRs - will try and get on top of it all next week.

[sinisaos]

@dantownsend It would be great if you find time to review and merge these PRs #134, #145, #158, #160 (I think they are good and even if some edge cases occur, we can always fix it later) in the Piccolo API because after that we can make changes in the Piccolo Admin and add a lot of real nice features (I already have most of the Piccolo Admin solutions ready).

[sinisaos]

@dantownsend Can you review and merge #145. After that we can make the changes in #160 and implement both of these new features also in Piccolo Admin?

[dantownsend]

@sinisaos Yeah, I'll try and merge this in this week. I've started reviewing the custom image PR.

[sinisaos]

@dantownsend Just a reminder. If you find time, it would be nice to merge this because after that we can update Piccolo Admin as well, and finish audit logs app.

[dantownsend]

Yeah, I need to merge it in. I need to carefully review it first though, as bulk updates and bulk deletes are very powerful, and we don't want there to be any unforeseen issues.

[sinisaos]

You are right that we should be careful because these are dangerous actions, but these actions have an effect on rows only if we pass primary keys to ids list, otherwise nothing happens.

[dantownsend]

That's a good point - if it's driven by the IDs you pass in, then less chance of accidentally modifying other rows.

[sinisaos]

Yes. Exactly that. I don't think there is any fear of modifying the record if we don't pass that record PK to ids list.

[dantownsend]

This needs tweaking. I think it should be:

if not read_only:
    root_methods.append('POST')
    if allow_bulk_delete:
        root_methods.append('DELETE')
    if allow_bulk_update:
        root_methods.append('PATCH')

At the moment, if allow_bulk_update == True, then it also enables DELETE.

[sinisaos]

You're right. I will change that, but first I need to resolve the conflicts because the email column test and bulk delete and update use the same Studio table but with different columns, since it's been a while since I wrote these tests and there have been changes since then.

[dantownsend]

For bulk update, we're using a parameter called row_ids, and for bulk delete we're using a parameter called __ids. I think it would be better if they're the same (i.e. both are __ids).

[sinisaos]

Can we stick to rows_ids (for both bulk delete and update) as I'm having trouble testing the FastAPI patch bulk in here with the __ids query parameter and I getting error. With rows_ids the test passes without problems.

[dantownsend]

To make __ids work, you might need to modify this so it's DELETE or PATCH:

https://github.com/sinisaos/piccolo_api/blob/6e072d29da05da5eb0284d98cf8c0389198bda89/piccolo_api/fastapi/endpoints.py#L483-L499