fix: HTTP status responses for upserts

[taimoorzaeem]

This might fix #1070.

Fixed

• PUT returns 201 instead of 200 when rows are inserted
• POST with Prefer: resolution=merge-duplicates returns 200 instead of 201 when no rows are inserted

[laurenceisla]

Also, I'm wondering if the pgrst.inserted guc could/should be initialized to '0' here:

postgrest/src/PostgREST/Query.hs

Lines 234 to 236 in 0703f27

	setPgLocals :: AppConfig -> KM.KeyMap JSON.Value -> BS.ByteString -> [(ByteString, ByteString)] ->
	ApiRequest -> PgVersion -> DbHandler ()
	setPgLocals AppConfig{..} claims role roleSettings req actualPgVersion = lift $

It would avoid using coalesce(...,0) in the query.

Edit: NVM, @steve-chavez confirmed that it may be kinda costly, it's better to keep it as it is right now.

[taimoorzaeem]

With this change, the cost of single upsert becomes:

test/spec/Feature/Query/PlanSpec.hs:195:19: 
  5) Feature.Query.PlanSpec.spec, writes plans, outputs the total cost for a single upsert
       expected: 1.29
        but got: 3.55

@laurenceisla @steve-chavez Is it still worth it?

[steve-chavez]

outputs the total cost for a single upsert

@taimoorzaeem To be extra sure, we would need to test upserts with more than one single row. These are done with resolution=merge-duplicates. If the extra cost is constant, then it would be worth it, I think.

So how about adding some more tests below here:

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

Lines 183 to 195 in a9d6c31

	it "outputs the total cost for a single upsert" $ do
	r <- request methodPut "/tiobe_pls?name=eq.Go"
	(acceptHdrs "application/vnd.pgrst.plan+json")
	[json| [ { "name": "Go", "rank": 19 } ]|]
	let totalCost = planCost r
	resHeaders = simpleHeaders r
	resStatus = simpleStatus r
	liftIO $ do
	resHeaders `shouldSatisfy` elem ("Content-Type", "application/vnd.pgrst.plan+json; for=\"application/json\"; charset=utf-8")
	resStatus `shouldBe` Status { statusCode = 200, statusMessage="OK" }
	totalCost `shouldBe` 1.29

I'm thinking:

 it "outputs the total cost for an upsert with 10 rows" $ do

 it "outputs the total cost for an upsert with 100 rows" $ do

 it "outputs the total cost for an upsert with 1000 rows" $ do

There should be a helper function that generates the payload. Rough idea:

$ postgrest-repl

ghci> [("Lang " <> show i, i) | i <- [20..120] ] :: [(Text, Int)]

[("Lang 20",20),("Lang 21",21)....

And maybe using Aeson.toJSONList to convert to JSON.

---

These tests can be added in another PR. In fact that would be better to see the resulting costs there to compare.

Sometimes we do commits that only add tests, like add10bd

[taimoorzaeem]

These tests can be added in another PR. In fact that would be better to see the resulting costs there to compare.

Alright. I'll open another PR to add these tests.

[laurenceisla]

As far as I can see, there were no existing tests for Prefer: merge-duplicates where all the elements in the payload already exists, thus generating only updates and no inserts.

Adding this test to the beginning of UpsertSpec.hs makes the test fail:

it "UPDATEs rows and returns 200 on pk conflict on all the items" $
  request methodPost "/tiobe_pls" [("Prefer", "return=representation"), ("Prefer", "resolution=merge-duplicates")]
    [json| [
      { "name": "Python", "rank": 6 },
      { "name": "Java", "rank": 2 },
      { "name": "C", "rank": 1 }
    ]|] `shouldRespondWith` [json| [
      { "name": "Python", "rank": 6 },
      { "name": "Java", "rank": 2 },
      { "name": "C", "rank": 1 }
    ]|]
    { matchStatus = 200
    , matchHeaders = ["Preference-Applied" <:> "resolution=merge-duplicates, return=representation", matchContentTypeJson]
    }

All of those already exist in the database, so a 200 should be returned since no insert was done. The good thing is that the changes you made in SQL already allow this, only needs implementation for Merge Duplicates as you did with PUT.

So far so good, we're almost there! This wasn't a simple issue by any means.

[laurenceisla]

Lastly, what's missing from the original issue is to verify that:

• It returns 200 when the payload is []
• It returns 200 when Prefer: ignore-duplicates is sent and all the elements in the body are duplicates.

[taimoorzaeem]

@laurenceisla

It returns 200 when Prefer: ignore-duplicates is sent and all the elements in the body are duplicates.

Hmm, for ignore-duplicates, it's getting tricky.

mutatePlanToQuery :: Maybe PreferResolution -> MutatePlan -> SQL.Snippet
mutatePlanToQuery resolution (Insert mainQi iCols body onConflct putConditions returnings _ applyDefaults) =
  "INSERT INTO " <> fromQi mainQi <> (if null iCols then " " else "(" <> cols <> ") ") <>
  fromJsonBodyF body iCols True False applyDefaults <>
  -- Only used for PUT
  (if null putConditions then mempty else "WHERE " <> addConfigPgrstInserted True <> " AND " <> intercalateSnippet " AND " (pgFmtLogicTree (QualifiedIdentifier mempty "pgrst_body") <$> putConditions)) <>
  (if null putConditions && isJust resolution then "WHERE " <> addConfigPgrstInserted True else mempty) <>
  maybe mempty (\(oncDo, oncCols) ->
    if null oncCols then
      mempty
    else
      " ON CONFLICT(" <> intercalateSnippet ", " (pgFmtIdent <$> oncCols) <> ") " <> case oncDo of
      IgnoreDuplicates ->
        "DO NOTHING"
      MergeDuplicates  ->
        if null iCols
           then "DO NOTHING"
           else "DO UPDATE SET " <> intercalateSnippet ", " ((pgFmtIdent . cfName) <> const " = EXCLUDED." <> (pgFmtIdent . cfName) <$> iCols) <> (if null putConditions && isNothing resolution then mempty else "WHERE " <> addConfigPgrstInserted False)
    ) onConflct <> " " <>
    returningF mainQi returnings

I first add 1 when row is inserted, but if there is resolution=ignore-duplicates, I need to subtract that.
In other words, I need to do something like this:

 IgnoreDuplicates ->
        "DO NOTHING" <> " WHERE " <> addConfigPgrstInserted False -- not possible i guess

[laurenceisla]

Hmm, for ignore-duplicates, it's getting tricky.

True that. I noticed that PostgreSQL allows ON CONFLICT WHERE index_predicate ..., but it didn't work for me to set the pgrst.inserted value there. It may work in a different way.

We could leave that outside of this PR to check for a better solution later on, it won't close the issue completely but it's good progress.

[laurenceisla]

Good job! I clarified a message and rebased against the changes made in main to be ready to merge.