Support bulk updates using PATCH #2693

laurenceisla · 2023-03-01T03:21:54Z

Closes #1959. It allows bulk updates using PATCH only when the header Prefer: params=multiple-objects is applied as suggested here #1959 (comment).

Allow bulk updates
Allow different keys in the body
Needs: feat: undefined json keys as defaults when using ?columns and a Prefer header #2672
Clean and refactor
Add tests
Add changelog

test/spec/Feature/Query/UpdateSpec.hs

laurenceisla · 2023-03-02T01:45:07Z

test/spec/Feature/Query/UpdateSpec.hs

+    it "does not update when no pk is specified in the body" $ do
+      request methodPatch "/bulk_update_items"
+          [("Prefer", "tx=commit"), ("Prefer", "params=multiple-objects")]
+          [json|[
+            { "name": "Item 1" }
+          , { "name": "Item 2" }
+          ]|]
+        `shouldRespondWith` 400


Here, the error is returned by the database and has this form:

{ "code": "42703", "details": null, "hint": null, "message": "column pgrst_body.id does not exist" }

It looks a bit confusing, maybe an SchemaCache error could be created to specify that this is an issue with bulk updates and no PK is found in the body. @steve-chavez WDYT?

@laurenceisla That would be much better 👍

Why does it assume an id column on the error? Is it hardcoded?

Is it hardcoded?

No, in this case, the PK for the table is id. The next test example has a composite PK of id,name and only id is specified in the body. In that case, the message would be:

{ "code": "42703", "details": null, "hint": null, "message": "column pgrst_body.name does not exist" }

Ah, got it. Q: What does happen when specifying ?columns=id and the body doesn't have an id?

It does not give an error unlike normal PATCH. That's because the WHERE with the primary key is forced in the bulk update:

WHERE "test"."bulk_update_items"."id" = "pgrst_body"."id"

In this case, the pgrs_body.id is null, so the query works but it won't update any row.

I think that's fine.

In this case, the pgrs_body.id is null, so the query works but it won't update any row.

I guess one way to prevent this would be to count the array elements and compare it to the updated items. If they're equal then we can assume a unique key was passed for every row, if not, we could throw an error.

Not sure if worth it though, it could be confusing as well because we can't tell the user which array element would have the incorrect unique.

Not sure if worth it though, it could be confusing as well because we can't tell the user which array element would have the incorrect unique.

Yeah, I'm not sure about this one either, maybe it's better to discuss later for another PR.

It looks a bit confusing, maybe an SchemaCache error could be created to specify that this is an issue with bulk updates and no PK is found in the body.

I mentioned this but, as you said on another chat, using it with ?columns won't allow to verify if the JSON body has the id. So I'll go with the names you suggested maybe json_body or bulk_update_body instead of pgrst_body.

src/PostgREST/Query/QueryBuilder.hs

src/PostgREST/Query/SqlFragment.hs

steve-chavez · 2023-03-03T01:21:32Z

test/spec/Feature/Query/UpdateSpec.hs

+    it "updates with limit and offset taking only the first item in the json array body" $ do
+      baseTable "bulk_update_items" "id" tblDataBeforeBulk
+      `mutatesWith`
+      requestMutation methodPatch "/bulk_update_items?limit=2&offset=1&order=id"
+        [("Prefer", "params=multiple-objects")]
+        [json|[
+          { "name": "item", "observation": "Lost item" }
+        , { "name": "item-2 - 2nd", "observation": "Damaged item" }
+        , { "name": "item-3 - 3rd", "observation": null }
+        ]|]
+      `shouldMutateInto`
+      [json|[
+        { "id": 1, "name": "item-1", "observation": null }
+      , { "id": 2, "name": "item", "observation": "Lost item" }
+      , { "id": 3, "name": "item", "observation": "Lost item" }
+      ]|]


A bit confused by this one. So any value on limit and offset will have the same outcome?

In other words, it works as a normal update (without bulk): it takes the first element in the body and updates applying limits/offsets. Now that you mention it, it could be confusing if the user expects a bulk update of some kind, maybe a custom error should be expected here, like the one where limit/offset are not allowed for PUT?

Just to be clear, limit/offset are ignored right? So limit=100&offset=400 would result on the same?

Now that you mention it, it could be confusing if the user expects a bulk update of some kind, maybe a custom error should be expected here, like the one where limit/offset are not allowed for PUT?

Yeah, I agree. I think we can ban limit/offset for bulk updates.

Just to be clear, limit/offset are ignored right? So limit=100&offset=400 would result on the same?

No, it does not ignore limits and the result would be different depending on the values. In other words. It works as if it was this test

postgrest/test/spec/Feature/Query/UpdateSpec.hs

Lines 511 to 521 in b05ea14

it "works with views with an explicit order by unique col" $

baseTable "limited_update_items_view" "id" tblDataBefore

`mutatesWith`

requestMutation methodPatch "/limited_update_items_view?order=id&limit=1&offset=1"

[json| {"name": "updated-item"} |]

`shouldMutateInto`

[json|[

{ "id": 1, "name": "item-1" }

, { "id": 2, "name": "updated-item" }

, { "id": 3, "name": "item-3" }

]|]

I implemented the Error handling for this case and I noticed that the PATCH and DELETE with limits and without order in the query string throw this error:

postgrest/src/PostgREST/ApiRequest.hs

Line 253 in b05ea14

| method `elem` ["PATCH", "DELETE"] && not (null qsRanges) && null qsOrder = Left LimitNoOrderError

The thing is that using the Range header with order in the query string, works as if we were using limit and offset. Also, using Range without order throws a different error that reaches the database, with this message:

"message": "syntax error at or near \"RETURNING\""

@steve-chavez Is this behavior expected? This is unrelated to this PR, btw and is reproducible in the main branch.

Also, using Range without order throws a different error that reaches the database, with this message:
"message": "syntax error at or near "RETURNING""

Yeah that seems like a bug. Fixing it in another PR would be great.

steve-chavez · 2023-03-03T01:25:39Z

test/spec/Feature/Query/UpdateSpec.hs

+    it "rejects a json array that has objects with different keys" $
+      request methodPatch "/bulk_update_items"
+          [("Prefer", "tx=commit"), ("Prefer", "params=multiple-objects")]
+          [json|[
+            { "id": 1, "name": "Item 1" }
+          , { "id": 2, "name": "Item 2" }
+          , { "id": 3, "name": "Item 3", "observation": "New item" }
+          ]|]
+        `shouldRespondWith`
+          [json| {"message":"All object keys must match","code":"PGRST102","hint":null,"details":null} |]
+          { matchStatus  = 400
+          , matchHeaders = [matchContentTypeJson]
+          }


Hm, this is definitely undesirable. Users will want to update different columns for each array element. I think we'll need #2672 for solving this.

Maybe it can be solved without #2672. So assume you specify columns here or another test. You could do:

UPDATE bulk_update_items SET observation = COALESCE(pgrst_body.observation, observation) WHERE ..

Meaning that a value will be updated to the new value in the body or to its old value.

This will allow updating rows with a changing number of columns. WDYT?

After implementing this I noticed that setting a value to null will fail, e.g.:

PATCH /bulk_update_items?columns=id,name,observation

[ { "id": 1, "name": "Item 1" } , { "id": 3, "name": "Item 3", "observation": null } ]

The item with id=3 will keep its old observation and won't be set to null. Is there a conditional that could be used here, to only apply COALESCE in these cases? Although, I see it difficult due to the fact that columns does not check the body, right?

Right. Wolfgang suggested a way to do this with the ? operator on jsonb here.

On #2672 I had the same problem as well, but I decided to go for the alternative of using the || operator on jsonb.

Maybe those approaches could help.

I ended up using ? instead. Although it works now, I know that it isn't too performant, but when i used || , I got this query:

UPDATE "test"."bulk_update_items" SET "id" = "pgrst_body"."id", "name" = "pgrst_body"."name", "observation" = "pgrst_body"."observation" FROM (SELECT $1::jsonb AS json_data) pgrst_payload CROSS JOIN LATERAL (SELECT CASE WHEN jsonb_typeof(pgrst_payload.json_data) = 'array' THEN pgrst_payload.json_data ELSE jsonb_build_array(pgrst_payload.json_data) END AS val) pgrst_uniform_json CROSS JOIN LATERAL (SELECT jsonb_agg(jsonb_build_object('id', "test"."bulk_update_items"."id",'name', "test"."bulk_update_items"."name",'observation', "test"."bulk_update_items"."observation") || elem) AS val FROM jsonb_array_elements(pgrst_uniform_json.val) elem) pgrst_json_old_vals CROSS JOIN LATERAL (SELECT * FROM jsonb_to_recordset(pgrst_json_old_vals.val) AS _("id" integer, "name" text, "observation" text)) pgrst_body WHERE "test"."bulk_update_items"."id" = "pgrst_body"."id" RETURNING 1;

Which gives an error when accessing test.bulk_update_items inside the pgrst_json_old_vals subquery due to the CROSS JOIN. Couldn't find a way to make it work that way, maybe I'm missing something.

laurenceisla · 2023-03-25T08:10:22Z

src/PostgREST/ApiRequest/Preferences.hs

-  = ApplyDefaults  -- ^ Use the default column value for the unspecified keys.
-  | IgnoreDefaults -- ^ Inserts: null values / Updates: the keys are not SET to any value
+  = ApplyDefaults  -- ^ Inserts and Updates: Use the default column value for the unspecified keys.
+  | IgnoreValues   -- ^ Inserts: null values | Updates: the keys are not SET to any value
+  | ApplyNulls     -- ^ Inserts: null values | Updates: SET the values to null


