Add support for UPSERT by steve-chavez · Pull Request #1048 · PostgREST/postgrest

steve-chavez · 2018-01-21T02:42:20Z

Closes #256. UPSERT is done with the ON CONFLICT clause available on PostgreSQL >= 9.5.

Bulk UPSERT can be done by adding Prefer: resolution=ignore-duplicates/merge-duplicates headers, these do a INSERT .. ON CONFLICT(<pkCols>) DO NOTHING/DO UPDATE.
The ignore-duplicates/merge-duplicates Prefer values only work with tables that have a PK, if the table doesn't have a PK the Prefer will be ignored(no error).

Single row UPSERT can be done with PUT if the client specifies only PK columns as eq filters and the payload is identified by the URI, other restrictions include not allowing Content-Range/limit/offset(took into account the previous implementation).

begriffs · 2018-01-21T03:11:02Z

🥂 Wow, this is a great feature. People have been wanting this one for a long long time, thank you!

I'll review the code and add notes.

begriffs · 2018-01-21T04:39:16Z

src/PostgREST/App.hs


-        (ActionDelete, TargetIdent _, Nothing) ->
-          case mutateSqlParts of
+        (ActionSingleUpsert, TargetIdent (QualifiedIdentifier tSchema tName), Just (PayloadJSON payload pType pKeys)) ->


I was confused below because pKeys made me think "primary keys" rather than "payload keys." Maybe name it pjKeys?

begriffs · 2018-01-21T04:41:02Z

src/PostgREST/App.hs

+                       createWriteStatement sq mq (contentType == CTSingularJSON) False
+                                            (contentType == CTTextCSV) (iPreferRepresentation apiRequest) []
+                let (_, queryTotal, _, body) = extractQueryResult row
+                if queryTotal /= 1


Nice, queryTotal /= 1 is a clever way to detect that the PUT is being applied incorrectly, rather than comparing the filters in the request to primary keys etc etc.

Looking further into the code, I see that mutateRequest checks for ActionSingleUpsert that the request contains equality filters on all primary keys. Does that make the queryTotal /= 1 here unnecessary, or what is the interaction between the logic?

Is not clear from the code but I'm enforcing a different condition here, so first of all my interpretation of RFC 7231 is that the PUT uri must identify the payload being inserted so that the next GET can return the same exact payload right? So what I'm enforcing here is this:

-- table numbers is empty it "fails if the uri doesn't identify the payload" $ put "/numbers?id=eq.7" [str| [ { "id": 11, "name": "eleven"} ]|] `shouldRespondWith` 405

If that PUT would succeed the subsequent GET on /numbers?id=eq.7 would give an empty array instead of the payload. This is actually done by the sql query that WHEREs the payload according to the querystring parameters, if that WHERE gives false then nothing gets upserted and the queryTotal will be 0.

I tried to not have the restriction of just allowing primary key cols eq filters first, but without that there's no reliable way of identifying the payload with the same sql query, consider:

-- id is persons pk, there's already many records with the name John PUT /persons?name=eq.John { "id": 12, "first_name": "John", "last_name": "Doe" }

The WHERE would give true and the PUT would succeed with a queryTotal == 1 but since name is not the pk the subsequent GET /persons?name=eq.John would have many records and not just the same payload.

@begriffs Let me know if I'm interpreting the RFC right and if so how can this logic be made more simple/clear.

begriffs · 2018-01-21T04:43:50Z

src/PostgREST/App.hs

+                if queryTotal /= 1
+                  then do
+                    HT.condemn
+                    return $ simpleError status405 [] "The URI must univocally identify the payload when using PUT"


This error message might be unclear. What about, "PUT must be applied to exactly one resource." Maybe include a hint like, "Request attempted to modify ${queryTotal} table rows"

Check my comments in #1048 (comment), if I'm misunderstanding something in the RFC I'll change it but for now not sure what would be the clearer message.

I'm haven't figured out how to make a request that causes this error message to happen. What do I need to do?

This test shows how to make the error happen: https://github.com/begriffs/postgrest/pull/1048/files#diff-385e57e2f6697b6568f08e4d151b1d7cR133

Thanks, now I see. What about adding a hint to the error message, like

{ "message": "The URI must univocally identify the payload when using PUT", "hint": "Changing primary key values with PUT is not allowed" }

The existing message is fine, but the hint could make it more concrete for people.

Or "hint": "Payload values do not match resource in primary key column(s)"
whatever you think explains it best.

begriffs · 2018-01-21T06:42:19Z

test/Feature/UpsertSpec.hs

+          `shouldRespondWith`
+          [json|[ { "id": 1 }, { "id": 2 }, { "id": 4} ]|] { matchStatus = 201 , matchHeaders = [matchContentTypeJson] }
+
+      it "succeeds and ignores the Prefer: resolution header if the table has no PK" $


What about this alternative: if a table has no pk then make the Prefer header resolution=ignore-duplicates translate to ON CONFLICT DO NOTHING (which has an empty conflict target). But have resolution=merge-duplicates raise an error on tables without a pk because there's no obvious conflict target other than the primary keys. Currently it appears that the entire ON CONFLICT clause is dropped for these tables.

I feel like if a client explicitly requests merge-duplicates then it's an error to ignore it on tables without a pk. Is there a use-case you have in mind that I am overlooking?

I chose to drop the ON CONFLICT clause for table with no pks because I thought that Prefer semantics meant the condition it's optional and shouldn't give an error, also according to RFC 7240:

A server that does not recognize or is unable to comply with
particular preference tokens in the Prefer header field of a request
MUST ignore those tokens and continue processing instead of signaling
an error.

