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%.

:exclamation: 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%)	:arrow_up:
piccolo_api/crud/validators.py	90.56% <100.00%> (+0.18%)	:arrow_up:
piccolo_api/fastapi/endpoints.py	100.00% <100.00%> (+0.80%)	:arrow_up:

[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.