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

Conversation

sinisaos
Copy link
Member

@sinisaos sinisaos commented Jun 4, 2022

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

@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2022

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%) ⬆️

Comment on lines 842 to 844
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.

@sinisaos
Copy link
Member Author

sinisaos commented Jun 30, 2022

@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

admin_actions

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)

@sinisaos sinisaos changed the title Added deletion in bulk Bulk delete and bulk update Jun 30, 2022
@dantownsend
Copy link
Member

@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
Copy link
Member Author

sinisaos commented Jul 1, 2022

@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
Copy link
Member Author

@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
Copy link
Member

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

@sinisaos
Copy link
Member Author

sinisaos commented Sep 9, 2022

@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
Copy link
Member

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
Copy link
Member Author

sinisaos commented Sep 9, 2022

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
Copy link
Member

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

@sinisaos
Copy link
Member Author

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.

Comment on lines 240 to 242
["POST", "DELETE", "PATCH"]
if allow_bulk_delete or allow_bulk_update
else ["POST"]
Copy link
Member

@dantownsend dantownsend Oct 3, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Comment on lines 299 to 300
If you pass a wrong or non-existent value to the query parameters ``rows_ids``,
no record will be changed and api response will be empty list.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@dantownsend dantownsend Oct 3, 2022

Choose a reason for hiding this comment

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

@sinisaos sinisaos closed this Sep 1, 2023
@sinisaos sinisaos deleted the bulk_delete branch September 1, 2023 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants