Skip to content

Commit

Permalink
fix: PUT responding with status 200 when rows are inserted
Browse files Browse the repository at this point in the history
  • Loading branch information
taimoorzaeem committed Oct 11, 2023
1 parent 056c748 commit 23619ff
Show file tree
Hide file tree
Showing 11 changed files with 67 additions and 34 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- #2771, Add `Server-Timing` header with JWT duration - @taimoorzaeem
- #2698, Add config `jwt-cache-max-lifetime` and implement JWT caching - @taimoorzaeem
- #2943, Add `handling=strict/lenient` for Prefer header - @taimoorzaeem

### Fixed

- #2824, Fix range request with 0 rows and 0 offset return status 416 - @strengthless
- #1070, Fix PUT responding with 200 when rows are inserted - @taimoorzaeem

## [11.2.1] - 2023-10-03

Expand Down
3 changes: 2 additions & 1 deletion src/PostgREST/Query.hs
Original file line number Diff line number Diff line change
Expand Up @@ -188,13 +188,14 @@ 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
Expand Down
6 changes: 3 additions & 3 deletions src/PostgREST/Query/QueryBuilder.hs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ 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 " <> coalesceAddPgrstInsertedF <> " AND " <> intercalateSnippet " AND " (pgFmtLogicTree (QualifiedIdentifier mempty "pgrst_body") <$> putConditions)) <>
maybe mempty (\(oncDo, oncCols) ->
if null oncCols then
mempty
Expand All @@ -98,9 +98,9 @@ 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 then mempty else "WHERE " <> coalesceSubPgrstInsertedF)
) onConflct <> " " <>
returningF mainQi returnings
returningF mainQi returnings
where
cols = intercalateSnippet ", " $ pgFmtIdent . cfName <$> iCols

Expand Down
9 changes: 9 additions & 0 deletions src/PostgREST/Query/SqlFragment.hs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ module PostgREST.Query.SqlFragment
, fromJsonBodyF
, responseHeadersF
, responseStatusF
, coalesceAddPgrstInsertedF
, coalesceSubPgrstInsertedF
, currentSettingF
, returningF
, singleParameter
, sourceCTE
Expand Down Expand Up @@ -439,6 +442,12 @@ responseHeadersF = currentSettingF "response.headers"
responseStatusF :: SQL.Snippet
responseStatusF = currentSettingF "response.status"

coalesceAddPgrstInsertedF :: SQL.Snippet
coalesceAddPgrstInsertedF = "set_config('pgrst.inserted', (coalesce(" <> currentSettingF "pgrst.inserted" <> "::int, 0) + 1)::text, true) <> '0'"

coalesceSubPgrstInsertedF :: SQL.Snippet
coalesceSubPgrstInsertedF = "set_config('pgrst.inserted', (coalesce(" <> currentSettingF "pgrst.inserted" <> "::int, 0) - 1)::text, true) <> '-1'"

currentSettingF :: SQL.Snippet -> SQL.Snippet
currentSettingF setting =
-- nullif is used because of https://gist.github.com/steve-chavez/8d7033ea5655096903f3b52f8ed09a15
Expand Down
22 changes: 15 additions & 7 deletions src/PostgREST/Query/Statements.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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
-- ^ 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 ->
prepareWrite :: SQL.Snippet -> SQL.Snippet -> Bool -> Bool -> MediaType -> ResultAggregate ->
Maybe PreferRepresentation -> [Text] -> Bool -> SQL.Statement () ResultSet
prepareWrite selectQuery mutateQuery isInsert mt rAgg rep pKeys =
prepareWrite selectQuery mutateQuery isInsert isPut mt rAgg rep pKeys =
SQL.dynamicallyParameterized (mtSnippet mt snippet) decodeIt
where
checkUpsert snip = if isInsert && isPut then snip else "''"
pgrstInsertedF = checkUpsert "nullif(current_setting('pgrst.inserted', true),'')::int"
snippet =
"WITH " <> sourceCTE <> " AS (" <> mutateQuery <> ") " <>
"SELECT " <>
Expand All @@ -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 =
Expand All @@ -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 =
Expand All @@ -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
Expand All @@ -124,15 +130,16 @@ prepareCall rout callProcQuery selectQuery countQuery countTotal mt rAgg =
"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 =
Expand All @@ -150,6 +157,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 =
Expand Down
17 changes: 10 additions & 7 deletions src/PostgREST/Response.hs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,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
Expand Down Expand Up @@ -153,11 +154,11 @@ updateResponse MutateReadPlan{mrMedia} ctxApiRequest@ApiRequest{iPreferences=Pre
prefHeader = prefAppliedHeader $ Preferences Nothing preferRepresentation Nothing preferCount preferTransaction preferMissing preferHandling []
headers = catMaybes [contentRangeHeader, prefHeader] ++ serverTimingHeader serverTimingParams

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'

Expand All @@ -174,9 +175,11 @@ singleUpsertResponse MutateReadPlan{mrMedia} ctxApiRequest@ApiRequest{iPreferenc
sTHeader = serverTimingHeader serverTimingParams
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 ++ sTHeader ++ prefHeader, LBS.fromStrict rsBody)
Just Full -> (upsertStatus, cTHeader ++ sTHeader ++ prefHeader, LBS.fromStrict rsBody)
Just None -> (HTTP.status204, sTHeader ++ prefHeader, mempty)
_ -> (HTTP.status204, sTHeader ++ prefHeader, mempty)
(ovStatus, ovHeaders) <- overrideStatusHeaders rsGucStatus rsGucHeaders status headers
Expand Down
10 changes: 4 additions & 6 deletions test/spec/Feature/Query/MultipleSchemaSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test/spec/Feature/Query/PlanSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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 plan for application/vnd.pgrst.object" $ do
r <- request methodDelete "/projects?id=eq.6"
Expand Down
6 changes: 3 additions & 3 deletions test/spec/Feature/Query/ServerTimingSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,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 = [ matchHeaderPresent "Server-Timing" ]
}
Expand Down
10 changes: 9 additions & 1 deletion test/spec/Feature/Query/UpsertSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,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
Expand All @@ -294,6 +295,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
Expand All @@ -307,6 +309,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
Expand All @@ -319,6 +322,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
Expand Down Expand Up @@ -401,7 +405,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
Expand Down
12 changes: 7 additions & 5 deletions test/spec/Feature/RollbackSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,12 @@ shouldPersistMutations reqHeaders respHeaders = do

it "does persist put" $ do
request methodPut "/items?id=eq.0"
reqHeaders
[json|{"id":0}|]
reqHeaders
[json|{"id":0}|]
`shouldRespondWith`
[json|[{"id":0}]|]
{ matchHeaders = respHeaders }
[json|[{"id":0}]|]
{ matchStatus = 201
, matchHeaders = respHeaders }
get "/items?id=eq.0"
`shouldRespondWith`
[json|[{"id":0}]|]
Expand Down Expand Up @@ -175,7 +176,8 @@ shouldNotPersistMutations reqHeaders respHeaders = do
[json|{"id":0}|]
`shouldRespondWith`
[json|[{"id":0}]|]
{ matchHeaders = respHeaders }
{ matchStatus = 201
, matchHeaders = respHeaders }
get "/items?id=eq.0"
`shouldRespondWith`
[json|[]|]
Expand Down

0 comments on commit 23619ff

Please sign in to comment.