I noticed that the comment I added in PR #2672 is wrong.

| IgnoreDefaults -- ^ Inserts: null values / Updates: the keys are not SET to any value

When no header is sent, it updates applying null values instead of "ignoring" the key and keeping the old value.

I added other header as a proposal:

IgnoreDefaults is now ApplyNulls and is the default behavior when no header is sent, that is, it updates the unspecified columns to null values.

IgnoreValues does not set to any value (in reality it sets to the original value but the result is the same).

steve-chavez · 2023-10-21T22:45:27Z

Will close the PR because it got too stale. Also the design of the feature changed on #1959 (comment)

laurenceisla commented Mar 1, 2023

View reviewed changes

test/spec/Feature/Query/UpdateSpec.hs Outdated Show resolved Hide resolved

laurenceisla force-pushed the bulk-patch branch from 22f3f35 to 6c2410f Compare March 1, 2023 23:16

laurenceisla marked this pull request as ready for review March 1, 2023 23:26

laurenceisla commented Mar 2, 2023

View reviewed changes

steve-chavez reviewed Mar 3, 2023

View reviewed changes

src/PostgREST/Query/QueryBuilder.hs Outdated Show resolved Hide resolved

steve-chavez reviewed Mar 3, 2023

View reviewed changes

src/PostgREST/Query/SqlFragment.hs Outdated Show resolved Hide resolved

steve-chavez reviewed Mar 3, 2023

View reviewed changes

laurenceisla force-pushed the bulk-patch branch from 4b2e2be to 9718091 Compare March 14, 2023 04:05

laurenceisla marked this pull request as draft March 15, 2023 01:07

laurenceisla mentioned this pull request Mar 15, 2023

Fix bug with limited PATCH or DELETE with Range header #2705

Closed

Add support for bulk updates

ff618d9

laurenceisla force-pushed the bulk-patch branch from 17f84a2 to ff618d9 Compare March 23, 2023 23:52

Specify what to do with the undefined keys in the json body

079ff83

laurenceisla commented Mar 25, 2023

View reviewed changes

laurenceisla marked this pull request as ready for review March 25, 2023 08:28

laurenceisla marked this pull request as draft March 25, 2023 08:28

laurenceisla mentioned this pull request Mar 28, 2023

Change to Prefer: missing=default #2723

Merged

skavem mentioned this pull request Sep 25, 2023

Support bulk update supabase/postgrest-js#174

Open

steve-chavez closed this Oct 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support bulk updates using PATCH #2693

Support bulk updates using PATCH #2693

laurenceisla commented Mar 1, 2023 •

edited

Loading

laurenceisla Mar 2, 2023

steve-chavez Mar 2, 2023

laurenceisla Mar 2, 2023

steve-chavez Mar 3, 2023

laurenceisla Mar 8, 2023

steve-chavez Mar 8, 2023

laurenceisla Mar 15, 2023

steve-chavez Mar 3, 2023

laurenceisla Mar 8, 2023

steve-chavez Mar 8, 2023

laurenceisla Mar 8, 2023

laurenceisla Mar 15, 2023 •

edited

Loading

steve-chavez Mar 15, 2023

steve-chavez Mar 3, 2023

steve-chavez Mar 3, 2023

laurenceisla Mar 8, 2023 •

edited

Loading

steve-chavez Mar 8, 2023

laurenceisla Mar 25, 2023 •

edited

Loading

laurenceisla Mar 25, 2023 •

edited

Loading

steve-chavez commented Oct 21, 2023

	it "works with views with an explicit order by unique col" $
	baseTable "limited_update_items_view" "id" tblDataBefore
	`mutatesWith`
	requestMutation methodPatch "/limited_update_items_view?order=id&limit=1&offset=1"
	[json\| {"name": "updated-item"} \|]
	`shouldMutateInto`
	[json\|[
	{ "id": 1, "name": "item-1" }
	, { "id": 2, "name": "updated-item" }
	, { "id": 3, "name": "item-3" }
	]\|]

Support bulk updates using PATCH #2693

Support bulk updates using PATCH #2693

Conversation

laurenceisla commented Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurenceisla Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurenceisla Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurenceisla Mar 25, 2023 • edited Loading

Choose a reason for hiding this comment

laurenceisla Mar 25, 2023 • edited Loading

Choose a reason for hiding this comment

steve-chavez commented Oct 21, 2023

laurenceisla commented Mar 1, 2023 •

edited

Loading

laurenceisla Mar 15, 2023 •

edited

Loading

laurenceisla Mar 8, 2023 •

edited

Loading

laurenceisla Mar 25, 2023 •

edited

Loading

laurenceisla Mar 25, 2023 •

edited

Loading