From 29a0c7b7804fd1584dd4f85e610b056c1794cee8 Mon Sep 17 00:00:00 2001 From: Laurence Isla Date: Mon, 8 Aug 2022 20:08:17 -0500 Subject: [PATCH 1/8] Allow full table update when no filters are specified in the query string --- src/PostgREST/Query/QueryBuilder.hs | 7 +- test/spec/Feature/Query/QueryLimitedSpec.hs | 9 +- test/spec/Feature/Query/UpdateSpec.hs | 214 +++++++++++++++++++- 3 files changed, 221 insertions(+), 9 deletions(-) diff --git a/src/PostgREST/Query/QueryBuilder.hs b/src/PostgREST/Query/QueryBuilder.hs index 9ae3d08f2f..d992830dcb 100644 --- a/src/PostgREST/Query/QueryBuilder.hs +++ b/src/PostgREST/Query/QueryBuilder.hs @@ -111,12 +111,12 @@ mutateRequestToQuery (Update mainQi uCols body logicForest pkFlts range ordts re SQL.sql $ "SELECT " <> emptyBodyReturnedColumns <> " FROM " <> fromQi mainQi <> " WHERE false" | range == allRange = - let whereLogic | null logicForest = if null pkFlts then "FALSE" else pgrstUpdateBodyF - | otherwise = logicForestF in + let whereLogic | null logicForest = if null pkFlts || not colsHavePk then mempty else " WHERE " <> pgrstUpdateBodyF + | otherwise = " WHERE " <> logicForestF in "WITH " <> normalizedBody body <> " " <> "UPDATE " <> mainTbl <> " SET " <> SQL.sql nonRangeCols <> " " <> "FROM (SELECT * FROM json_populate_recordset (null::" <> mainTbl <> " , " <> SQL.sql selectBody <> " )) pgrst_update_body " <> - "WHERE " <> whereLogic <> " " <> + whereLogic <> " " <> SQL.sql (returningF mainQi returnings) | otherwise = @@ -137,6 +137,7 @@ mutateRequestToQuery (Update mainQi uCols body logicForest pkFlts range ordts re where mainTbl = SQL.sql (fromQi mainQi) + colsHavePk = all (`elem` uCols) pkFlts logicForestF = intercalateSnippet " AND " (pgFmtLogicTree mainQi <$> logicForest) pgrstUpdateBodyF = SQL.sql (BS.intercalate " AND " $ (\x -> pgFmtColumn mainQi x <> " = " <> pgFmtColumn (QualifiedIdentifier mempty "pgrst_update_body") x) <$> pkFlts) emptyBodyReturnedColumns = if null returnings then "NULL" else BS.intercalate ", " (pgFmtColumn (QualifiedIdentifier mempty $ qiName mainQi) <$> returnings) diff --git a/test/spec/Feature/Query/QueryLimitedSpec.hs b/test/spec/Feature/Query/QueryLimitedSpec.hs index 46f5e38ef2..addca7907c 100644 --- a/test/spec/Feature/Query/QueryLimitedSpec.hs +++ b/test/spec/Feature/Query/QueryLimitedSpec.hs @@ -93,13 +93,16 @@ spec = { "id": 8, "name": "HaikuOS" } ]|] { matchStatus = 201 } - it "doesn't affect updates(2 rows would be modified if it did)" $ + it "doesn't affect updates" $ request methodPatch "/employees?select=first_name,last_name,occupation" [("Prefer", "return=representation")] [json| [{"occupation": "Barista"}] |] `shouldRespondWith` - [json|[]|] - { matchStatus = 404 } + [json|[ + { "first_name": "Frances M.", "last_name": "Roe", "occupation": "Barista" }, + { "first_name": "Daniel B.", "last_name": "Lyon", "occupation": "Barista" }, + { "first_name": "Edwin S.", "last_name": "Smith", "occupation": "Barista" } ]|] + { matchStatus = 200 } it "doesn't affect deletions" $ request methodDelete "/employees?select=first_name,last_name" diff --git a/test/spec/Feature/Query/UpdateSpec.hs b/test/spec/Feature/Query/UpdateSpec.hs index 32a327e428..e6be9af628 100644 --- a/test/spec/Feature/Query/UpdateSpec.hs +++ b/test/spec/Feature/Query/UpdateSpec.hs @@ -388,17 +388,40 @@ spec = do } context "limited update" $ do - it "does not work when no limit query or filter is given" $ + it "updates the whole table when no limit query or filter is given" $ 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" [("Prefer", "tx=commit"), ("Prefer", "count=exact")] [json| {"name": "updated-item"} |] `shouldRespondWith` "" - { matchStatus = 404 + { matchStatus = 204 , matchHeaders = [ matchHeaderAbsent hContentType - , "Content-Range" <:> "*/0" ] + , "Content-Range" <:> "0-2/3" + , "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": "updated-item" } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "limited_update_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + it "works with the limit query param" $ do get "/limited_update_items" `shouldRespondWith` @@ -700,6 +723,80 @@ spec = do `shouldRespondWith` "" { matchStatus = 204 } + it "updates with pk in ?columns" $ do + get "/bulk_update_items" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodPatch "/bulk_update_items?columns=id,observation" + [("Prefer", "tx=commit")] + [json|[ + { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } + , { "id": 3, "name": "item-3 - 3rd", "observation": "New item" } + ]|] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Content-Range" <:> "0-1/*" + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/bulk_update_items?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": "Lost item" } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": "New item" } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "bulk_update_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "updates with composite pk in ?columns" $ do + get "/bulk_update_items_cpk" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodPatch "/bulk_update_items_cpk?columns=id,name,observation" + [("Prefer", "tx=commit")] + [json|[ + { "id": 1, "name": "item-1", "observation": "Lost item" } + , { "id": 3, "name": "item-3", "observation": "New item" } + ]|] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Content-Range" <:> "0-1/*" + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/bulk_update_items_cpk?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": "Lost item" } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": "New item" } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "bulk_update_items_cpk"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + it "updates with filters taking only the first item in the json array body" $ do get "/bulk_update_items" `shouldRespondWith` @@ -774,6 +871,117 @@ spec = do `shouldRespondWith` "" { matchStatus = 204 } + it "updates the whole table wihtout filters, taking only the first item in the json array body" $ do + get "/bulk_update_items" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodPatch "/bulk_update_items" + [("Prefer", "tx=commit")] + [json|[ + { "name": "item-all", "observation": "Damaged item" } + , { "name": "item-3 - 3rd", "observation": null } + ]|] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Content-Range" <:> "0-2/*" + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/bulk_update_items?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-all", "observation": "Damaged item" } + , { "id": 2, "name": "item-all", "observation": "Damaged item" } + , { "id": 3, "name": "item-all", "observation": "Damaged item" } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "bulk_update_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "updates the whole table without filters and no pk in ?columns, taking only the first item in the json array body" $ do + get "/bulk_update_items" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodPatch "/bulk_update_items?columns=observation" + [("Prefer", "tx=commit")] + [json|[ + { "id": 1, "name": "item-4", "observation": "Damaged item" } + , { "id": 3, "name": "item-3 - 3rd", "observation": null } + ]|] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Content-Range" <:> "0-2/*" + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/bulk_update_items?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": "Damaged item" } + , { "id": 2, "name": "item-2", "observation": "Damaged item" } + , { "id": 3, "name": "item-3", "observation": "Damaged item" } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "bulk_update_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "updates the whole table without filters and no composite pk in ?columns, taking only the first item in the json array body" $ do + get "/bulk_update_items_cpk" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodPatch "/bulk_update_items_cpk?columns=id,observation" + [("Prefer", "tx=commit")] + [json|[ + { "id": 1, "name": "item-4", "observation": "Damaged item" } + , { "id": 3, "name": "item-3 - 3rd", "observation": null } + ]|] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Content-Range" <:> "0-2/*" + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/bulk_update_items_cpk?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": "Damaged item" } + , { "id": 1, "name": "item-2", "observation": "Damaged item" } + , { "id": 1, "name": "item-3", "observation": "Damaged item" } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "bulk_update_items_cpk"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + it "rejects a json array that isn't exclusively composed of objects" $ request methodPatch "/bulk_update_items" [("Prefer", "tx=commit")] From bc91c1de75c138d284cb73a51a707a6a6a84da67 Mon Sep 17 00:00:00 2001 From: Laurence Isla Date: Tue, 9 Aug 2022 12:53:34 -0500 Subject: [PATCH 2/8] Add no pk table test --- test/spec/Feature/Query/UpdateSpec.hs | 37 +++++++++++++++++++++++++++ test/spec/fixtures/data.sql | 3 +++ test/spec/fixtures/privileges.sql | 1 + test/spec/fixtures/schema.sql | 6 +++++ 4 files changed, 47 insertions(+) diff --git a/test/spec/Feature/Query/UpdateSpec.hs b/test/spec/Feature/Query/UpdateSpec.hs index e6be9af628..43e0e166e7 100644 --- a/test/spec/Feature/Query/UpdateSpec.hs +++ b/test/spec/Feature/Query/UpdateSpec.hs @@ -982,6 +982,43 @@ spec = do `shouldRespondWith` "" { matchStatus = 204 } + it "updates the whole table with no pk defined in it" $ do + get "/bulk_update_items_no_pk" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodPatch "/bulk_update_items_no_pk" + [("Prefer", "tx=commit")] + [json|[ + { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } + , { "id": 3, "name": "item-3 - 3rd", "observation": null } + ]|] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Content-Range" <:> "0-1/*" + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/bulk_update_items_no_pk?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } + , { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } + , { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "bulk_update_items_no_pk"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + it "rejects a json array that isn't exclusively composed of objects" $ request methodPatch "/bulk_update_items" [("Prefer", "tx=commit")] diff --git a/test/spec/fixtures/data.sql b/test/spec/fixtures/data.sql index 1082133741..7d16b83530 100644 --- a/test/spec/fixtures/data.sql +++ b/test/spec/fixtures/data.sql @@ -787,6 +787,9 @@ INSERT INTO test.bulk_update_items (id, name, observation) VALUES (1, 'item-1', TRUNCATE TABLE test.bulk_update_items_cpk CASCADE; INSERT INTO test.bulk_update_items_cpk (id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); +TRUNCATE TABLE test.bulk_update_items_no_pk CASCADE; +INSERT INTO test.bulk_update_items (id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); + TRUNCATE TABLE shops CASCADE; INSERT INTO shops(id, address, shop_geom) VALUES(1, '1369 Cambridge St', 'SRID=4326;POINT(-71.10044 42.373695)'); INSERT INTO shops(id, address, shop_geom) VALUES(2, '757 Massachusetts Ave', 'SRID=4326;POINT(-71.10543 42.366432)'); diff --git a/test/spec/fixtures/privileges.sql b/test/spec/fixtures/privileges.sql index 9a0bc95a81..09031e3d1a 100644 --- a/test/spec/fixtures/privileges.sql +++ b/test/spec/fixtures/privileges.sql @@ -191,6 +191,7 @@ GRANT ALL ON TABLE , view_test , bulk_update_items , bulk_update_items_cpk + , bulk_update_items_no_pk , shops , shop_bles , "SPECIAL ""@/\#~_-".names diff --git a/test/spec/fixtures/schema.sql b/test/spec/fixtures/schema.sql index b2e13ab19f..424a1f62a7 100644 --- a/test/spec/fixtures/schema.sql +++ b/test/spec/fixtures/schema.sql @@ -2653,6 +2653,12 @@ CREATE TABLE test.bulk_update_items_cpk ( PRIMARY KEY (id, name) ); +CREATE TABLE test.bulk_update_items_no_pk ( + id INT, + name TEXT, + observation TEXT +); + create extension if not exists postgis with schema extensions; create table shops ( From 30c4edf2c96ef728d5329d648fed8b20be6a642d Mon Sep 17 00:00:00 2001 From: Laurence Isla Date: Wed, 10 Aug 2022 20:40:39 -0500 Subject: [PATCH 3/8] Add/move tests for full table updates to PgSafeUpdate.hs --- test/spec/Feature/Query/PgSafeUpdateSpec.hs | 358 ++++++++++++++++++-- test/spec/Feature/Query/UpdateSpec.hs | 182 ---------- test/spec/fixtures/data.sql | 34 +- test/spec/fixtures/privileges.sql | 12 +- test/spec/fixtures/schema.sql | 48 ++- 5 files changed, 403 insertions(+), 231 deletions(-) diff --git a/test/spec/Feature/Query/PgSafeUpdateSpec.hs b/test/spec/Feature/Query/PgSafeUpdateSpec.hs index e53412a851..5544257300 100644 --- a/test/spec/Feature/Query/PgSafeUpdateSpec.hs +++ b/test/spec/Feature/Query/PgSafeUpdateSpec.hs @@ -13,21 +13,121 @@ import SpecHelper spec :: SpecWith ((), Application) spec = describe "Enabling pg-safeupdate" $ do - context "Full table update" $ - it "does not update and throws no error if no condition is present" $ - request methodPatch "/safe_update" - [("Prefer", "count=exact")] - [json| {"name": "New name"} |] + context "Full table update" $ do + it "does not update and throws error if no condition is present" $ + request methodPatch "/safe_update_items" + [] + [json| {"name": "updated-item"} |] + `shouldRespondWith` + [json|{ + "code": "21000", + "details": null, + "hint": null, + "message": "UPDATE requires a WHERE clause" + }|] + { matchStatus = 400 } + + it "does not do a bulk update and throws error if no filter is found in the array body" $ + request methodPatch "/safe_update_items" + [("Prefer", "tx=commit")] + [json|[ + { "name": "item-all", "observation": "Damaged item" } + , { "name": "item-3 - 3rd", "observation": null } + ]|] + `shouldRespondWith` + [json|{ + "code": "21000", + "details": null, + "hint": null, + "message": "UPDATE requires a WHERE clause" + }|] + { matchStatus = 400 } + + it "does not do a bulk update and throws error without filters and no pk in ?columns" $ + request methodPatch "/safe_update_items?columns=observation" + [("Prefer", "tx=commit")] + [json|[ + { "id": 1, "name": "item-4", "observation": "Damaged item" } + , { "id": 3, "name": "item-3 - 3rd", "observation": null } + ]|] + `shouldRespondWith` + [json|{ + "code": "21000", + "details": null, + "hint": null, + "message": "UPDATE requires a WHERE clause" + }|] + { matchStatus = 400 } + + it "does not do a bulk update and throws error without filters and no composite pk in ?columns" $ + request methodPatch "/safe_update_items_cpk?columns=id,observation" + [("Prefer", "tx=commit")] + [json|[ + { "id": 1, "name": "item-4", "observation": "Damaged item" } + , { "id": 3, "name": "item-3 - 3rd", "observation": null } + ]|] + `shouldRespondWith` + [json|{ + "code": "21000", + "details": null, + "hint": null, + "message": "UPDATE requires a WHERE clause" + }|] + { matchStatus = 400 } + + it "does not do a bulk update tables without pks and throws error" $ + request methodPatch "/safe_update_items_no_pk" + [("Prefer", "tx=commit")] + [json|[ + { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } + , { "id": 3, "name": "item-3 - 3rd", "observation": null } + ]|] + `shouldRespondWith` + [json|{ + "code": "21000", + "details": null, + "hint": null, + "message": "UPDATE requires a WHERE clause" + }|] + { matchStatus = 400 } + + it "allows full table update if a filter is present" $ do + get "/safe_update_items" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodPatch "/safe_update_items?id=gt.0" + [("Prefer", "tx=commit")] + [json|{"observation": "Damaged item"}|] `shouldRespondWith` "" - { matchStatus = 404 + { matchStatus = 204 , matchHeaders = [ matchHeaderAbsent hContentType - , "Content-Range" <:> "*/0" ] + , "Content-Range" <:> "0-2/*" + , "Preference-Applied" <:> "tx=commit" ] } + get "/safe_update_items?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": "Damaged item" } + , { "id": 2, "name": "item-2", "observation": "Damaged item" } + , { "id": 3, "name": "item-3", "observation": "Damaged item" } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "safe_update_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + context "Full table delete" $ do it "does not delete and throws error if no condition is present" $ - request methodDelete "/safe_delete" [] mempty + request methodDelete "/safe_delete_items" [] mempty `shouldRespondWith` [json|{ "code": "21000", @@ -37,40 +137,248 @@ spec = }|] { matchStatus = 400 } - it "allows full table delete if a filter is present" $ - request methodDelete "/safe_delete?id=gt.0" - [("Prefer", "count=exact")] - mempty + it "allows full table delete if a filter is present" $ do + get "/safe_delete_items" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodDelete "/safe_delete_items?id=gt.0" + [("Prefer", "tx=commit"), ("Prefer", "count=exact")] mempty `shouldRespondWith` "" { matchStatus = 204 , matchHeaders = [ matchHeaderAbsent hContentType - , "Content-Range" <:> "*/3" ] + , "Content-Range" <:> "*/3" + , "Preference-Applied" <:> "tx=commit" ] } + get "/safe_delete_items?order=id" + `shouldRespondWith` + [json|[]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "safe_delete_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + disabledSpec :: SpecWith ((), Application) disabledSpec = describe "Disabling pg-safeupdate" $ do - context "Full table update" $ - it "does not update and does not throw error if no condition is present" $ - request methodPatch "/unsafe_update" - [("Prefer", "count=exact")] - [json| {"name": "New name"} |] + context "Full table update" $ do + it "works if no condition is present" $ do + get "/unsafe_update_items" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodPatch "/unsafe_update_items" + [("Prefer", "tx=commit"), ("Prefer", "count=exact")] + [json| {"name": "updated-item"} |] `shouldRespondWith` "" - { matchStatus = 404 + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Content-Range" <:> "0-2/3" + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/unsafe_update_items?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "updated-item", "observation": null } + , { "id": 2, "name": "updated-item", "observation": null } + , { "id": 3, "name": "updated-item", "observation": null } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "unsafe_update_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "works wihtout filters, taking only the first item in the json array body" $ do + get "/unsafe_update_items" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodPatch "/unsafe_update_items" + [("Prefer", "tx=commit")] + [json|[ + { "name": "item-all", "observation": "Damaged item" } + , { "name": "item-3 - 3rd", "observation": null } + ]|] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Content-Range" <:> "0-2/*" + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/unsafe_update_items?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-all", "observation": "Damaged item" } + , { "id": 2, "name": "item-all", "observation": "Damaged item" } + , { "id": 3, "name": "item-all", "observation": "Damaged item" } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "unsafe_update_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "works without filters and no pk in ?columns, taking only the first item in the json array body" $ do + get "/unsafe_update_items" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodPatch "/unsafe_update_items?columns=observation" + [("Prefer", "tx=commit")] + [json|[ + { "id": 1, "name": "item-4", "observation": "Damaged item" } + , { "id": 3, "name": "item-3 - 3rd", "observation": null } + ]|] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Content-Range" <:> "0-2/*" + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/unsafe_update_items?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": "Damaged item" } + , { "id": 2, "name": "item-2", "observation": "Damaged item" } + , { "id": 3, "name": "item-3", "observation": "Damaged item" } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "unsafe_update_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "works without filters and no composite pk in ?columns, taking only the first item in the json array body" $ do + get "/unsafe_update_items_cpk" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodPatch "/unsafe_update_items_cpk?columns=id,observation" + [("Prefer", "tx=commit")] + [json|[ + { "id": 1, "name": "item-4", "observation": "Damaged item" } + , { "id": 3, "name": "item-3 - 3rd", "observation": null } + ]|] + `shouldRespondWith` + "" + { matchStatus = 204 , matchHeaders = [ matchHeaderAbsent hContentType - , "Content-Range" <:> "*/0" ] + , "Content-Range" <:> "0-2/*" + , "Preference-Applied" <:> "tx=commit" ] } - context "Full table delete" $ - it "deletes and does not throw error if no condition is present" $ do - request methodDelete "/unsafe_delete" - [("Prefer", "count=exact")] + get "/unsafe_update_items_cpk?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": "Damaged item" } + , { "id": 1, "name": "item-2", "observation": "Damaged item" } + , { "id": 1, "name": "item-3", "observation": "Damaged item" } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "unsafe_update_items_cpk"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "works with no pk defined in the table, taking only the first item in the json array body" $ do + get "/unsafe_update_items_no_pk" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodPatch "/unsafe_update_items_no_pk" + [("Prefer", "tx=commit")] + [json|[ + { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } + , { "id": 3, "name": "item-3 - 3rd", "observation": null } + ]|] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Content-Range" <:> "0-2/*" + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/unsafe_update_items_no_pk?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } + , { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } + , { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "unsafe_update_items_no_pk"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + context "Full table delete" $ do + it "works if no condition is present" $ do + get "/unsafe_delete_items" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodDelete "/unsafe_delete_items" + [("Prefer", "tx=commit"), ("Prefer", "count=exact")] mempty `shouldRespondWith` "" { matchStatus = 204 , matchHeaders = [ matchHeaderAbsent hContentType - , "Content-Range" <:> "*/3" ] + , "Content-Range" <:> "*/3" + , "Preference-Applied" <:> "tx=commit" ] } + + get "/unsafe_delete_items?order=id" + `shouldRespondWith` + [json|[]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "unsafe_delete_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } diff --git a/test/spec/Feature/Query/UpdateSpec.hs b/test/spec/Feature/Query/UpdateSpec.hs index 43e0e166e7..2c2892ee08 100644 --- a/test/spec/Feature/Query/UpdateSpec.hs +++ b/test/spec/Feature/Query/UpdateSpec.hs @@ -388,40 +388,6 @@ spec = do } context "limited update" $ do - it "updates the whole table when no limit query or filter is given" $ 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" - [("Prefer", "tx=commit"), ("Prefer", "count=exact")] - [json| {"name": "updated-item"} |] - `shouldRespondWith` - "" - { matchStatus = 204 - , matchHeaders = [ matchHeaderAbsent hContentType - , "Content-Range" <:> "0-2/3" - , "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": "updated-item" } - ]|] - - request methodPost "/rpc/reset_items_tables" - [("Prefer", "tx=commit")] - [json| {"tbl_name": "limited_update_items"} |] - `shouldRespondWith` "" - { matchStatus = 204 } - it "works with the limit query param" $ do get "/limited_update_items" `shouldRespondWith` @@ -871,154 +837,6 @@ spec = do `shouldRespondWith` "" { matchStatus = 204 } - it "updates the whole table wihtout filters, taking only the first item in the json array body" $ do - get "/bulk_update_items" - `shouldRespondWith` - [json|[ - { "id": 1, "name": "item-1", "observation": null } - , { "id": 2, "name": "item-2", "observation": null } - , { "id": 3, "name": "item-3", "observation": null } - ]|] - - request methodPatch "/bulk_update_items" - [("Prefer", "tx=commit")] - [json|[ - { "name": "item-all", "observation": "Damaged item" } - , { "name": "item-3 - 3rd", "observation": null } - ]|] - `shouldRespondWith` - "" - { matchStatus = 204 - , matchHeaders = [ matchHeaderAbsent hContentType - , "Content-Range" <:> "0-2/*" - , "Preference-Applied" <:> "tx=commit" ] - } - - get "/bulk_update_items?order=id" - `shouldRespondWith` - [json|[ - { "id": 1, "name": "item-all", "observation": "Damaged item" } - , { "id": 2, "name": "item-all", "observation": "Damaged item" } - , { "id": 3, "name": "item-all", "observation": "Damaged item" } - ]|] - - request methodPost "/rpc/reset_items_tables" - [("Prefer", "tx=commit")] - [json| {"tbl_name": "bulk_update_items"} |] - `shouldRespondWith` "" - { matchStatus = 204 } - - it "updates the whole table without filters and no pk in ?columns, taking only the first item in the json array body" $ do - get "/bulk_update_items" - `shouldRespondWith` - [json|[ - { "id": 1, "name": "item-1", "observation": null } - , { "id": 2, "name": "item-2", "observation": null } - , { "id": 3, "name": "item-3", "observation": null } - ]|] - - request methodPatch "/bulk_update_items?columns=observation" - [("Prefer", "tx=commit")] - [json|[ - { "id": 1, "name": "item-4", "observation": "Damaged item" } - , { "id": 3, "name": "item-3 - 3rd", "observation": null } - ]|] - `shouldRespondWith` - "" - { matchStatus = 204 - , matchHeaders = [ matchHeaderAbsent hContentType - , "Content-Range" <:> "0-2/*" - , "Preference-Applied" <:> "tx=commit" ] - } - - get "/bulk_update_items?order=id" - `shouldRespondWith` - [json|[ - { "id": 1, "name": "item-1", "observation": "Damaged item" } - , { "id": 2, "name": "item-2", "observation": "Damaged item" } - , { "id": 3, "name": "item-3", "observation": "Damaged item" } - ]|] - - request methodPost "/rpc/reset_items_tables" - [("Prefer", "tx=commit")] - [json| {"tbl_name": "bulk_update_items"} |] - `shouldRespondWith` "" - { matchStatus = 204 } - - it "updates the whole table without filters and no composite pk in ?columns, taking only the first item in the json array body" $ do - get "/bulk_update_items_cpk" - `shouldRespondWith` - [json|[ - { "id": 1, "name": "item-1", "observation": null } - , { "id": 2, "name": "item-2", "observation": null } - , { "id": 3, "name": "item-3", "observation": null } - ]|] - - request methodPatch "/bulk_update_items_cpk?columns=id,observation" - [("Prefer", "tx=commit")] - [json|[ - { "id": 1, "name": "item-4", "observation": "Damaged item" } - , { "id": 3, "name": "item-3 - 3rd", "observation": null } - ]|] - `shouldRespondWith` - "" - { matchStatus = 204 - , matchHeaders = [ matchHeaderAbsent hContentType - , "Content-Range" <:> "0-2/*" - , "Preference-Applied" <:> "tx=commit" ] - } - - get "/bulk_update_items_cpk?order=id" - `shouldRespondWith` - [json|[ - { "id": 1, "name": "item-1", "observation": "Damaged item" } - , { "id": 1, "name": "item-2", "observation": "Damaged item" } - , { "id": 1, "name": "item-3", "observation": "Damaged item" } - ]|] - - request methodPost "/rpc/reset_items_tables" - [("Prefer", "tx=commit")] - [json| {"tbl_name": "bulk_update_items_cpk"} |] - `shouldRespondWith` "" - { matchStatus = 204 } - - it "updates the whole table with no pk defined in it" $ do - get "/bulk_update_items_no_pk" - `shouldRespondWith` - [json|[ - { "id": 1, "name": "item-1", "observation": null } - , { "id": 2, "name": "item-2", "observation": null } - , { "id": 3, "name": "item-3", "observation": null } - ]|] - - request methodPatch "/bulk_update_items_no_pk" - [("Prefer", "tx=commit")] - [json|[ - { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } - , { "id": 3, "name": "item-3 - 3rd", "observation": null } - ]|] - `shouldRespondWith` - "" - { matchStatus = 204 - , matchHeaders = [ matchHeaderAbsent hContentType - , "Content-Range" <:> "0-1/*" - , "Preference-Applied" <:> "tx=commit" ] - } - - get "/bulk_update_items_no_pk?order=id" - `shouldRespondWith` - [json|[ - { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } - , { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } - , { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } - ]|] - - request methodPost "/rpc/reset_items_tables" - [("Prefer", "tx=commit")] - [json| {"tbl_name": "bulk_update_items_no_pk"} |] - `shouldRespondWith` "" - { matchStatus = 204 } - it "rejects a json array that isn't exclusively composed of objects" $ request methodPatch "/bulk_update_items" [("Prefer", "tx=commit")] diff --git a/test/spec/fixtures/data.sql b/test/spec/fixtures/data.sql index 7d16b83530..042822ca7c 100644 --- a/test/spec/fixtures/data.sql +++ b/test/spec/fixtures/data.sql @@ -787,9 +787,6 @@ INSERT INTO test.bulk_update_items (id, name, observation) VALUES (1, 'item-1', TRUNCATE TABLE test.bulk_update_items_cpk CASCADE; INSERT INTO test.bulk_update_items_cpk (id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); -TRUNCATE TABLE test.bulk_update_items_no_pk CASCADE; -INSERT INTO test.bulk_update_items (id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); - TRUNCATE TABLE shops CASCADE; INSERT INTO shops(id, address, shop_geom) VALUES(1, '1369 Cambridge St', 'SRID=4326;POINT(-71.10044 42.373695)'); INSERT INTO shops(id, address, shop_geom) VALUES(2, '757 Massachusetts Ave', 'SRID=4326;POINT(-71.10543 42.366432)'); @@ -807,11 +804,26 @@ INSERT INTO "SPECIAL ""@/\#~_-".names (id, name) VALUES (1, 'John'), (2, 'Mary') TRUNCATE TABLE do$llar$s CASCADE; INSERT INTO do$llar$s (a$num$) VALUES (100), (200), (300); -TRUNCATE TABLE safe_update CASCADE; -INSERT INTO safe_update(id, name) VALUES (1, 'First'), (2, 'Second'), (3, 'Third'); -TRUNCATE TABLE safe_delete CASCADE; -INSERT INTO safe_delete(id, name) VALUES (1, 'First'), (2, 'Second'), (3, 'Third'); -TRUNCATE TABLE unsafe_update CASCADE; -INSERT INTO unsafe_update(id, name) VALUES (1, 'First'), (2, 'Second'), (3, 'Third'); -TRUNCATE TABLE unsafe_delete CASCADE; -INSERT INTO unsafe_delete(id, name) VALUES (1, 'First'), (2, 'Second'), (3, 'Third'); +TRUNCATE TABLE safe_update_items CASCADE; +INSERT INTO safe_update_items(id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); + +TRUNCATE TABLE safe_update_items_cpk CASCADE; +INSERT INTO safe_update_items_cpk(id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); + +TRUNCATE TABLE safe_update_items_no_pk CASCADE; +INSERT INTO safe_update_items_no_pk(id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); + +TRUNCATE TABLE safe_delete_items CASCADE; +INSERT INTO safe_delete_items(id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); + +TRUNCATE TABLE unsafe_update_items CASCADE; +INSERT INTO unsafe_update_items(id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); + +TRUNCATE TABLE unsafe_update_items_cpk CASCADE; +INSERT INTO unsafe_update_items_cpk(id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); + +TRUNCATE TABLE unsafe_update_items_no_pk CASCADE; +INSERT INTO unsafe_update_items_no_pk(id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); + +TRUNCATE TABLE unsafe_delete_items CASCADE; +INSERT INTO unsafe_delete_items(id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); diff --git a/test/spec/fixtures/privileges.sql b/test/spec/fixtures/privileges.sql index 09031e3d1a..a196a16806 100644 --- a/test/spec/fixtures/privileges.sql +++ b/test/spec/fixtures/privileges.sql @@ -196,10 +196,14 @@ GRANT ALL ON TABLE , shop_bles , "SPECIAL ""@/\#~_-".names , do$llar$s - , safe_update - , safe_delete - , unsafe_update - , unsafe_delete + , safe_update_items + , safe_update_items_cpk + , safe_update_items_no_pk + , safe_delete_items + , unsafe_update_items + , unsafe_update_items_cpk + , unsafe_update_items_no_pk + , unsafe_delete_items 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 424a1f62a7..fdd06f622b 100644 --- a/test/spec/fixtures/schema.sql +++ b/test/spec/fixtures/schema.sql @@ -2534,7 +2534,7 @@ select * from limited_delete_items_cpk; create function reset_items_tables(tbl_name text default '') returns void as $_$ begin execute format( $$ - delete from %I; + delete from %I where true; insert into %I values (1, 'item-1'), (2, 'item-2'), (3, 'item-3'); $$::text, tbl_name, tbl_name); @@ -2698,24 +2698,54 @@ CREATE TABLE do$llar$s ( -- Tables and functions to test the pg-safeupdate library -CREATE TABLE test.safe_update( +CREATE TABLE test.safe_update_items( id INT PRIMARY KEY, - name TEXT + name TEXT, + observation TEXT +); + +CREATE TABLE test.safe_update_items_cpk( + id INT, + name TEXT, + observation TEXT, + PRIMARY KEY (id, name) ); -CREATE TABLE test.safe_delete( +CREATE TABLE test.safe_update_items_no_pk( + id INT, + name TEXT, + observation TEXT +); + +CREATE TABLE test.safe_delete_items( id INT PRIMARY KEY, - name TEXT + name TEXT, + observation TEXT ); -CREATE TABLE test.unsafe_update( +CREATE TABLE test.unsafe_update_items( id INT PRIMARY KEY, - name TEXT + name TEXT, + observation TEXT +); + +CREATE TABLE test.unsafe_update_items_cpk( + id INT, + name TEXT, + observation TEXT, + PRIMARY KEY (id, name) ); -CREATE TABLE test.unsafe_delete( +CREATE TABLE test.unsafe_update_items_no_pk( + id INT, + name TEXT, + observation TEXT +); + +CREATE TABLE test.unsafe_delete_items( id INT PRIMARY KEY, - name TEXT + name TEXT, + observation TEXT ); CREATE OR REPLACE FUNCTION test.load_safeupdate() RETURNS VOID AS $$ From c17d1b96482f389fa3fdb380408d2b0ab8664671 Mon Sep 17 00:00:00 2001 From: Laurence Isla Date: Wed, 10 Aug 2022 21:05:28 -0500 Subject: [PATCH 4/8] Remove full table PATCH restriction from changelog --- CHANGELOG.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87d94ab5cd..aff3503c82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,7 +61,6 @@ This project adheres to [Semantic Versioning](http://semver.org/). - #2277, #2238, #1643, Prevent views from breaking one-to-many/many-to-one embeds when using column or FK as target - @steve-chavez + When using a column or FK as target for embedding(`/tbl?select=*,col-or-fk(*)`), only tables are now detected and views are not. + You can still use a column or an inferred FK on a view to embed a table(`/view?select=*,col-or-fk(*)`) - - #1959, An accidental full table PATCH(without filters) is not possible anymore, it requires filters or a `limit` parameter - @steve-chavez, @laurenceisla - #2317, Increase the `db-pool-timeout` to 1 hour to prevent frequent high connection latency - @steve-chavez - #2341, The search path now correctly identifies schemas with uppercase and special characters in their names (regression) - @laurenceisla - #2364, "404 Not Found" on nested routes and "405 Method Not Allowed" errors no longer start an empty database transaction - @steve-chavez @@ -82,8 +81,6 @@ This project adheres to [Semantic Versioning](http://semver.org/). - #2277, Views now are not detected when embedding using the column or FK as target (`/view?select=*,column(*)`) - @steve-chavez + This embedding form was easily made ambiguous whenever a new view was added. + For migrating, clients must be updated to the embedding form of `/view?select=*,other_view!column(*)`. - - #1959, A full table PATCH(without filters) is now restricted, it requires a `limit` parameter - @steve-chavez - + A `PATCH /tbl` will now result in 0 rows updated, unless `PATCH /tbl?limit=10&order=` is done - #2312, Using `Prefer: return=representation` no longer returns a `Location` header - @laurenceisla ## [9.0.1] - 2022-06-03 From aaa5526ac584619fa1d9ca80ab6b0aff9b56685c Mon Sep 17 00:00:00 2001 From: Laurence Isla Date: Wed, 10 Aug 2022 22:33:33 -0500 Subject: [PATCH 5/8] Reorder conditionals --- src/PostgREST/Query/QueryBuilder.hs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/PostgREST/Query/QueryBuilder.hs b/src/PostgREST/Query/QueryBuilder.hs index d992830dcb..192bd5b70c 100644 --- a/src/PostgREST/Query/QueryBuilder.hs +++ b/src/PostgREST/Query/QueryBuilder.hs @@ -111,8 +111,9 @@ mutateRequestToQuery (Update mainQi uCols body logicForest pkFlts range ordts re SQL.sql $ "SELECT " <> emptyBodyReturnedColumns <> " FROM " <> fromQi mainQi <> " WHERE false" | range == allRange = - let whereLogic | null logicForest = if null pkFlts || not colsHavePk then mempty else " WHERE " <> pgrstUpdateBodyF - | otherwise = " WHERE " <> logicForestF in + let whereLogic | not (null logicForest) = " WHERE " <> logicForestF + | not (null pkFlts) && pkInCols = " WHERE " <> pgrstUpdateBodyF + | otherwise = mempty in "WITH " <> normalizedBody body <> " " <> "UPDATE " <> mainTbl <> " SET " <> SQL.sql nonRangeCols <> " " <> "FROM (SELECT * FROM json_populate_recordset (null::" <> mainTbl <> " , " <> SQL.sql selectBody <> " )) pgrst_update_body " <> @@ -137,7 +138,7 @@ mutateRequestToQuery (Update mainQi uCols body logicForest pkFlts range ordts re where mainTbl = SQL.sql (fromQi mainQi) - colsHavePk = all (`elem` uCols) pkFlts + pkInCols = all (`elem` uCols) pkFlts logicForestF = intercalateSnippet " AND " (pgFmtLogicTree mainQi <$> logicForest) pgrstUpdateBodyF = SQL.sql (BS.intercalate " AND " $ (\x -> pgFmtColumn mainQi x <> " = " <> pgFmtColumn (QualifiedIdentifier mempty "pgrst_update_body") x) <$> pkFlts) emptyBodyReturnedColumns = if null returnings then "NULL" else BS.intercalate ", " (pgFmtColumn (QualifiedIdentifier mempty $ qiName mainQi) <$> returnings) From 9f50115d999fe505f51980469079196c0663ad1c Mon Sep 17 00:00:00 2001 From: Laurence Isla Date: Thu, 11 Aug 2022 15:16:49 -0500 Subject: [PATCH 6/8] Clarify pk search in columns --- src/PostgREST/Query/QueryBuilder.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PostgREST/Query/QueryBuilder.hs b/src/PostgREST/Query/QueryBuilder.hs index 192bd5b70c..66ee3271e8 100644 --- a/src/PostgREST/Query/QueryBuilder.hs +++ b/src/PostgREST/Query/QueryBuilder.hs @@ -138,7 +138,7 @@ mutateRequestToQuery (Update mainQi uCols body logicForest pkFlts range ordts re where mainTbl = SQL.sql (fromQi mainQi) - pkInCols = all (`elem` uCols) pkFlts + pkInCols = S.fromList pkFlts `S.isSubsetOf` uCols logicForestF = intercalateSnippet " AND " (pgFmtLogicTree mainQi <$> logicForest) pgrstUpdateBodyF = SQL.sql (BS.intercalate " AND " $ (\x -> pgFmtColumn mainQi x <> " = " <> pgFmtColumn (QualifiedIdentifier mempty "pgrst_update_body") x) <$> pkFlts) emptyBodyReturnedColumns = if null returnings then "NULL" else BS.intercalate ", " (pgFmtColumn (QualifiedIdentifier mempty $ qiName mainQi) <$> returnings) From 3affe7c191fe20e956e74a552f3c62afa8945d8b Mon Sep 17 00:00:00 2001 From: Laurence Isla Date: Thu, 11 Aug 2022 17:19:33 -0500 Subject: [PATCH 7/8] Move some tests to UpdateSpec.hs --- test/spec/Feature/Query/PgSafeUpdateSpec.hs | 148 -------------------- test/spec/Feature/Query/UpdateSpec.hs | 148 ++++++++++++++++++++ test/spec/fixtures/data.sql | 9 +- test/spec/fixtures/privileges.sql | 2 - test/spec/fixtures/schema.sql | 13 -- 5 files changed, 151 insertions(+), 169 deletions(-) diff --git a/test/spec/Feature/Query/PgSafeUpdateSpec.hs b/test/spec/Feature/Query/PgSafeUpdateSpec.hs index 5544257300..6364b77f67 100644 --- a/test/spec/Feature/Query/PgSafeUpdateSpec.hs +++ b/test/spec/Feature/Query/PgSafeUpdateSpec.hs @@ -204,154 +204,6 @@ disabledSpec = `shouldRespondWith` "" { matchStatus = 204 } - it "works wihtout filters, taking only the first item in the json array body" $ do - get "/unsafe_update_items" - `shouldRespondWith` - [json|[ - { "id": 1, "name": "item-1", "observation": null } - , { "id": 2, "name": "item-2", "observation": null } - , { "id": 3, "name": "item-3", "observation": null } - ]|] - - request methodPatch "/unsafe_update_items" - [("Prefer", "tx=commit")] - [json|[ - { "name": "item-all", "observation": "Damaged item" } - , { "name": "item-3 - 3rd", "observation": null } - ]|] - `shouldRespondWith` - "" - { matchStatus = 204 - , matchHeaders = [ matchHeaderAbsent hContentType - , "Content-Range" <:> "0-2/*" - , "Preference-Applied" <:> "tx=commit" ] - } - - get "/unsafe_update_items?order=id" - `shouldRespondWith` - [json|[ - { "id": 1, "name": "item-all", "observation": "Damaged item" } - , { "id": 2, "name": "item-all", "observation": "Damaged item" } - , { "id": 3, "name": "item-all", "observation": "Damaged item" } - ]|] - - request methodPost "/rpc/reset_items_tables" - [("Prefer", "tx=commit")] - [json| {"tbl_name": "unsafe_update_items"} |] - `shouldRespondWith` "" - { matchStatus = 204 } - - it "works without filters and no pk in ?columns, taking only the first item in the json array body" $ do - get "/unsafe_update_items" - `shouldRespondWith` - [json|[ - { "id": 1, "name": "item-1", "observation": null } - , { "id": 2, "name": "item-2", "observation": null } - , { "id": 3, "name": "item-3", "observation": null } - ]|] - - request methodPatch "/unsafe_update_items?columns=observation" - [("Prefer", "tx=commit")] - [json|[ - { "id": 1, "name": "item-4", "observation": "Damaged item" } - , { "id": 3, "name": "item-3 - 3rd", "observation": null } - ]|] - `shouldRespondWith` - "" - { matchStatus = 204 - , matchHeaders = [ matchHeaderAbsent hContentType - , "Content-Range" <:> "0-2/*" - , "Preference-Applied" <:> "tx=commit" ] - } - - get "/unsafe_update_items?order=id" - `shouldRespondWith` - [json|[ - { "id": 1, "name": "item-1", "observation": "Damaged item" } - , { "id": 2, "name": "item-2", "observation": "Damaged item" } - , { "id": 3, "name": "item-3", "observation": "Damaged item" } - ]|] - - request methodPost "/rpc/reset_items_tables" - [("Prefer", "tx=commit")] - [json| {"tbl_name": "unsafe_update_items"} |] - `shouldRespondWith` "" - { matchStatus = 204 } - - it "works without filters and no composite pk in ?columns, taking only the first item in the json array body" $ do - get "/unsafe_update_items_cpk" - `shouldRespondWith` - [json|[ - { "id": 1, "name": "item-1", "observation": null } - , { "id": 2, "name": "item-2", "observation": null } - , { "id": 3, "name": "item-3", "observation": null } - ]|] - - request methodPatch "/unsafe_update_items_cpk?columns=id,observation" - [("Prefer", "tx=commit")] - [json|[ - { "id": 1, "name": "item-4", "observation": "Damaged item" } - , { "id": 3, "name": "item-3 - 3rd", "observation": null } - ]|] - `shouldRespondWith` - "" - { matchStatus = 204 - , matchHeaders = [ matchHeaderAbsent hContentType - , "Content-Range" <:> "0-2/*" - , "Preference-Applied" <:> "tx=commit" ] - } - - get "/unsafe_update_items_cpk?order=id" - `shouldRespondWith` - [json|[ - { "id": 1, "name": "item-1", "observation": "Damaged item" } - , { "id": 1, "name": "item-2", "observation": "Damaged item" } - , { "id": 1, "name": "item-3", "observation": "Damaged item" } - ]|] - - request methodPost "/rpc/reset_items_tables" - [("Prefer", "tx=commit")] - [json| {"tbl_name": "unsafe_update_items_cpk"} |] - `shouldRespondWith` "" - { matchStatus = 204 } - - it "works with no pk defined in the table, taking only the first item in the json array body" $ do - get "/unsafe_update_items_no_pk" - `shouldRespondWith` - [json|[ - { "id": 1, "name": "item-1", "observation": null } - , { "id": 2, "name": "item-2", "observation": null } - , { "id": 3, "name": "item-3", "observation": null } - ]|] - - request methodPatch "/unsafe_update_items_no_pk" - [("Prefer", "tx=commit")] - [json|[ - { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } - , { "id": 3, "name": "item-3 - 3rd", "observation": null } - ]|] - `shouldRespondWith` - "" - { matchStatus = 204 - , matchHeaders = [ matchHeaderAbsent hContentType - , "Content-Range" <:> "0-2/*" - , "Preference-Applied" <:> "tx=commit" ] - } - - get "/unsafe_update_items_no_pk?order=id" - `shouldRespondWith` - [json|[ - { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } - , { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } - , { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } - ]|] - - request methodPost "/rpc/reset_items_tables" - [("Prefer", "tx=commit")] - [json| {"tbl_name": "unsafe_update_items_no_pk"} |] - `shouldRespondWith` "" - { matchStatus = 204 } - context "Full table delete" $ do it "works if no condition is present" $ do get "/unsafe_delete_items" diff --git a/test/spec/Feature/Query/UpdateSpec.hs b/test/spec/Feature/Query/UpdateSpec.hs index 2c2892ee08..e60222e401 100644 --- a/test/spec/Feature/Query/UpdateSpec.hs +++ b/test/spec/Feature/Query/UpdateSpec.hs @@ -837,6 +837,154 @@ spec = do `shouldRespondWith` "" { matchStatus = 204 } + it "updates the whole table without filters, taking only the first item in the json array body" $ do + get "/bulk_update_items" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodPatch "/bulk_update_items" + [("Prefer", "tx=commit")] + [json|[ + { "name": "item-all", "observation": "Damaged item" } + , { "name": "item-3 - 3rd", "observation": null } + ]|] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Content-Range" <:> "0-2/*" + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/bulk_update_items?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-all", "observation": "Damaged item" } + , { "id": 2, "name": "item-all", "observation": "Damaged item" } + , { "id": 3, "name": "item-all", "observation": "Damaged item" } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "bulk_update_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "updates the whole table without filters and no pk in ?columns, taking only the first item in the json array body" $ do + get "/bulk_update_items" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodPatch "/bulk_update_items?columns=observation" + [("Prefer", "tx=commit")] + [json|[ + { "id": 1, "name": "item-4", "observation": "Damaged item" } + , { "id": 3, "name": "item-3 - 3rd", "observation": null } + ]|] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Content-Range" <:> "0-2/*" + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/bulk_update_items?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": "Damaged item" } + , { "id": 2, "name": "item-2", "observation": "Damaged item" } + , { "id": 3, "name": "item-3", "observation": "Damaged item" } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "bulk_update_items"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "updates the whole table without filters and no composite pk in ?columns, taking only the first item in the json array body" $ do + get "/bulk_update_items_cpk" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodPatch "/bulk_update_items_cpk?columns=id,observation" + [("Prefer", "tx=commit")] + [json|[ + { "id": 1, "name": "item-4", "observation": "Damaged item" } + , { "id": 3, "name": "item-3 - 3rd", "observation": null } + ]|] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Content-Range" <:> "0-2/*" + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/bulk_update_items_cpk?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": "Damaged item" } + , { "id": 1, "name": "item-2", "observation": "Damaged item" } + , { "id": 1, "name": "item-3", "observation": "Damaged item" } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "bulk_update_items_cpk"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + + it "updates the whole table with no pk defined in it" $ do + get "/bulk_update_items_no_pk" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1", "observation": null } + , { "id": 2, "name": "item-2", "observation": null } + , { "id": 3, "name": "item-3", "observation": null } + ]|] + + request methodPatch "/bulk_update_items_no_pk" + [("Prefer", "tx=commit")] + [json|[ + { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } + , { "id": 3, "name": "item-3 - 3rd", "observation": null } + ]|] + `shouldRespondWith` + "" + { matchStatus = 204 + , matchHeaders = [ matchHeaderAbsent hContentType + , "Content-Range" <:> "0-2/*" + , "Preference-Applied" <:> "tx=commit" ] + } + + get "/bulk_update_items_no_pk?order=id" + `shouldRespondWith` + [json|[ + { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } + , { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } + , { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" } + ]|] + + request methodPost "/rpc/reset_items_tables" + [("Prefer", "tx=commit")] + [json| {"tbl_name": "bulk_update_items_no_pk"} |] + `shouldRespondWith` "" + { matchStatus = 204 } + it "rejects a json array that isn't exclusively composed of objects" $ request methodPatch "/bulk_update_items" [("Prefer", "tx=commit")] diff --git a/test/spec/fixtures/data.sql b/test/spec/fixtures/data.sql index 042822ca7c..e5039223a0 100644 --- a/test/spec/fixtures/data.sql +++ b/test/spec/fixtures/data.sql @@ -787,6 +787,9 @@ INSERT INTO test.bulk_update_items (id, name, observation) VALUES (1, 'item-1', TRUNCATE TABLE test.bulk_update_items_cpk CASCADE; INSERT INTO test.bulk_update_items_cpk (id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); +TRUNCATE TABLE test.bulk_update_items_no_pk CASCADE; +INSERT INTO test.bulk_update_items_no_pk (id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); + TRUNCATE TABLE shops CASCADE; INSERT INTO shops(id, address, shop_geom) VALUES(1, '1369 Cambridge St', 'SRID=4326;POINT(-71.10044 42.373695)'); INSERT INTO shops(id, address, shop_geom) VALUES(2, '757 Massachusetts Ave', 'SRID=4326;POINT(-71.10543 42.366432)'); @@ -819,11 +822,5 @@ INSERT INTO safe_delete_items(id, name, observation) VALUES (1, 'item-1', NULL), TRUNCATE TABLE unsafe_update_items CASCADE; INSERT INTO unsafe_update_items(id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); -TRUNCATE TABLE unsafe_update_items_cpk CASCADE; -INSERT INTO unsafe_update_items_cpk(id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); - -TRUNCATE TABLE unsafe_update_items_no_pk CASCADE; -INSERT INTO unsafe_update_items_no_pk(id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); - TRUNCATE TABLE unsafe_delete_items CASCADE; INSERT INTO unsafe_delete_items(id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL); diff --git a/test/spec/fixtures/privileges.sql b/test/spec/fixtures/privileges.sql index a196a16806..b79e0de71c 100644 --- a/test/spec/fixtures/privileges.sql +++ b/test/spec/fixtures/privileges.sql @@ -201,8 +201,6 @@ GRANT ALL ON TABLE , safe_update_items_no_pk , safe_delete_items , unsafe_update_items - , unsafe_update_items_cpk - , unsafe_update_items_no_pk , unsafe_delete_items TO postgrest_test_anonymous; diff --git a/test/spec/fixtures/schema.sql b/test/spec/fixtures/schema.sql index fdd06f622b..ffdc6cbc84 100644 --- a/test/spec/fixtures/schema.sql +++ b/test/spec/fixtures/schema.sql @@ -2729,19 +2729,6 @@ CREATE TABLE test.unsafe_update_items( observation TEXT ); -CREATE TABLE test.unsafe_update_items_cpk( - id INT, - name TEXT, - observation TEXT, - PRIMARY KEY (id, name) -); - -CREATE TABLE test.unsafe_update_items_no_pk( - id INT, - name TEXT, - observation TEXT -); - CREATE TABLE test.unsafe_delete_items( id INT PRIMARY KEY, name TEXT, From 5f173711e98d5c2aebab81f82d9527e21eb977c1 Mon Sep 17 00:00:00 2001 From: Laurence Isla Date: Thu, 11 Aug 2022 18:56:17 -0500 Subject: [PATCH 8/8] Fix test --- test/spec/Feature/Query/QueryLimitedSpec.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/spec/Feature/Query/QueryLimitedSpec.hs b/test/spec/Feature/Query/QueryLimitedSpec.hs index addca7907c..eb192876c7 100644 --- a/test/spec/Feature/Query/QueryLimitedSpec.hs +++ b/test/spec/Feature/Query/QueryLimitedSpec.hs @@ -93,7 +93,7 @@ spec = { "id": 8, "name": "HaikuOS" } ]|] { matchStatus = 201 } - it "doesn't affect updates" $ + it "doesn't affect updates(2 rows would be modified if it did)" $ request methodPatch "/employees?select=first_name,last_name,occupation" [("Prefer", "return=representation")] [json| [{"occupation": "Barista"}] |]