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

Add support for bulk deletes #2724

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

laurenceisla
Copy link
Member

@laurenceisla laurenceisla commented Mar 28, 2023

Closes #2314

  • Full support for Bulk DELETE
  • Tests
  • Cleaning and refactor
  • Changelog

@laurenceisla laurenceisla mentioned this pull request Mar 28, 2023
5 tasks
@laurenceisla
Copy link
Member Author

This PR changes the logic used for bulk deletes that was specified in #2355 (comment).

As it is mentioned in #2314 (comment), bulk deletes could also use the same header as bulk updates Prefer: params=multiple-objects; unique_keys=id. That would mean the following:

  • Deletes won't detect the body unless that header is used.
  • PostgREST detects only the PK inside the JSON body and delete accordingly.
  • An error is returned when limits are used.

Now, I'm not so sure about how to handle the ?columns query parameter, since it's different for updates and inserts, where they are used to modify/add values. In the previous PR, we used any key in the JSON body as filters and ?columns helped to restrict the body and avoid parsing it. In this case, we could add that behavior: any key in ?columns is added to the filter in the `WHERE.

Although that would get in conflict with the future implementation of the header params: `unique_keys=id.

@steve-chavez
Copy link
Member

Although that would get in conflict with the future implementation of the header params: `unique_keys=id.

Really that was just a loose idea, even the naming is wrong for a header.

So the problem with using ?columns for filters is that while it would work for delete, it won't for update - we need to come up with a consistent interface.

I'd say keep columns working as it is, it shouldn't affect this feature or bulk updates. You can scope it to PKs on the body for now.

If say a pk is not included in columns, that should be validated in a way that's not dependent on columns. I think we discussed how to do this before, similarly to PUT, a query can ensure every row has a PK.

@laurenceisla laurenceisla marked this pull request as ready for review April 4, 2023 11:31
@laurenceisla laurenceisla marked this pull request as draft April 5, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Bulk DELETE, restrict full table DELETE without filters
2 participants