Skip to content

Commit

Permalink
fix: HTTP status responses for upserts
Browse files Browse the repository at this point in the history
  • Loading branch information
taimoorzaeem committed Oct 26, 2023
1 parent 6920a88 commit 7a868f6
Showing 12 changed files with 97 additions and 42 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -18,6 +18,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
### Fixed

- #3015, Fix unnecessary count() on RPC returning single - @steve-chavez
- #1070, Fix HTTP status responses for upserts - @taimoorzaeem
+ `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

## [11.2.2] - 2023-10-25

1 change: 1 addition & 0 deletions src/PostgREST/ApiRequest/Preferences.hs
Original file line number Diff line number Diff line change
@@ -176,6 +176,7 @@ class ToHeaderValue a where
data PreferResolution
= MergeDuplicates
| IgnoreDuplicates
deriving Eq

instance ToHeaderValue PreferResolution where
toHeaderValue MergeDuplicates = "resolution=merge-duplicates"
4 changes: 3 additions & 1 deletion src/PostgREST/Query.hs
Original file line number Diff line number Diff line change
@@ -188,16 +188,18 @@ openApiQuery sCache pgVer AppConfig{..} tSchema =
writeQuery :: MutateReadPlan -> ApiRequest -> AppConfig -> DbHandler ResultSet
writeQuery MutateReadPlan{mrReadPlan, mrMutatePlan, mrResAgg, mrMedia} ApiRequest{iPreferences=Preferences{..}} conf =
let
(isInsert, pkCols) = case mrMutatePlan of {Insert{insPkCols} -> (True, insPkCols); _ -> (False, mempty);}
(isPut, isInsert, pkCols) = case mrMutatePlan of {Insert{where_,insPkCols} -> ((not . null) where_, True, insPkCols); _ -> (False,False, mempty);}
in
lift . SQL.statement mempty $
Statements.prepareWrite
(QueryBuilder.readPlanToQuery mrReadPlan)
(QueryBuilder.mutatePlanToQuery mrMutatePlan)
isInsert
isPut
mrMedia
mrResAgg
preferRepresentation
preferResolution
pkCols
(configDbPreparedStatements conf)

8 changes: 5 additions & 3 deletions src/PostgREST/Query/QueryBuilder.hs
Original file line number Diff line number Diff line change
@@ -87,7 +87,8 @@ mutatePlanToQuery (Insert mainQi iCols body onConflct putConditions returnings _
"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 " <> intercalateSnippet " AND " (pgFmtLogicTree (QualifiedIdentifier mempty "pgrst_body") <$> putConditions)) <>
(if null putConditions then mempty else "WHERE " <> addConfigPgrstInserted True <> " AND " <> intercalateSnippet " AND " (pgFmtLogicTree (QualifiedIdentifier mempty "pgrst_body") <$> putConditions)) <>
(if null putConditions && mergeDups then "WHERE " <> addConfigPgrstInserted True else mempty) <>
maybe mempty (\(oncDo, oncCols) ->
if null oncCols then
mempty
@@ -98,11 +99,12 @@ mutatePlanToQuery (Insert mainQi iCols body onConflct putConditions returnings _
MergeDuplicates ->
if null iCols
then "DO NOTHING"
else "DO UPDATE SET " <> intercalateSnippet ", " ((pgFmtIdent . cfName) <> const " = EXCLUDED." <> (pgFmtIdent . cfName) <$> iCols)
else "DO UPDATE SET " <> intercalateSnippet ", " ((pgFmtIdent . cfName) <> const " = EXCLUDED." <> (pgFmtIdent . cfName) <$> iCols) <> (if null putConditions && not mergeDups then mempty else "WHERE " <> addConfigPgrstInserted False)
) onConflct <> " " <>
returningF mainQi returnings
returningF mainQi returnings
where
cols = intercalateSnippet ", " $ pgFmtIdent . cfName <$> iCols
mergeDups = case onConflct of {Just (MergeDuplicates,_) -> True; _ -> False;}

-- An update without a limit is always filtered with a WHERE
mutatePlanToQuery (Update mainQi uCols body logicForest range ordts returnings applyDefaults)
7 changes: 7 additions & 0 deletions src/PostgREST/Query/SqlFragment.hs
Original file line number Diff line number Diff line change
@@ -24,6 +24,8 @@ module PostgREST.Query.SqlFragment
, fromJsonBodyF
, responseHeadersF
, responseStatusF
, addConfigPgrstInserted
, currentSettingF
, returningF
, singleParameter
, sourceCTE
@@ -439,6 +441,11 @@ responseHeadersF = currentSettingF "response.headers"
responseStatusF :: SQL.Snippet
responseStatusF = currentSettingF "response.status"

addConfigPgrstInserted :: Bool -> SQL.Snippet
addConfigPgrstInserted add =
let (symbol, num) = if add then ("+", "0") else ("-", "-1") in
"set_config('pgrst.inserted', (coalesce(" <> currentSettingF "pgrst.inserted" <> "::int, 0) " <> symbol <> " 1)::text, true) <> '" <> num <> "'"

currentSettingF :: SQL.Snippet -> SQL.Snippet
currentSettingF setting =
-- nullif is used because of https://gist.github.com/steve-chavez/8d7033ea5655096903f3b52f8ed09a15
24 changes: 16 additions & 8 deletions src/PostgREST/Query/Statements.hs
Original file line number Diff line number Diff line change
@@ -49,15 +49,19 @@ data ResultSet
-- ^ the HTTP headers to be added to the response
, rsGucStatus :: Maybe Text
-- ^ the HTTP status to be added to the response
, rsInserted :: Maybe Int64

Check warning on line 52 in src/PostgREST/Query/Statements.hs

Codecov / codecov/patch

src/PostgREST/Query/Statements.hs#L52

Added line #L52 was not covered by tests
-- ^ the number of rows inserted (Only used with PUT)
}
| RSPlan BS.ByteString -- ^ the plan of the query


prepareWrite :: SQL.Snippet -> SQL.Snippet -> Bool -> MediaType -> ResultAggregate ->
Maybe PreferRepresentation -> [Text] -> Bool -> SQL.Statement () ResultSet
prepareWrite selectQuery mutateQuery isInsert mt rAgg rep pKeys =
prepareWrite :: SQL.Snippet -> SQL.Snippet -> Bool -> Bool -> MediaType -> ResultAggregate ->
Maybe PreferRepresentation -> Maybe PreferResolution -> [Text] -> Bool -> SQL.Statement () ResultSet
prepareWrite selectQuery mutateQuery isInsert isPut mt rAgg rep resolution pKeys =
SQL.dynamicallyParameterized (mtSnippet mt snippet) decodeIt
where
checkUpsert snip = if isInsert && (isPut || resolution == Just MergeDuplicates) then snip else "''"
pgrstInsertedF = checkUpsert "nullif(current_setting('pgrst.inserted', true),'')::int"
snippet =
"WITH " <> sourceCTE <> " AS (" <> mutateQuery <> ") " <>
"SELECT " <>
@@ -66,7 +70,8 @@ prepareWrite selectQuery mutateQuery isInsert mt rAgg rep pKeys =
locF <> " AS header, " <>
aggF Nothing rAgg <> " AS body, " <>
responseHeadersF <> " AS response_headers, " <>
responseStatusF <> " AS response_status " <>
responseStatusF <> " AS response_status, " <>
pgrstInsertedF <> " AS response_inserted " <>
"FROM (" <> selectF <> ") _postgrest_t"

locF =
@@ -86,7 +91,7 @@ prepareWrite selectQuery mutateQuery isInsert mt rAgg rep pKeys =
decodeIt :: HD.Result ResultSet
decodeIt = case mt of
MTPlan{} -> planRow
_ -> fromMaybe (RSStandard Nothing 0 mempty mempty Nothing Nothing) <$> HD.rowMaybe (standardRow False)
_ -> fromMaybe (RSStandard Nothing 0 mempty mempty Nothing Nothing Nothing) <$> HD.rowMaybe (standardRow False)

prepareRead :: SQL.Snippet -> SQL.Snippet -> Bool -> MediaType -> ResultAggregate -> Bool -> SQL.Statement () ResultSet
prepareRead selectQuery countQuery countTotal mt rAgg =
@@ -100,7 +105,8 @@ prepareRead selectQuery countQuery countTotal mt rAgg =
"pg_catalog.count(_postgrest_t) AS page_total, " <>
aggF Nothing rAgg <> " AS body, " <>
responseHeadersF <> " AS response_headers, " <>
responseStatusF <> " AS response_status " <>
responseStatusF <> " AS response_status, " <>
"''" <> " AS response_inserted " <>
"FROM ( SELECT * FROM " <> sourceCTE <> " ) _postgrest_t"

(countCTEF, countResultF) = countF countQuery countTotal
@@ -126,15 +132,16 @@ prepareCall rout callProcQuery selectQuery countQuery countTotal mt rAgg =
else "pg_catalog.count(_postgrest_t)") <> " AS page_total, " <>
aggF (Just rout) rAgg <> " AS body, " <>
responseHeadersF <> " AS response_headers, " <>
responseStatusF <> " AS response_status " <>
responseStatusF <> " AS response_status, " <>
"''" <> " AS response_inserted " <>
"FROM (" <> selectQuery <> ") _postgrest_t"

(countCTEF, countResultF) = countF countQuery countTotal

decodeIt :: HD.Result ResultSet
decodeIt = case mt of
MTPlan{} -> planRow
_ -> fromMaybe (RSStandard (Just 0) 0 mempty mempty Nothing Nothing) <$> HD.rowMaybe (standardRow True)
_ -> fromMaybe (RSStandard (Just 0) 0 mempty mempty Nothing Nothing Nothing) <$> HD.rowMaybe (standardRow True)

preparePlanRows :: SQL.Snippet -> Bool -> SQL.Statement () (Maybe Int64)
preparePlanRows countQuery =
@@ -152,6 +159,7 @@ standardRow noLocation =
<*> (if noLocation then pure mempty else fmap splitKeyValue <$> arrayColumn HD.bytea) <*> column HD.bytea
<*> nullableColumn HD.bytea
<*> nullableColumn HD.text
<*> nullableColumn HD.int8
where
splitKeyValue :: ByteString -> (ByteString, ByteString)
splitKeyValue kv =
27 changes: 18 additions & 9 deletions src/PostgREST/Response.hs
Original file line number Diff line number Diff line change
@@ -22,6 +22,7 @@ import qualified Data.Aeson as JSON
import qualified Data.ByteString.Char8 as BS
import qualified Data.ByteString.Lazy as LBS
import qualified Data.HashMap.Strict as HM
import Data.Maybe (fromJust)
import Data.Text.Read (decimal)
import qualified Network.HTTP.Types.Header as HTTP
import qualified Network.HTTP.Types.Status as HTTP
@@ -35,6 +36,7 @@ import qualified PostgREST.Response.OpenAPI as OpenAPI
import PostgREST.ApiRequest (ApiRequest (..),
InvokeMethod (..))
import PostgREST.ApiRequest.Preferences (PreferRepresentation (..),
PreferResolution (..),
Preferences (..),
prefAppliedHeader,
shouldCount)
@@ -119,8 +121,13 @@ createResponse QualifiedIdentifier{..} MutateReadPlan{mrMutatePlan, mrMedia} ctx
if shouldCount preferCount then Just rsQueryTotal else Nothing
, prefHeader ]

let status = HTTP.status201
let (headers', bod) = case preferRepresentation of
let isInsertIfGTZero i =
if i <= 0 && preferResolution == Just MergeDuplicates then
HTTP.status200
else
HTTP.status201
status = maybe HTTP.status200 isInsertIfGTZero rsInserted
(headers', bod) = case preferRepresentation of
Just Full -> (headers ++ contentTypeHeaders mrMedia ctxApiRequest, LBS.fromStrict rsBody)
Just None -> (headers, mempty)
Just HeadersOnly -> (headers, mempty)
@@ -142,11 +149,11 @@ updateResponse MutateReadPlan{mrMedia} ctxApiRequest@ApiRequest{iPreferences=Pre
prefHeader = prefAppliedHeader $ Preferences Nothing preferRepresentation Nothing preferCount preferTransaction preferMissing preferHandling []
headers = catMaybes [contentRangeHeader, prefHeader]

let
(status, headers', body) = case preferRepresentation of
Just Full -> (HTTP.status200, headers ++ contentTypeHeaders mrMedia ctxApiRequest, LBS.fromStrict rsBody)
Just None -> (HTTP.status204, headers, mempty)
_ -> (HTTP.status204, headers, mempty)
let (status, headers', body) =
case preferRepresentation of
Just Full -> (HTTP.status200, headers ++ contentTypeHeaders mrMedia ctxApiRequest, LBS.fromStrict rsBody)
Just None -> (HTTP.status204, headers, mempty)
_ -> (HTTP.status204, headers, mempty)

(ovStatus, ovHeaders) <- overrideStatusHeaders rsGucStatus rsGucHeaders status headers'

@@ -162,9 +169,11 @@ singleUpsertResponse MutateReadPlan{mrMedia} ctxApiRequest@ApiRequest{iPreferenc
prefHeader = maybeToList . prefAppliedHeader $ Preferences Nothing preferRepresentation Nothing preferCount preferTransaction Nothing preferHandling []
cTHeader = contentTypeHeaders mrMedia ctxApiRequest

let (status, headers, body) =
let isInsertIfGTZero i = if i > 0 then HTTP.status201 else HTTP.status200
upsertStatus = isInsertIfGTZero $ fromJust rsInserted
(status, headers, body) =
case preferRepresentation of
Just Full -> (HTTP.status200, cTHeader ++ prefHeader, LBS.fromStrict rsBody)
Just Full -> (upsertStatus, cTHeader ++ prefHeader, LBS.fromStrict rsBody)
Just None -> (HTTP.status204, prefHeader, mempty)
_ -> (HTTP.status204, prefHeader, mempty)
(ovStatus, ovHeaders) <- overrideStatusHeaders rsGucStatus rsGucHeaders status headers
10 changes: 4 additions & 6 deletions test/spec/Feature/Query/MultipleSchemaSpec.hs
Original file line number Diff line number Diff line change
@@ -225,13 +225,11 @@ spec =

it "succeeds on PUT on the v2 schema" $
request methodPut "/children?id=eq.111" [("Content-Profile", "v2"), ("Prefer", "return=representation")]
[json| [ { "id": 111, "name": "child v2-111", "parent_id": null } ]|]
[json|[{"id": 111, "name": "child v2-111", "parent_id": null}]|]
`shouldRespondWith`
[json|[{ "id": 111, "name": "child v2-111", "parent_id": null }]|]
{
matchStatus = 200
, matchHeaders = [matchContentTypeJson, "Content-Profile" <:> "v2"]
}
[json|[{"id": 111, "name": "child v2-111", "parent_id": null}]|]
{ matchStatus = 201
, matchHeaders = [matchContentTypeJson, "Content-Profile" <:> "v2"]}

context "OpenAPI output" $ do
it "succeeds in reading table definition from default schema v1 if no schema is selected via header" $ do
10 changes: 5 additions & 5 deletions test/spec/Feature/Query/PlanSpec.hs
Original file line number Diff line number Diff line change
@@ -192,7 +192,7 @@ spec actualPgVersion = do
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
totalCost `shouldBe` 3.55

it "outputs the total cost for 2 upserts" $ do
r <- request methodPost "/tiobe_pls"
@@ -206,7 +206,7 @@ spec actualPgVersion = do
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` 3.27
totalCost `shouldBe` 5.53

