From f4becf99ad978b8180d7005cb79ef48d652a13cc Mon Sep 17 00:00:00 2001 From: steve-chavez Date: Sat, 19 Mar 2022 11:46:43 +0100 Subject: [PATCH] feat: add limited update --- CHANGELOG.md | 8 +- src/PostgREST/Query/QueryBuilder.hs | 49 ++++-- src/PostgREST/Request/ApiRequest.hs | 2 +- src/PostgREST/Request/DbRequestBuilder.hs | 6 +- src/PostgREST/Request/Types.hs | 1 + test/spec/Feature/Query/UpdateSpec.hs | 199 ++++++++++++++++++++++ test/spec/fixtures/data.sql | 9 + test/spec/fixtures/privileges.sql | 4 + test/spec/fixtures/schema.sql | 31 ++++ 9 files changed, 291 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 397427bf9..1aa8ad1a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,10 @@ 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 + + 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 ### Fixed @@ -43,6 +47,8 @@ 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 + + Previously, `limit/offset` only limited the returned rows but not the actual updated rows ## [9.0.0] - 2021-11-25 @@ -63,7 +69,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - #2031, Improve error message for ambiguous embedding and add a relevant hint that includes unambiguous embedding suggestions - @laurenceisla - #1917, Add error codes with the `"PGRST"` prefix to the error response body to differentiate PostgREST errors from PostgreSQL errors - @laurenceisla - #1917, Normalize the error response body by always having the `detail` and `hint` error fields with a `null` value if they are empty - @laurenceisla - - #2176, Errors raised with `SQLSTATE` now include the message and the code in the response body - @laurenceisla + - #2176, Errors raised with `SQLSTATE` now include the message and the code in the response body - @laurenceisla ### Fixed diff --git a/src/PostgREST/Query/QueryBuilder.hs b/src/PostgREST/Query/QueryBuilder.hs index 133e149c7..74a0b3c32 100644 --- a/src/PostgREST/Query/QueryBuilder.hs +++ b/src/PostgREST/Query/QueryBuilder.hs @@ -29,6 +29,7 @@ import PostgREST.DbStructure.Table (Table (..)) import PostgREST.Request.Preferences (PreferResolution (..)) import PostgREST.Query.SqlFragment +import PostgREST.RangeQuery (allRange) import PostgREST.Request.Types import Protolude @@ -105,24 +106,44 @@ mutateRequestToQuery (Insert mainQi iCols body onConflct putConditions returning ]) where cols = BS.intercalate ", " $ pgFmtIdent <$> S.toList iCols -mutateRequestToQuery (Update mainQi uCols body logicForest returnings) = - if S.null uCols + +mutateRequestToQuery (Update mainQi uCols body logicForest (range, rangeId) returnings) + | S.null uCols = -- if there are no columns we cannot do UPDATE table SET {empty}, it'd be invalid syntax -- selecting an empty resultset from mainQi gives us the column names to prevent errors when using &select= -- the select has to be based on "returnings" to make computed overloaded functions not throw - then SQL.sql ("SELECT " <> emptyBodyReturnedColumns <> " FROM " <> fromQi mainQi <> " WHERE false") - else - "WITH " <> normalizedBody body <> " " <> - "UPDATE " <> SQL.sql (fromQi mainQi) <> " SET " <> SQL.sql cols <> " " <> - "FROM (SELECT * FROM json_populate_recordset (null::" <> SQL.sql (fromQi mainQi) <> " , " <> SQL.sql selectBody <> " )) _ " <> - (if null logicForest then mempty else "WHERE " <> intercalateSnippet " AND " (pgFmtLogicTree mainQi <$> logicForest)) <> " " <> - SQL.sql (returningF mainQi returnings) + SQL.sql $ "SELECT " <> emptyBodyReturnedColumns <> " FROM " <> fromQi mainQi <> " WHERE false" + + | range == allRange = + "WITH " <> normalizedBody body <> " " <> + "UPDATE " <> mainTbl <> " SET " <> SQL.sql nonRangeCols <> " " <> + "FROM (SELECT * FROM json_populate_recordset (null::" <> mainTbl <> " , " <> SQL.sql selectBody <> " )) _ " <> + whereLogic <> " " <> + SQL.sql (returningF mainQi returnings) + + | otherwise = + "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 <> + whereLogic <> " " <> + "ORDER BY " <> SQL.sql _rangeId <> " " <> limitOffsetF range <> + ") " <> + "UPDATE " <> mainTbl <> " SET " <> SQL.sql rangeCols <> + "FROM pgrst_affected_rows " <> + "WHERE " <> SQL.sql whereRangeId <> " " <> + SQL.sql (returningF mainQi returnings) + where - cols = BS.intercalate ", " (pgFmtIdent <> const " = _." <> pgFmtIdent <$> S.toList uCols) - emptyBodyReturnedColumns :: SqlFragment - emptyBodyReturnedColumns - | null returnings = "NULL" - | otherwise = BS.intercalate ", " (pgFmtColumn (QualifiedIdentifier mempty $ qiName mainQi) <$> returnings) + whereLogic = if null logicForest then mempty else " WHERE " <> intercalateSnippet " AND " (pgFmtLogicTree mainQi <$> logicForest) + mainTbl = SQL.sql (fromQi mainQi) + 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) + mutateRequestToQuery (Delete mainQi logicForest returnings) = "DELETE FROM " <> SQL.sql (fromQi mainQi) <> " " <> (if null logicForest then mempty else "WHERE " <> intercalateSnippet " AND " (map (pgFmtLogicTree mainQi) logicForest)) <> " " <> diff --git a/src/PostgREST/Request/ApiRequest.hs b/src/PostgREST/Request/ApiRequest.hs index 609192df5..3c8da7647 100644 --- a/src/PostgREST/Request/ApiRequest.hs +++ b/src/PostgREST/Request/ApiRequest.hs @@ -151,7 +151,7 @@ targetToJsonRpcParams target params = -} data ApiRequest = ApiRequest { iAction :: Action -- ^ Similar but not identical to HTTP verb, e.g. Create/Invoke both POST - , iRange :: M.HashMap Text NonnegRange -- ^ Requested range of rows within response + , iRange :: M.HashMap Text NonnegRange -- ^ Requested range of rows within response , iTopLevelRange :: NonnegRange -- ^ Requested range of rows from the top level , iTarget :: Target -- ^ The target, be it calling a proc or accessing a table , iPayload :: Maybe Payload -- ^ Data sent by client and used for mutation actions diff --git a/src/PostgREST/Request/DbRequestBuilder.hs b/src/PostgREST/Request/DbRequestBuilder.hs index f9df1b842..d92f7c78f 100644 --- a/src/PostgREST/Request/DbRequestBuilder.hs +++ b/src/PostgREST/Request/DbRequestBuilder.hs @@ -283,7 +283,9 @@ addOrders ApiRequest{..} rReq = addRanges :: ApiRequest -> ReadRequest -> Either ApiRequestError ReadRequest addRanges ApiRequest{..} rReq = - foldr addRangeToNode (Right rReq) =<< ranges + case iAction of + ActionMutate _ -> Right rReq + _ -> foldr addRangeToNode (Right rReq) =<< ranges where ranges :: Either ApiRequestError [(EmbedPath, NonnegRange)] ranges = first QueryParamError $ QueryParams.pRequestRange `traverse` M.toList iRange @@ -319,7 +321,7 @@ mutateRequest mutation schema tName ApiRequest{..} pkCols readReq = mapLeft ApiR case mutation of MutationCreate -> Right $ Insert qi iColumns body ((,) <$> iPreferResolution <*> Just confCols) [] returnings - MutationUpdate -> Right $ Update qi iColumns body combinedLogic returnings + MutationUpdate -> Right $ Update qi iColumns body combinedLogic (iTopLevelRange, pkCols) returnings MutationSingleUpsert -> if null qsLogic && qsFilterFields == S.fromList pkCols && diff --git a/src/PostgREST/Request/Types.hs b/src/PostgREST/Request/Types.hs index a42417119..6abcf3b51 100644 --- a/src/PostgREST/Request/Types.hs +++ b/src/PostgREST/Request/Types.hs @@ -135,6 +135,7 @@ data MutateQuery , updCols :: S.Set FieldName , updBody :: Maybe LBS.ByteString , where_ :: [LogicTree] + , mutRange :: (NonnegRange, [FieldName]) , returning :: [FieldName] } | Delete diff --git a/test/spec/Feature/Query/UpdateSpec.hs b/test/spec/Feature/Query/UpdateSpec.hs index 842cbda12..03599d0e2 100644 --- a/test/spec/Feature/Query/UpdateSpec.hs +++ b/test/spec/Feature/Query/UpdateSpec.hs @@ -386,3 +386,202 @@ spec = do { matchStatus = 204 , matchHeaders = [matchHeaderAbsent hContentType] } + + context "limited update" $ do + it "works with the limit query param" $ do + get "/limited_update_items" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 2, "name": "item-2" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodPatch "/limited_update_items?limit=2" + [("Prefer", "tx=commit")] + [json| {"name": "updated-item"} |] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/limited_update_items?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "updated-item" } + , { "id": 2, "name": "updated-item" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodPost "/rpc/reset_limited_items" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "limited_update_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "works with the limit query param plus a filter" $ do + get "/limited_update_items" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 2, "name": "item-2" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodPatch "/limited_update_items?limit=1&id=gt.2" + [("Prefer", "tx=commit")] + [json| {"name": "updated-item"} |] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/limited_update_items?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 2, "name": "item-2" } + , { "id": 3, "name": "updated-item" } + ]|] + + request methodPost "/rpc/reset_limited_items" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "limited_update_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "works with the limit and offset query params" $ do + get "/limited_update_items" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 2, "name": "item-2" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodPatch "/limited_update_items?limit=1&offset=1" + [("Prefer", "tx=commit")] + [json| {"name": "updated-item"} |] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/limited_update_items?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 2, "name": "updated-item" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodPost "/rpc/reset_limited_items" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "limited_update_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "works on a table with a composite pk" $ do + get "/limited_update_items_cpk" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 2, "name": "item-2" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodPatch "/limited_update_items_cpk?limit=1&offset=1" + [("Prefer", "tx=commit")] + [json| {"name": "updated-item"} |] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/limited_update_items_cpk?order=id,name" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 2, "name": "updated-item" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodPost "/rpc/reset_limited_items" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "limited_update_items_cpk"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "works with views with an inferred pk" $ do + get "/limited_update_items_view" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 2, "name": "item-2" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodPatch "/limited_update_items_view?limit=1&offset=1" + [("Prefer", "tx=commit")] + [json| {"name": "updated-item"} |] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/limited_update_items_view?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 2, "name": "updated-item" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodPost "/rpc/reset_limited_items" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "limited_update_items_view"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "works on a table without a pk" $ do + get "/limited_update_items_no_pk" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1" } + , { "id": 2, "name": "item-2" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodPatch "/limited_update_items_no_pk?limit=1" + [("Prefer", "tx=commit")] + [json| {"name": "updated-item"} |] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/limited_update_items_no_pk?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "updated-item" } + , { "id": 2, "name": "item-2" } + , { "id": 3, "name": "item-3" } + ]|] + + request methodPost "/rpc/reset_limited_items" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "limited_update_items_no_pk"} |] + `shouldRespondWith` "" + { matchStatus = 204 } diff --git a/test/spec/fixtures/data.sql b/test/spec/fixtures/data.sql index b6af72797..1f62599c0 100644 --- a/test/spec/fixtures/data.sql +++ b/test/spec/fixtures/data.sql @@ -736,3 +736,12 @@ 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_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_updated_items_no_pk CASCADE; +INSERT INTO test.limited_updated_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 3d4a3b585..8685493cc 100644 --- a/test/spec/fixtures/privileges.sql +++ b/test/spec/fixtures/privileges.sql @@ -163,6 +163,10 @@ GRANT ALL ON TABLE , clientinfo , contact , chores + , limited_mut_items + , limited_mut_items_cpk + , limited_mut_items_no_pk + , limited_mut_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 e52b56348..4b43e3354 100644 --- a/test/spec/fixtures/schema.sql +++ b/test/spec/fixtures/schema.sql @@ -2465,3 +2465,34 @@ CREATE AGGREGATE test.unsupported_agg (*) ( SFUNC = public.dummy, STYPE = int ); + +create view no_pk_view as +select * from no_pk; + +create table limited_update_items( + id int primary key +, name text +); + +create table limited_update_items_cpk( + id int +, name text +, primary key (id, name) +); + +create table limited_update_items_no_pk( + id int +, name text +); + +create view limited_update_items_view as +select * from limited_update_items; + +create function reset_limited_items(tbl_name text default '') returns void as $_$ begin + execute format( + $$ + delete from %I; + insert into %I values (1, 'item-1'), (2, 'item-2'), (3, 'item-3'); + $$::text, + tbl_name, tbl_name); +end; $_$ language plpgsql volatile;