I thought to later add the Preference-Applied feature so the client can check the success of the condition.

Great point that this is only a Prefer header so we shouldn't raise an error if the header can't be applied.

About tables with no primary keys, we could conceivably do ON CONFLICT with no columns in parentheses. The docs say:

If no list of column names is given at all, the default is all the columns of the table in their declared order

Only problem is this might cause an expensive sequential scan for duplicates rather than using indices, so it would put extra load on the server. Your choice to drop the ON CONFLICT for tables without keys is probably the right thing to do.

Would it be difficult to include a Preference-Applied response header for resolution? Might help people debug a surprising result.

begriffs · 2018-01-21T06:48:41Z

This is really pleasant and readable code. I just had a few small comments, and a question about the proper behavior on tables lacking a primary key.

jekirl · 2018-01-27T19:44:36Z

Wow this is great! First time I have actually looked at the PostgREST code base, but just looking at the PR this is really well done. Thanks

begriffs · 2018-02-09T19:46:56Z

I tried this out locally again and it's fun to use. 🎈

Noticed a few things:

"You must specify all columns in the payload when using PUT"
- This returns HTTP 405, which is misleading because PUT is supported on the resource, it's just that the payload was wrong. Seems more like it should be a 400 error. (405 is perfect for the "Filters must include all and only primary key columns with 'eq' operator" though)

This was weird, but when I did a POST with a single item that duplicated a row in the table, and used Prefer: resolution=ignore-duplicates, then the server had an internal error.

{
  "details": "Row number 0",
  "message": "Row error: unexpected null"
}

Seems like a haskell error because running the generated query in psql just gives

┌──────────────────┬────────────┬───────────┬──────────┐
│ total_result_set │ page_total │ array_agg │ ?column? │
├──────────────────┼────────────┼───────────┼──────────┤
│                  │          0 │ ∅         │          │
└──────────────────┴────────────┴───────────┴──────────┘

Including multiple items in the POST array makes the error go away, even when both items are dups of existing rows.

steve-chavez · 2018-02-16T19:08:43Z

@begriffs Added latest corrections.

begriffs · 2018-02-20T04:10:46Z

test/Feature/UpsertSpec.hs

+      it "succeeds if not a single resource is created" $ do
+        request methodPost "/tiobe_pls" [("Prefer", "return=representation"), ("Prefer", "resolution=ignore-duplicates")]
+          [json|[ { "name": "Java", "rank": 1 } ]|] `shouldRespondWith`
+          [json|[]|] { matchStatus = 201 , matchHeaders = [matchContentTypeJson] }


If no records got created, then a 201 response doesn't seem accurate. Maybe a 200 in this case?

If it's a pain to change then it maybe it's not such a big deal. What do you think?

Actually tried that before by applying a queryTotal == 0 condition but other unrelated tests failed.

Like this one, the issue is the return=minimal always makes the queryTotal == 0 despite rows being inserted.

There's another way of detecting the insertion by using the xmax system column, tried to use that for PUT as well (since it should respond 200 for update and 201 for insert according to RFC) but it involves a not-so-easy refactoring.

@begriffs How about if I merge as it is and open an issue for that to correct it later?

Also some old insert tests like this one https://github.com/begriffs/postgrest/blob/master/test/Feature/InsertSpec.hs#L129-L135 respond with 201 despite no row being inserted.

OK, since it's an existing issue, we can merge this PR and fix that one in its own.

(Cool xmax trick btw!)

begriffs · 2018-02-20T04:11:59Z

I added one more note about response codes. Aside from that and rebasing it looks good to merge!

- Ensure creating nothing on ignore-duplicates succeeds - Refactor locationF query

begriffs reviewed Jan 21, 2018

View reviewed changes

steve-chavez force-pushed the upsert branch from c8e0b04 to 4bf87f0 Compare January 27, 2018 22:47

steve-chavez force-pushed the upsert branch 2 times, most recently from 943ec9e to 283a994 Compare February 19, 2018 13:51

begriffs reviewed Feb 20, 2018

View reviewed changes

steve-chavez added 3 commits February 20, 2018 13:20

Add UPSERT for POST with Prefer:resoultion=merge/ignore-duplicates

338745c

Allow PUT method: UPSERT of a single row

5c79789

Add Preference-Applied header for POST upsert

fa4bd7f

- Ensure creating nothing on ignore-duplicates succeeds - Refactor locationF query

steve-chavez force-pushed the upsert branch from 283a994 to fa4bd7f Compare February 20, 2018 18:22

steve-chavez merged commit 108f3cd into PostgREST:master Feb 21, 2018

steve-chavez mentioned this pull request Apr 2, 2018

Bulk insert with only one record fails when no primary keys specified in select= #1087

Closed

steve-chavez mentioned this pull request May 16, 2018

Fix UPSERT bug on camelCase PK column #1114

Merged

steve-chavez mentioned this pull request Apr 11, 2019

Fix invalid SQL generated for bulk upsert with empty array #1270

Merged

Uh oh!

Conversation

steve-chavez commented Jan 21, 2018

Uh oh!

begriffs commented Jan 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steve-chavez Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steve-chavez Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begriffs commented Jan 21, 2018

Uh oh!

jekirl commented Jan 27, 2018

Uh oh!

begriffs commented Feb 9, 2018

Uh oh!

steve-chavez commented Feb 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steve-chavez Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steve-chavez Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begriffs commented Feb 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

steve-chavez Jan 25, 2018 •

edited

Loading

steve-chavez Jan 25, 2018 •

edited

Loading

steve-chavez Feb 20, 2018 •

edited

Loading

steve-chavez Feb 20, 2018 •

edited

Loading