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-delete #2355

Closed
wants to merge 18 commits into from

Conversation

laurenceisla
Copy link
Member

@laurenceisla laurenceisla commented Jul 1, 2022

Closes #2314

  • Full support for bulk-delete
  • Modify error message from 204 to 422 when body is missing/empty
  • Tests
  • Refactor
  • Changelog

After much deliberation, the changes implemented are as follows:

  • Deletes now allow filters (equality only) in the body. That is:
    DELETE /table
    {"col1": "val1", "col2": "val2"}
    Gives the same result as:
    DELETE /table?col1=eq.val1&col2=eq.val2
  • Bulk deletes behave in the same way (body used as filters in the WHERE clause), differentiating themselves from the behaviors with PATCH (body used for values in the SET clause) or POST (body used for values to be inserted).
  • Using ?columns circumvent the parsing of the json body, which verifies that all the objects have the same keys, and uses only the specified columns as filters in the query.
  • DELETE without filters/body/limits/columns (full table delete) is not allowed and throw an HTTP 422 error.
  • If there are filters or limits specified in the query string, then the body will be ignored (for both single object and bulk DELETE).

src/PostgREST/App.hs Outdated Show resolved Hide resolved
@steve-chavez
Copy link
Member

I think we can close #699 once this is merged(and continue the cancel request part on #2352)

@laurenceisla laurenceisla reopened this Jul 5, 2022
@laurenceisla laurenceisla force-pushed the bulk-delete branch 2 times, most recently from 6f3390b to 78b39a5 Compare July 7, 2022 00:43
@laurenceisla laurenceisla marked this pull request as ready for review July 7, 2022 00:59
@steve-chavez
Copy link
Member

steve-chavez commented Jul 7, 2022

As it is, one deficiency of the new bulk delete(and bulk update suffers from something similar too) is that it doesn't fail with an appropriate message when no filters or body are specified, it just returns 204. This is desirable as discussed on the issues, but it is surprising for someone that hasn't read our docs.

Do you think it would be possible to respond with a 422 when no filters, limits or body are present?


For when a body is present, it might be possible to also count the pks in the body on the query and then verify this count is > 0 to fail with the same error(this could be adopted for PATCH too).

@laurenceisla laurenceisla marked this pull request as draft July 8, 2022 16:19
@laurenceisla laurenceisla force-pushed the bulk-delete branch 3 times, most recently from ad1be43 to 0b2d13d Compare July 20, 2022 17:55
whereLogic = if null logicForest then mempty else " WHERE " <> intercalateSnippet " AND " (pgFmtLogicTree mainQi <$> logicForest)
hasEmptyBody = body == mempty
logicForestF = intercalateSnippet " AND " (pgFmtLogicTree mainQi <$> logicForest)
pgrstDeleteBodyF = SQL.sql (BS.intercalate " AND " $ (\x -> pgFmtColumn mainQi x <> " = " <> pgFmtColumn (QualifiedIdentifier mempty "pgrst_delete_body") x) <$> pkFlts)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about the use of columns in the query string with DELETE. For POST and PATCH it's used as a way to select data from the body to be inserted and updated (in the SET clause). But for DELETE it would be used to select PKs from the body to be used in the WHERE clause, which is redundant since it already filters using the PKs (as seen in this line). I can see these options:

  • Do not use columns (but it won't have an option to use the body "as is", without JSON parsing)
  • Use it and filter only PKs (will need a warning "Only PKs allowed" if other columns are selected)
  • Could be used as a way to add more filters in the body.

Copy link
Member

Choose a reason for hiding this comment

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

Could be used as a way to add more filters in the body.

Hm, this would be useful but it would be inconsistent with bulk PATCH since columns couldn't do the same there.

Use it and filter only PKs (will need a warning "Only PKs allowed" if other columns are selected)

I think this would be the best way for now. Also leaves room for an enhancement later without a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

After deliberation on another platform, we agreed on using the third option for DELETE:

Could be used as a way to add more filters in the body

Works for single and bulk deletes.

@laurenceisla laurenceisla marked this pull request as ready for review July 22, 2022 04:05
@laurenceisla laurenceisla force-pushed the bulk-delete branch 2 times, most recently from fecfa21 to a8de57f Compare July 28, 2022 03:11
@@ -342,6 +345,7 @@ mutateRequest mutation schema tName ApiRequest{..} pkCols readReq = mapLeft ApiR
rootOrder = maybe [] snd $ find (\(x, _) -> null x) qsOrder
combinedLogic = foldr addFilterToLogicForest logic qsFiltersRoot
body = payRaw <$> iPayload -- the body is assumed to be json at this stage(ApiRequest validates)
noBodyFilterLimit = null qsColumns && null qsLogic && null qsFilters && null qsRanges && (isNothing body || S.null iColumns)
Copy link
Member

Choose a reason for hiding this comment

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

This check would be better at the ApiRequest level since it doesn't depend on the db. This way we avoid taking a pool connection and opening an empty transaction when this error happens.

@steve-chavez steve-chavez mentioned this pull request Jul 30, 2022
1 task
@steve-chavez
Copy link
Member

Hm, I've been thinking that this one could also be done with underscore operators. Like:

DELETE /tbl?id=_in.body->ids

{ "ids": [1,2,3,4]}

I guess one problem with that approach would be how to compare two values that are on the same json object, which this PR already does. For underscore operators, I'm not sure if this would be possible:

DELETE /tbl?col1=_eq.body->*->col1&col2=_eq.body->*->col2


[
  {"col1": "val1", "col2": "val2"}
, {"col1": "val3", "col2": "val4"}
]

The * is borrowed from JSON path's [*] which means get keys from all array elements.

@laurenceisla
Copy link
Member Author

Closed in favor of #2724

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