Revert bulk update patch

- Revert patch #2311
- Keep the refactor done to qsFiltersRoot
- Keep the refactor done to the items tables
- Add tests that now work with pg-safeupdate as a result

test/spec/Feature/Query/PgSafeUpdateSpec.hs

@@ -13,21 +13,58 @@ 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"
+    context "Full table update" $ do
+      it "does not update and throws error if no condition is present" $
+        request methodPatch "/safe_update_items"
             [("Prefer", "count=exact")]
             [json| {"name": "New name"} |]
+          `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"), ("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 "/safe_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": "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 +74,101 @@ spec =
             }|]
             { matchStatus  = 400 }
 
-      it "allows full table delete if a filter is present" $
-        request methodDelete "/safe_delete?id=gt.0"
-            [("Prefer", "count=exact")]
+      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" ]
+                             , "Content-Range" <:> "0-2/3"
+                             , "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?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 }
+
+    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 }

test/spec/Feature/Query/QueryLimitedSpec.hs

@@ -98,8 +98,11 @@ spec =
             [("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"

test/spec/Feature/Query/SingularSpec.hs

@@ -65,7 +65,7 @@ spec =
             }
 
       it "raises an error for multiple rows" $ do
-        request methodPatch "/addresses?limit=4&order=id"
+        request methodPatch "/addresses"
             [("Prefer", "tx=commit"), singular]
             [json| { address: "zzz" } |]
           `shouldRespondWith`
@@ -81,7 +81,7 @@ spec =
             [json|[{"id":1,"address":"address 1"}]|]
 
       it "raises an error for multiple rows with return=rep" $ do
-        request methodPatch "/addresses?limit=4&order=id"
+        request methodPatch "/addresses"
             [("Prefer", "tx=commit"), ("Prefer", "return=representation"), singular]
             [json| { address: "zzz" } |]
           `shouldRespondWith`

test/spec/Feature/Query/UpdateSpec.hs

@@ -388,17 +388,6 @@ spec = do
           }
 
   context "limited update" $ do
-    it "does not work when no limit query or filter is given" $
-      request methodPatch "/limited_update_items"
-          [("Prefer", "tx=commit"), ("Prefer", "count=exact")]
-          [json| {"name": "updated-item"} |]
-        `shouldRespondWith`
-          ""
-          { matchStatus  = 404
-          , matchHeaders = [ matchHeaderAbsent hContentType
-                           , "Content-Range" <:> "*/0" ]
-          }
-
     it "works with the limit query param" $ do
       get "/limited_update_items"
         `shouldRespondWith`
@@ -624,181 +613,3 @@ spec = do
         [json| {"tbl_name": "limited_update_items_no_pk"} |]
         `shouldRespondWith` ""
         { matchStatus  = 204 }
-
-  context "bulk updates" $ do
-    it "can update tables with simple pk" $ 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|[
-            { "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?order=id"
-        `shouldRespondWith`
-          [json|[
-            { "id": 1, "name": "item-1 - 1st", "observation": "Lost item" }
-          , { "id": 2, "name": "item-2", "observation": null }
-          , { "id": 3, "name": "item-3 - 3rd", "observation": null }
-          ]|]
-
-      request methodPost "/rpc/reset_items_tables"
-        [("Prefer", "tx=commit")]
-        [json| {"tbl_name": "bulk_update_items"} |]
-        `shouldRespondWith` ""
-        { matchStatus  = 204 }
-
-    it "can update tables with composite pk" $ 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"
-          [("Prefer", "tx=commit")]
-          [json|[
-            { "id": 1, "name": "item-1", "observation": "Lost item" }
-          , { "id": 2, "name": "item-2", "observation": null }
-          ]|]
-        `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": null  }
-          ]|]
-
-      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`
-          [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?id=eq.2"
-          [("Prefer", "tx=commit")]
-          [json|[
-            { "id": 4, "name": "item-4", "observation": "Damaged item" }
-          , { "id": 3, "name": "item-3 - 3rd", "observation": null }
-          ]|]
-        `shouldRespondWith`
-          ""
-          { matchStatus  = 204
-          , matchHeaders = [ matchHeaderAbsent hContentType
-                           , "Content-Range" <:> "0-0/*"
-                           , "Preference-Applied" <:> "tx=commit" ]
-          }
-
-      get "/bulk_update_items?order=id"
-        `shouldRespondWith`
-          [json|[
-            { "id": 1, "name": "item-1", "observation": null }
-          , { "id": 3, "name": "item-3", "observation": null }
-          , { "id": 4, "name": "item-4", "observation": "Damaged item" }
-          ]|]
-
-      request methodPost "/rpc/reset_items_tables"
-        [("Prefer", "tx=commit")]
-        [json| {"tbl_name": "bulk_update_items"} |]
-        `shouldRespondWith` ""
-        { matchStatus  = 204 }
-
-    it "updates with limit and offset 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?limit=2&offset=1&order=id"
-          [("Prefer", "tx=commit")]
-          [json|[
-            { "name": "item-4", "observation": "Damaged item" }
-          , { "name": "item-3 - 3rd", "observation": null }
-          ]|]
-        `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": null }
-          , { "id": 2, "name": "item-4", "observation": "Damaged item" }
-          , { "id": 3, "name": "item-4", "observation": "Damaged item" }
-          ]|]
-
-      request methodPost "/rpc/reset_items_tables"
-        [("Prefer", "tx=commit")]
-        [json| {"tbl_name": "bulk_update_items"} |]
-        `shouldRespondWith` ""
-        { matchStatus  = 204 }
-
-    it "rejects a json array that isn't exclusively composed of objects" $
-      request methodPatch "/bulk_update_items"
-          [("Prefer", "tx=commit")]
-          [json|[
-            { "id": 1, "name": "Item 1" }
-          , 2
-          , "Item 2"
-          , { "id": 3, "name": "Item 3" }
-          ]|]
-        `shouldRespondWith`
-          [json| {"message":"All object keys must match","code":"PGRST102","hint":null,"details":null} |]
-          { matchStatus  = 400
-          , matchHeaders = [matchContentTypeJson]
-          }
-
-    it "rejects a json array that has objects with different keys" $
-      request methodPatch "/bulk_update_items"
-          [("Prefer", "tx=commit")]
-          [json|[
-            { "id": 1, "name": "Item 1" }
-          , { "id": 2 }
-          , { "id": 3, "name": "Item 3" }
-          ]|]
-        `shouldRespondWith`
-          [json| {"message":"All object keys must match","code":"PGRST102","hint":null,"details":null} |]
-          { matchStatus  = 400
-          , matchHeaders = [matchContentTypeJson]
-          }

