From 331e88ea3936dcdf6b949006d23ae9b171dfb33c Mon Sep 17 00:00:00 2001 From: steve-chavez Date: Tue, 22 Mar 2022 13:09:10 +0100 Subject: [PATCH] feat: add limited delete --- CHANGELOG.md | 4 +- src/PostgREST/Query/QueryBuilder.hs | 35 +++-- src/PostgREST/Query/SqlFragment.hs | 10 ++ src/PostgREST/Request/DbRequestBuilder.hs | 2 +- src/PostgREST/Request/Types.hs | 1 + test/spec/Feature/Query/DeleteSpec.hs | 161 ++++++++++++++++++++++ test/spec/fixtures/data.sql | 21 ++- test/spec/fixtures/privileges.sql | 12 +- test/spec/fixtures/schema.sql | 19 +++ 9 files changed, 242 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1aa8ad1a9..60d3b9861 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). + #1689, Add the ability to run without `db-anon-role` disabling anonymous access. - @wolfgangwalther - #1543, Allow access to fields of composite types in select=, order= and filters through JSON operators -> and ->>. - @wolfgangwalther - #2075, Allow access to array items in ?select=, ?order= and filters through JSON operators -> and ->>. - @wolfgangwalther - - #2156, Allow applying `limit/offset` to UPDATE to only affect a subset of rows - @steve-chavez + - #2156, Allow applying `limit/offset` to UPDATE/DELETE to only affect a subset of rows - @steve-chavez + Uses the table primary key, so it needs a select privilege on the primary key columns + If no primary key is available, it will fallback to using the "ctid" system column(will also require a select privilege on it) + Will work on views if the PK(or "ctid") is present on its SELECT clause @@ -47,7 +47,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - #2001, Return 204 No Content without Content-Type for RPCs returning VOID - @wolfgangwalther + Previously, those RPCs would return "null" as a body with Content-Type: application/json. - - #2156, `limit/offset` now limits the affected rows on UPDATE - @steve-chavez + - #2156, `limit/offset` now limits the affected rows on UPDATE/DELETE - @steve-chavez + Previously, `limit/offset` only limited the returned rows but not the actual updated rows ## [9.0.0] - 2021-11-25 diff --git a/src/PostgREST/Query/QueryBuilder.hs b/src/PostgREST/Query/QueryBuilder.hs index 74a0b3c32..2f1d2668d 100644 --- a/src/PostgREST/Query/QueryBuilder.hs +++ b/src/PostgREST/Query/QueryBuilder.hs @@ -125,13 +125,13 @@ mutateRequestToQuery (Update mainQi uCols body logicForest (range, rangeId) retu "WITH " <> normalizedBody body <> ", " <> "pgrst_update_body AS (SELECT * FROM json_populate_recordset (null::" <> mainTbl <> " , " <> SQL.sql selectBody <> " ) LIMIT 1), " <> "pgrst_affected_rows AS (" <> - "SELECT " <> SQL.sql _rangeId <> " FROM " <> mainTbl <> + "SELECT " <> SQL.sql rangeIdF <> " FROM " <> mainTbl <> whereLogic <> " " <> - "ORDER BY " <> SQL.sql _rangeId <> " " <> limitOffsetF range <> + "ORDER BY " <> SQL.sql rangeIdF <> " " <> limitOffsetF range <> ") " <> "UPDATE " <> mainTbl <> " SET " <> SQL.sql rangeCols <> "FROM pgrst_affected_rows " <> - "WHERE " <> SQL.sql whereRangeId <> " " <> + "WHERE " <> SQL.sql whereRangeIdF <> " " <> SQL.sql (returningF mainQi returnings) where @@ -140,14 +140,29 @@ mutateRequestToQuery (Update mainQi uCols body logicForest (range, rangeId) retu emptyBodyReturnedColumns = if null returnings then "NULL" else BS.intercalate ", " (pgFmtColumn (QualifiedIdentifier mempty $ qiName mainQi) <$> returnings) nonRangeCols = BS.intercalate ", " (pgFmtIdent <> const " = _." <> pgFmtIdent <$> S.toList uCols) rangeCols = BS.intercalate ", " ((\col -> pgFmtIdent col <> " = (SELECT " <> pgFmtIdent col <> " FROM pgrst_update_body) ") <$> S.toList uCols) - _rangeId = if null rangeId then pgFmtColumn mainQi "ctid" else BS.intercalate ", " (pgFmtColumn mainQi <$> rangeId) - whereRangeId = BS.intercalate " AND " $ - (\col -> pgFmtColumn mainQi col <> " = " <> pgFmtColumn (QualifiedIdentifier mempty "pgrst_affected_rows") col) <$> (if null rangeId then ["ctid"] else rangeId) + (whereRangeIdF, rangeIdF) = mutRangeF mainQi rangeId -mutateRequestToQuery (Delete mainQi logicForest returnings) = - "DELETE FROM " <> SQL.sql (fromQi mainQi) <> " " <> - (if null logicForest then mempty else "WHERE " <> intercalateSnippet " AND " (map (pgFmtLogicTree mainQi) logicForest)) <> " " <> - SQL.sql (returningF mainQi returnings) +mutateRequestToQuery (Delete mainQi logicForest (range, rangeId) returnings) + | range == allRange = + "DELETE FROM " <> SQL.sql (fromQi mainQi) <> " " <> + whereLogic <> " " <> + SQL.sql (returningF mainQi returnings) + + | otherwise = + "WITH " <> + "pgrst_affected_rows AS (" <> + "SELECT " <> SQL.sql rangeIdF <> " FROM " <> SQL.sql (fromQi mainQi) <> + whereLogic <> " " <> + "ORDER BY " <> SQL.sql rangeIdF <> " " <> limitOffsetF range <> + ") " <> + "DELETE FROM " <> SQL.sql (fromQi mainQi) <> " " <> + "USING pgrst_affected_rows " <> + "WHERE " <> SQL.sql whereRangeIdF <> " " <> + SQL.sql (returningF mainQi returnings) + + where + whereLogic = if null logicForest then mempty else " WHERE " <> intercalateSnippet " AND " (pgFmtLogicTree mainQi <$> logicForest) + (whereRangeIdF, rangeIdF) = mutRangeF mainQi rangeId requestToCallProcQuery :: CallRequest -> SQL.Snippet requestToCallProcQuery (FunctionCall qi params args returnsScalar multipleCall returnings) = diff --git a/src/PostgREST/Query/SqlFragment.hs b/src/PostgREST/Query/SqlFragment.hs index 32eda78bf..0c275f9ff 100644 --- a/src/PostgREST/Query/SqlFragment.hs +++ b/src/PostgREST/Query/SqlFragment.hs @@ -17,6 +17,7 @@ module PostgREST.Query.SqlFragment , fromQi , limitOffsetF , locationF + , mutRangeF , normalizedBody , pgFmtColumn , pgFmtIdent @@ -334,3 +335,12 @@ unknownLiteral = unknownEncoder . encodeUtf8 intercalateSnippet :: ByteString -> [SQL.Snippet] -> SQL.Snippet intercalateSnippet _ [] = mempty intercalateSnippet frag snippets = foldr1 (\a b -> a <> SQL.sql frag <> b) snippets + +-- the "ctid" system column is always available to tables +mutRangeF :: QualifiedIdentifier -> [FieldName] -> (SqlFragment, SqlFragment) +mutRangeF mainQi rangeId = ( + BS.intercalate " AND " $ + (\col -> pgFmtColumn mainQi col <> " = " <> pgFmtColumn (QualifiedIdentifier mempty "pgrst_affected_rows") col) <$> + (if null rangeId then ["ctid"] else rangeId) + , if null rangeId then pgFmtColumn mainQi "ctid" else BS.intercalate ", " (pgFmtColumn mainQi <$> rangeId) + ) diff --git a/src/PostgREST/Request/DbRequestBuilder.hs b/src/PostgREST/Request/DbRequestBuilder.hs index d92f7c78f..428069a2f 100644 --- a/src/PostgREST/Request/DbRequestBuilder.hs +++ b/src/PostgREST/Request/DbRequestBuilder.hs @@ -332,7 +332,7 @@ mutateRequest mutation schema tName ApiRequest{..} pkCols readReq = mapLeft ApiR then Right $ Insert qi iColumns body (Just (MergeDuplicates, pkCols)) combinedLogic returnings else Left InvalidFilters - MutationDelete -> Right $ Delete qi combinedLogic returnings + MutationDelete -> Right $ Delete qi combinedLogic (iTopLevelRange, pkCols) returnings where confCols = fromMaybe pkCols qsOnConflict QueryParams.QueryParams{..} = iQueryParams diff --git a/src/PostgREST/Request/Types.hs b/src/PostgREST/Request/Types.hs index 6abcf3b51..4506aab84 100644 --- a/src/PostgREST/Request/Types.hs +++ b/src/PostgREST/Request/Types.hs @@ -141,6 +141,7 @@ data MutateQuery | Delete { in_ :: QualifiedIdentifier , where_ :: [LogicTree] + , mutRange :: (NonnegRange, [FieldName]) , returning :: [FieldName] } diff --git a/test/spec/Feature/Query/DeleteSpec.hs b/test/spec/Feature/Query/DeleteSpec.hs index 9c31d7a88..f4a338785 100644 --- a/test/spec/Feature/Query/DeleteSpec.hs +++ b/test/spec/Feature/Query/DeleteSpec.hs @@ -115,3 +115,164 @@ spec = { matchStatus = 204 , matchHeaders = [matchHeaderAbsent hContentType] } + + context "limited delete" $ do + it "works with the limit and offset query params" $ do + get "/limited_delete_items" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 2, "name": "item-2" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodDelete "/limited_delete_items?limit=1&offset=1" + [("Prefer", "tx=commit")] + mempty + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/limited_delete_items?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodPost "/rpc/reset_limited_items" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "limited_delete_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "works with the limit query param plus a filter" $ do + get "/limited_delete_items" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 2, "name": "item-2" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodDelete "/limited_delete_items?limit=1&id=gt.1" + [("Prefer", "tx=commit")] + mempty + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/limited_delete_items?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodPost "/rpc/reset_limited_items" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "limited_delete_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "works on a table with a composite pk" $ do + get "/limited_delete_items_cpk" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 2, "name": "item-2" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodDelete "/limited_delete_items_cpk?limit=1&offset=1" + [("Prefer", "tx=commit")] + mempty + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/limited_delete_items_cpk" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodPost "/rpc/reset_limited_items" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "limited_delete_items_cpk"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "works with views with an inferred pk" $ do + get "/limited_delete_items_view" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 2, "name": "item-2" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodDelete "/limited_delete_items_view?limit=1&offset=1" + [("Prefer", "tx=commit")] + mempty + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/limited_delete_items_view" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodPost "/rpc/reset_limited_items" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "limited_delete_items_view"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "works on a table without a pk" $ do + get "/limited_delete_items_no_pk" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 2, "name": "item-2" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodDelete "/limited_delete_items_no_pk?limit=1&offset=1" + [("Prefer", "tx=commit")] + mempty + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/limited_delete_items_no_pk" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodPost "/rpc/reset_limited_items" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "limited_delete_items_no_pk"} |] + `shouldRespondWith` "" + { matchStatus = 204 } diff --git a/test/spec/fixtures/data.sql b/test/spec/fixtures/data.sql index 1f62599c0..b5444c299 100644 --- a/test/spec/fixtures/data.sql +++ b/test/spec/fixtures/data.sql @@ -737,11 +737,20 @@ INSERT INTO test.fav_numbers VALUES (ROW(0.5, 0.5), 'A'), (ROW(0.6, 0.6), 'B'); TRUNCATE TABLE test.arrays CASCADE; INSERT INTO test.arrays VALUES (0, '{1,2,3}', '{{1,2,3},{4,5,6},{7,8,9}}'), (1, '{11,12,13}', '{{11,12,13},{14,15,16},{17,18,19}}'); -TRUNCATE TABLE test.limited_updated_items CASCADE; -INSERT INTO test.limited_updated_items VALUES (1, 'item-1'), (2, 'item-2'), (3, 'item-3'); +TRUNCATE TABLE test.limited_update_items CASCADE; +INSERT INTO test.limited_update_items VALUES (1, 'item-1'), (2, 'item-2'), (3, 'item-3'); -TRUNCATE TABLE test.limited_updated_items_cpk CASCADE; -INSERT INTO test.limited_updated_items_cpk VALUES (1, 'item-1'), (2, 'item-2'), (3, 'item-3'); +TRUNCATE TABLE test.limited_update_items_cpk CASCADE; +INSERT INTO test.limited_update_items_cpk VALUES (1, 'item-1'), (2, 'item-2'), (3, 'item-3'); -TRUNCATE TABLE test.limited_updated_items_no_pk CASCADE; -INSERT INTO test.limited_updated_items_no_pk VALUES (1, 'item-1'), (2, 'item-2'), (3, 'item-3'); +TRUNCATE TABLE test.limited_update_items_no_pk CASCADE; +INSERT INTO test.limited_update_items_no_pk VALUES (1, 'item-1'), (2, 'item-2'), (3, 'item-3'); + +TRUNCATE TABLE test.limited_delete_items CASCADE; +INSERT INTO test.limited_delete_items VALUES (1, 'item-1'), (2, 'item-2'), (3, 'item-3'); + +TRUNCATE TABLE test.limited_delete_items_cpk CASCADE; +INSERT INTO test.limited_delete_items_cpk VALUES (1, 'item-1'), (2, 'item-2'), (3, 'item-3'); + +TRUNCATE TABLE test.limited_delete_items_no_pk CASCADE; +INSERT INTO test.limited_delete_items_no_pk VALUES (1, 'item-1'), (2, 'item-2'), (3, 'item-3'); diff --git a/test/spec/fixtures/privileges.sql b/test/spec/fixtures/privileges.sql index 8685493cc..b0926d99d 100644 --- a/test/spec/fixtures/privileges.sql +++ b/test/spec/fixtures/privileges.sql @@ -163,10 +163,14 @@ GRANT ALL ON TABLE , clientinfo , contact , chores - , limited_mut_items - , limited_mut_items_cpk - , limited_mut_items_no_pk - , limited_mut_items_view + , limited_update_items + , limited_update_items_cpk + , limited_update_items_no_pk + , limited_update_items_view + , limited_delete_items + , limited_delete_items_cpk + , limited_delete_items_no_pk + , limited_delete_items_view TO postgrest_test_anonymous; GRANT INSERT ON TABLE insertonly TO postgrest_test_anonymous; diff --git a/test/spec/fixtures/schema.sql b/test/spec/fixtures/schema.sql index 4b43e3354..c0d7d0b23 100644 --- a/test/spec/fixtures/schema.sql +++ b/test/spec/fixtures/schema.sql @@ -2488,6 +2488,25 @@ create table limited_update_items_no_pk( create view limited_update_items_view as select * from limited_update_items; +create table limited_delete_items( + id int primary key +, name text +); + +create table limited_delete_items_cpk( + id int +, name text +, primary key (id, name) +); + +create table limited_delete_items_no_pk( + id int +, name text +); + +create view limited_delete_items_view as +select * from limited_delete_items; + create function reset_limited_items(tbl_name text default '') returns void as $_$ begin execute format( $$