it "outputs the total cost for an upsert with 10 rows" $ do
r <- request methodPost "/tiobe_pls"
@@ -220,7 +220,7 @@ spec actualPgVersion = do
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` 3.27
totalCost `shouldBe` 5.53

it "outputs the total cost for an upsert with 100 rows" $ do
r <- request methodPost "/tiobe_pls"
@@ -234,7 +234,7 @@ spec actualPgVersion = do
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` 3.27
totalCost `shouldBe` 5.53

it "outputs the total cost for an upsert with 1000 rows" $ do
r <- request methodPost "/tiobe_pls"
@@ -248,7 +248,7 @@ spec actualPgVersion = do
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` 3.27
totalCost `shouldBe` 5.53

it "outputs the plan for application/vnd.pgrst.object" $ do
r <- request methodDelete "/projects?id=eq.6"
6 changes: 3 additions & 3 deletions test/spec/Feature/Query/ServerTimingSpec.hs
Original file line number Diff line number Diff line change
@@ -45,11 +45,11 @@ spec =
}

it "works with put request" $
request methodPut "/tiobe_pls?name=eq.Go"
request methodPut "/tiobe_pls?name=eq.Python"
[("Prefer", "return=representation")]
[json| [ { "name": "Go", "rank": 19 } ]|]
[json| [ { "name": "Python", "rank": 19 } ]|]
`shouldRespondWith`
[json| [ { "name": "Go", "rank": 19 } ]|]
[json| [ { "name": "Python", "rank": 19 } ]|]
{ matchStatus = 200
, matchHeaders = map matchServerTimingHasTiming ["jwt", "plan", "query", "render"]
}
27 changes: 25 additions & 2 deletions test/spec/Feature/Query/UpsertSpec.hs
Original file line number Diff line number Diff line change
@@ -32,6 +32,21 @@ spec actualPgVersion =
, matchHeaders = ["Preference-Applied" <:> "resolution=merge-duplicates, return=representation", matchContentTypeJson]
}

it "UPDATEs rows on pk conflict" $
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]
}

it "INSERTs and UPDATEs row on composite pk conflict" $
request methodPost "/employees" [("Prefer", "return=representation"), ("Prefer", "resolution=merge-duplicates")]
[json| [
@@ -62,7 +77,7 @@ spec actualPgVersion =
it "succeeds when the payload has no elements" $
request methodPost "/articles" [("Prefer", "return=representation"), ("Prefer", "resolution=merge-duplicates")]
[json|[]|] `shouldRespondWith`
[json|[]|] { matchStatus = 201
[json|[]|] { matchStatus = 200 -- nothing was inserted, so it should be 200
, matchHeaders = [matchContentTypeJson] }

it "INSERTs and UPDATEs rows on single unique key conflict" $
@@ -282,6 +297,7 @@ spec actualPgVersion =
[json| [ { "name": "Go", "rank": 19 } ]|]
`shouldRespondWith`
[json| [ { "name": "Go", "rank": 19 } ]|]
{ matchStatus = 201 }

it "succeeds on table with composite pk" $ do
-- assert that the next request will indeed be an insert
@@ -294,6 +310,7 @@ spec actualPgVersion =
[json| [ { "first_name": "Susan", "last_name": "Heidt", "salary": "48000", "company": "GEX", "occupation": "Railroad engineer" } ]|]
`shouldRespondWith`
[json| [ { "first_name": "Susan", "last_name": "Heidt", "salary": "$48,000.00", "company": "GEX", "occupation": "Railroad engineer" } ]|]
{ matchStatus = 201 }

when (actualPgVersion >= pgVersion110) $
it "succeeds on a partitioned table with composite pk" $ do
@@ -307,6 +324,7 @@ spec actualPgVersion =
[json| [ { "name": "Supra", "year": 2021 } ]|]
`shouldRespondWith`
[json| [ { "name": "Supra", "year": 2021, "car_brand_name": null } ]|]
{ matchStatus = 201 }

it "succeeds if the table has only PK cols and no other cols" $ do
-- assert that the next request will indeed be an insert
@@ -319,6 +337,7 @@ spec actualPgVersion =
[json|[ { "id": 10 } ]|]
`shouldRespondWith`
[json|[ { "id": 10 } ]|]
{ matchStatus = 201 }

context "Updating row" $ do
it "succeeds on table with single pk col" $ do
@@ -401,7 +420,11 @@ spec actualPgVersion =
request methodPut "/tiobe_pls?name=eq.Ruby"
[("Prefer", "return=representation"), ("Accept", "application/vnd.pgrst.object+json")]
[json| [ { "name": "Ruby", "rank": 11 } ]|]
`shouldRespondWith` [json|{ "name": "Ruby", "rank": 11 }|] { matchHeaders = [matchContentTypeSingular] }
`shouldRespondWith`
[json|{ "name": "Ruby", "rank": 11 }|]
{ matchStatus = 201
, matchHeaders = [matchContentTypeSingular] }


context "with a camel case pk column" $ do
it "works with POST and merge-duplicates" $ do
Loading

0 comments on commit 7a868f6

Please sign in to comment.