test/spec/fixtures/data.sql

@@ -781,12 +781,6 @@ INSERT INTO private.internal_job (id, parent_id) VALUES (2, 1);
 TRUNCATE TABLE test.test CASCADE;
 INSERT INTO test.test (id, parent_id) VALUES (1, null), (2, 1);
 
-TRUNCATE TABLE test.bulk_update_items 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 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 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)');
@@ -804,11 +798,11 @@ 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_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_delete_items CASCADE;
+INSERT INTO unsafe_delete_items(id, name, observation) VALUES (1, 'item-1', NULL), (2, 'item-2', NULL), (3, 'item-3', NULL);

test/spec/fixtures/privileges.sql

@@ -189,16 +189,14 @@ GRANT ALL ON TABLE
     , series_popularity
     , test
     , view_test
-    , bulk_update_items
-    , bulk_update_items_cpk
     , shops
     , shop_bles
     , "SPECIAL ""@/\#~_-".names
     , do$llar$s
-    , safe_update
-    , safe_delete
-    , unsafe_update
-    , unsafe_delete
+    , safe_update_items
+    , safe_delete_items
+    , unsafe_update_items
+    , unsafe_delete_items
 TO postgrest_test_anonymous;
 
 GRANT INSERT ON TABLE insertonly TO postgrest_test_anonymous;

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; -- WHERE is required for pg-safeupdate tests
     insert into %I values (1, 'item-1'), (2, 'item-2'), (3, 'item-3');
   $$::text,
   tbl_name, tbl_name);
@@ -2638,21 +2638,6 @@ CREATE TABLE test.test (
 CREATE OR REPLACE VIEW test.view_test AS
   SELECT id FROM test.test;
 
--- Tables to test bulk updates
-
-CREATE TABLE test.bulk_update_items (
-  id INT PRIMARY KEY,
-  name TEXT,
-  observation TEXT
-);
-
-CREATE TABLE test.bulk_update_items_cpk (
-  id INT,
-  name TEXT,
-  observation TEXT,
-  PRIMARY KEY (id, name)
-);
-
 create extension if not exists postgis with schema extensions;
 
 create table shops (
@@ -2692,24 +2677,28 @@ 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_delete(
+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_delete(
+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 $$