From 0c25f128250f427f470910104eea6b8bee589083 Mon Sep 17 00:00:00 2001 From: Wolfgang Walther Date: Fri, 20 Nov 2020 10:18:49 +0100 Subject: [PATCH] fix: fix output of RPCs returning scalar values with multiple rows resolves #1584 BREAKING CHANGE: Changed output of RPCs to match return type better: * single scalar return: value (unchanged) * setof scalar return: array of values (new; was array of objects) * single composite return: object (new; was array of objects) * setof composite return: array of objects (unchanged) * void return: nothing (new; was "null") A single OUT column is now treated as "composite" instead of "scalar", i.e. consistent with multiple OUT columns. --- src/PostgREST/App.hs | 14 ++- src/PostgREST/DbStructure.hs | 8 +- src/PostgREST/Private/QueryFragment.hs | 12 +- src/PostgREST/Statements.hs | 28 ++--- src/PostgREST/Types.hs | 6 + test/Feature/QuerySpec.hs | 24 ++-- test/Feature/RpcSpec.hs | 150 ++++++++++++++----------- test/fixtures/schema.sql | 6 + 8 files changed, 150 insertions(+), 98 deletions(-) diff --git a/src/PostgREST/App.hs b/src/PostgREST/App.hs index ddc50db4e..df1f31c4f 100644 --- a/src/PostgREST/App.hs +++ b/src/PostgREST/App.hs @@ -297,9 +297,8 @@ app dbStructure conf apiRequest = preferParams = iPreferParameters apiRequest pq = requestToCallProcQuery (QualifiedIdentifier pdSchema pdName) (specifiedProcArgs (iColumns apiRequest) proc) (iPayload apiRequest) returnsScalar preferParams returning - stm = callProcStatement returnsScalar pq q cq shouldCount (contentType == CTSingularJSON) - (contentType == CTTextCSV) (contentType `elem` rawContentTypes) (preferParams == Just MultipleObjects) - bField pgVer prepared + stm = callProcStatement returnsScalar returnsSingle pq q cq shouldCount (contentType == CTSingularJSON) + (contentType == CTTextCSV) (preferParams == Just MultipleObjects) bField pgVer prepared row <- H.statement mempty stm let (tableTotal, queryTotal, body, gucHeaders, gucStatus) = row gucs = (,) <$> gucHeaders <*> gucStatus @@ -351,6 +350,10 @@ app dbStructure conf apiRequest = case iTarget apiRequest of TargetProc proc _ -> procReturnsScalar proc _ -> False + returnsSingle = + case iTarget apiRequest of + TargetProc proc _ -> procReturnsSingle proc + _ -> False pgVer = pgVersion dbStructure profileH = contentProfileH <$> iProfile apiRequest @@ -401,7 +404,10 @@ responseContentTypeOrError accepts rawContentTypes action target = serves conten -} binaryField :: ContentType -> [ContentType] -> Bool -> ReadRequest -> Either Response (Maybe FieldName) binaryField ct rawContentTypes isScalarProc readReq - | isScalarProc = Right Nothing + | isScalarProc = + if ct `elem` rawContentTypes + then Right $ Just "pgrst_scalar" + else Right Nothing | ct `elem` rawContentTypes = let fieldName = headMay fldNames in if length fldNames == 1 && fieldName /= Just "*" diff --git a/src/PostgREST/DbStructure.hs b/src/PostgREST/DbStructure.hs index 2718dd7db..6682a0c25 100644 --- a/src/PostgREST/DbStructure.hs +++ b/src/PostgREST/DbStructure.hs @@ -226,8 +226,12 @@ procsSqlQuery = [q| tn.nspname AS schema, COALESCE(comp.relname, t.typname) AS name, p.proretset AS rettype_is_setof, - -- Only pg pseudo type that is a row type is 'record' - (t.typtype = 'c' or t.typtype = 'p' and t.typname = 'record') AS rettype_is_composite, + (t.typtype = 'c' + -- Only pg pseudo type that is a row type is 'record' + or t.typtype = 'p' and t.typname = 'record' + -- if any INOUT or OUT arguments present, treat as composite + or COALESCE(proargmodes::text[] && '{b,o}', false) + ) AS rettype_is_composite, p.provolatile FROM pg_proc p JOIN pg_namespace pn ON pn.oid = p.pronamespace diff --git a/src/PostgREST/Private/QueryFragment.hs b/src/PostgREST/Private/QueryFragment.hs index 36b8eccf1..f669bdaf7 100644 --- a/src/PostgREST/Private/QueryFragment.hs +++ b/src/PostgREST/Private/QueryFragment.hs @@ -90,11 +90,15 @@ asCsvF = asCsvHeaderF <> " || '\n' || " <> asCsvBodyF ")" asCsvBodyF = "coalesce(string_agg(substring(_postgrest_t::text, 2, length(_postgrest_t::text) - 2), '\n'), '')" -asJsonF :: SqlFragment -asJsonF = "coalesce(json_agg(_postgrest_t), '[]')::character varying" +asJsonF :: Bool -> SqlFragment +asJsonF returnsScalar + | returnsScalar = "coalesce(json_agg(_postgrest_t.pgrst_scalar), '[]')::character varying" + | otherwise = "coalesce(json_agg(_postgrest_t), '[]')::character varying" -asJsonSingleF :: SqlFragment --TODO! unsafe when the query actually returns multiple rows, used only on inserting and returning single element -asJsonSingleF = "coalesce(string_agg(row_to_json(_postgrest_t)::text, ','), '')::character varying " +asJsonSingleF :: Bool -> SqlFragment --TODO! unsafe when the query actually returns multiple rows, used only on inserting and returning single element +asJsonSingleF returnsScalar + | returnsScalar = "coalesce(string_agg(to_json(_postgrest_t.pgrst_scalar)::text, ','), '')::character varying" + | otherwise = "coalesce(string_agg(to_json(_postgrest_t)::text, ','), '')::character varying" asBinaryF :: FieldName -> SqlFragment asBinaryF fieldName = "coalesce(string_agg(_postgrest_t." <> pgFmtIdent fieldName <> ", ''), '')" diff --git a/src/PostgREST/Statements.hs b/src/PostgREST/Statements.hs index 468976a95..86b2c257f 100644 --- a/src/PostgREST/Statements.hs +++ b/src/PostgREST/Statements.hs @@ -74,8 +74,8 @@ createWriteStatement selectQuery mutateQuery wantSingle isInsert asCsv rep pKeys bodyF | rep `elem` [None, HeadersOnly] = "''" | asCsv = asCsvF - | wantSingle = asJsonSingleF - | otherwise = asJsonF + | wantSingle = asJsonSingleF False + | otherwise = asJsonF False selectF -- prevent using any of the column names in ?select= when no response is returned from the CTE @@ -108,9 +108,9 @@ createReadStatement selectQuery countQuery isSingle countTotal asCsv binaryField bodyF | asCsv = asCsvF - | isSingle = asJsonSingleF + | isSingle = asJsonSingleF False | isJust binaryField = asBinaryF $ fromJust binaryField - | otherwise = asJsonF + | otherwise = asJsonF False decodeStandard :: HD.Result ResultsWithCount decodeStandard = @@ -128,10 +128,10 @@ standardRow = (,,,,,) <$> nullableColumn HD.int8 <*> column HD.int8 type ProcResults = (Maybe Int64, Int64, ByteString, Either SimpleError [GucHeader], Either SimpleError (Maybe Status)) -callProcStatement :: Bool -> H.Snippet -> H.Snippet -> H.Snippet -> Bool -> - Bool -> Bool -> Bool -> Bool -> Maybe FieldName -> PgVersion -> Bool -> +callProcStatement :: Bool -> Bool -> H.Snippet -> H.Snippet -> H.Snippet -> Bool -> + Bool -> Bool -> Bool -> Maybe FieldName -> PgVersion -> Bool -> H.Statement () ProcResults -callProcStatement returnsScalar callProcQuery selectQuery countQuery countTotal isSingle asCsv asBinary multObjects binaryField pgVer = +callProcStatement returnsScalar returnsSingle callProcQuery selectQuery countQuery countTotal asSingle asCsv multObjects binaryField pgVer = H.dynamicallyParameterized snippet decodeProc where snippet = @@ -149,16 +149,12 @@ callProcStatement returnsScalar callProcQuery selectQuery countQuery countTotal (countCTEF, countResultF) = countF countQuery countTotal bodyF - | returnsScalar = scalarBodyF - | isSingle = asJsonSingleF - | asCsv = asCsvF + | asSingle = asJsonSingleF returnsScalar + | asCsv = asCsvF | isJust binaryField = asBinaryF $ fromJust binaryField - | otherwise = asJsonF - - scalarBodyF - | asBinary = asBinaryF "pgrst_scalar" - | multObjects = "json_agg(_postgrest_t.pgrst_scalar)::character varying" - | otherwise = "(json_agg(_postgrest_t.pgrst_scalar)->0)::character varying" + | returnsSingle + && not multObjects = asJsonSingleF returnsScalar + | otherwise = asJsonF returnsScalar decodeProc :: HD.Result ProcResults decodeProc = diff --git a/src/PostgREST/Types.hs b/src/PostgREST/Types.hs index 289296113..ae13910b9 100644 --- a/src/PostgREST/Types.hs +++ b/src/PostgREST/Types.hs @@ -203,8 +203,14 @@ specifiedProcArgs keys proc = procReturnsScalar :: ProcDescription -> Bool procReturnsScalar proc = case proc of ProcDescription{pdReturnType = (Single (Scalar _))} -> True + ProcDescription{pdReturnType = (SetOf (Scalar _))} -> True _ -> False +procReturnsSingle :: ProcDescription -> Bool +procReturnsSingle proc = case proc of + ProcDescription{pdReturnType = (Single _)} -> True + _ -> False + procTableName :: ProcDescription -> Maybe TableName procTableName proc = case pdReturnType proc of SetOf (Composite qi) -> Just $ qiName qi diff --git a/test/Feature/QuerySpec.hs b/test/Feature/QuerySpec.hs index de3836e21..e7098f70a 100644 --- a/test/Feature/QuerySpec.hs +++ b/test/Feature/QuerySpec.hs @@ -827,14 +827,22 @@ spec actualPgVersion = do it "fails if a single column is not selected" $ do request methodGet "/images?select=img,name&name=eq.A.png" (acceptHdrs "application/octet-stream") "" `shouldRespondWith` - [json| {"message":"application/octet-stream requested but more than one column was selected"} |] - { matchStatus = 406 - , matchHeaders = [matchContentTypeJson] - } - request methodGet "/images?select=*&name=eq.A.png" (acceptHdrs "application/octet-stream") "" - `shouldRespondWith` 406 - request methodGet "/images?name=eq.A.png" (acceptHdrs "application/octet-stream") "" - `shouldRespondWith` 406 + [json| {"message":"application/octet-stream requested but more than one column was selected"} |] + { matchStatus = 406 } + + request methodGet "/images?select=*&name=eq.A.png" + (acceptHdrs "application/octet-stream") + "" + `shouldRespondWith` + [json| {"message":"application/octet-stream requested but more than one column was selected"} |] + { matchStatus = 406 } + + request methodGet "/images?name=eq.A.png" + (acceptHdrs "application/octet-stream") + "" + `shouldRespondWith` + [json| {"message":"application/octet-stream requested but more than one column was selected"} |] + { matchStatus = 406 } it "concatenates results if more than one row is returned" $ request methodGet "/images_base64?select=img&name=in.(A.png,B.png)" (acceptHdrs "application/octet-stream") "" diff --git a/test/Feature/RpcSpec.hs b/test/Feature/RpcSpec.hs index b6be576c5..f0f7ba032 100644 --- a/test/Feature/RpcSpec.hs +++ b/test/Feature/RpcSpec.hs @@ -167,12 +167,13 @@ spec actualPgVersion = `shouldRespondWith` 400 it "can embed if the related tables are in a hidden schema but exposed as views" $ do - post "/rpc/single_article?select=id,articleStars(userId)" [json|{ "id": 2}|] - `shouldRespondWith` [json|[{"id": 2, "articleStars": [{"userId": 3}]}]|] - { matchHeaders = [matchContentTypeJson] } + post "/rpc/single_article?select=id,articleStars(userId)" + [json|{ "id": 2}|] + `shouldRespondWith` + [json|{"id": 2, "articleStars": [{"userId": 3}]}|] get "/rpc/single_article?id=2&select=id,articleStars(userId)" - `shouldRespondWith` [json|[{"id": 2, "articleStars": [{"userId": 3}]}]|] - { matchHeaders = [matchContentTypeJson] } + `shouldRespondWith` + [json|{"id": 2, "articleStars": [{"userId": 3}]}|] it "can embed an M2M relationship table" $ get "/rpc/getallusers?select=name,tasks(name)&id=gt.1" @@ -234,11 +235,10 @@ spec actualPgVersion = { matchHeaders = [matchContentTypeJson] } it "returns setof integers" $ - post "/rpc/ret_setof_integers" [json|{}|] `shouldRespondWith` - [json|[{ "ret_setof_integers": 1 }, - { "ret_setof_integers": 2 }, - { "ret_setof_integers": 3 }]|] - { matchHeaders = [matchContentTypeJson] } + post "/rpc/ret_setof_integers" + [json|{}|] + `shouldRespondWith` + [json|[1,2,3]|] it "returns enum value" $ post "/rpc/ret_enum" [json|{ "val": "foo" }|] `shouldRespondWith` @@ -261,34 +261,40 @@ spec actualPgVersion = { matchHeaders = [matchContentTypeJson] } it "returns composite type in exposed schema" $ - post "/rpc/ret_point_2d" [json|{}|] `shouldRespondWith` - [json|[{"x": 10, "y": 5}]|] - { matchHeaders = [matchContentTypeJson] } + post "/rpc/ret_point_2d" + [json|{}|] + `shouldRespondWith` + [json|{"x": 10, "y": 5}|] it "cannot return composite type in hidden schema" $ post "/rpc/ret_point_3d" [json|{}|] `shouldRespondWith` 401 when (actualPgVersion >= pgVersion110) $ it "returns domain of composite type" $ - post "/rpc/ret_composite_domain" [json|{}|] `shouldRespondWith` - [json|[{"x": 10, "y": 5}]|] - { matchHeaders = [matchContentTypeJson] } + post "/rpc/ret_composite_domain" + [json|{}|] + `shouldRespondWith` + [json|{"x": 10, "y": 5}|] it "returns single row from table" $ - post "/rpc/single_article?select=id" [json|{"id": 2}|] `shouldRespondWith` - [json|[{"id": 2}]|] - { matchHeaders = [matchContentTypeJson] } + post "/rpc/single_article?select=id" + [json|{"id": 2}|] + `shouldRespondWith` + [json|{"id": 2}|] - it "returns null for void" $ - post "/rpc/ret_void" [json|{}|] `shouldRespondWith` - [json|null|] - { matchHeaders = [matchContentTypeJson] } + it "returns nothing for void" $ + post "/rpc/ret_void" + [json|{}|] + `shouldRespondWith` + "" + { matchHeaders = [matchContentTypeJson] } context "different types when overloaded" $ do it "returns composite type" $ - post "/rpc/ret_point_overloaded" [json|{"x": 1, "y": 2}|] `shouldRespondWith` - [json|[{"x": 1, "y": 2}]|] - { matchHeaders = [matchContentTypeJson] } + post "/rpc/ret_point_overloaded" + [json|{"x": 1, "y": 2}|] + `shouldRespondWith` + [json|{"x": 1, "y": 2}|] it "returns json scalar with prefer single object" $ request methodPost "/rpc/ret_point_overloaded" [("Prefer","params=single-object")] @@ -493,23 +499,29 @@ spec actualPgVersion = [json|[{"test":"hello","value":1}]|] { matchHeaders = [matchContentTypeJson] } context "procs with OUT/INOUT params" $ do - it "returns a scalar result when there is a single OUT param" $ do - get "/rpc/single_out_param?num=5" `shouldRespondWith` - [json|6|] { matchHeaders = [matchContentTypeJson] } - get "/rpc/single_json_out_param?a=1&b=two" `shouldRespondWith` - [json|{"a": 1, "b": "two"}|] { matchHeaders = [matchContentTypeJson] } + it "returns an object result when there is a single OUT param" $ do + get "/rpc/single_out_param?num=5" + `shouldRespondWith` + [json|{"num_plus_one":6}|] - it "returns a scalar result when there is a single INOUT param" $ - get "/rpc/single_inout_param?num=2" `shouldRespondWith` - [json|3|] { matchHeaders = [matchContentTypeJson] } + get "/rpc/single_json_out_param?a=1&b=two" + `shouldRespondWith` + [json|{"my_json": {"a": 1, "b": "two"}}|] - it "returns a row result when there are many OUT params" $ - get "/rpc/many_out_params" `shouldRespondWith` - [json|[{"my_json":{"a": 1, "b": "two"},"num":3,"str":"four"}]|] { matchHeaders = [matchContentTypeJson] } + it "returns an object result when there is a single INOUT param" $ + get "/rpc/single_inout_param?num=2" + `shouldRespondWith` + [json|{"num":3}|] - it "returns a row result when there are many INOUT params" $ - get "/rpc/many_inout_params?num=1&str=two&b=false" `shouldRespondWith` - [json| [{"num":1,"str":"two","b":false}]|] { matchHeaders = [matchContentTypeJson] } + it "returns an object result when there are many OUT params" $ + get "/rpc/many_out_params" + `shouldRespondWith` + [json|{"my_json":{"a": 1, "b": "two"},"num":3,"str":"four"}|] + + it "returns an object result when there are many INOUT params" $ + get "/rpc/many_inout_params?num=1&str=two&b=false" + `shouldRespondWith` + [json|{"num":1,"str":"two","b":false}|] context "procs with VARIADIC params" $ do when (actualPgVersion < pgVersion100) $ @@ -576,10 +588,12 @@ spec actualPgVersion = [json|"Hello, world"|] it "can handle procs with args that have a DEFAULT value" $ do - get "/rpc/many_inout_params?num=1&str=two" `shouldRespondWith` - [json| [{"num":1,"str":"two","b":true}]|] { matchHeaders = [matchContentTypeJson] } - get "/rpc/three_defaults?b=4" `shouldRespondWith` - [json|8|] { matchHeaders = [matchContentTypeJson] } + get "/rpc/many_inout_params?num=1&str=two" + `shouldRespondWith` + [json| {"num":1,"str":"two","b":true}|] + get "/rpc/three_defaults?b=4" + `shouldRespondWith` + [json|8|] it "can map a RAISE error code and message to a http status" $ get "/rpc/raise_pt402" @@ -619,18 +633,16 @@ spec actualPgVersion = context "should work with an overloaded function" $ do it "overloaded()" $ - get "/rpc/overloaded" `shouldRespondWith` - [json|[{ "overloaded": 1 }, - { "overloaded": 2 }, - { "overloaded": 3 }]|] - { matchHeaders = [matchContentTypeJson] } + get "/rpc/overloaded" + `shouldRespondWith` + [json|[1,2,3]|] it "overloaded(json) single-object" $ - request methodPost "/rpc/overloaded" [("Prefer","params=single-object")] - [json|[{"x": 1, "y": "first"}, {"x": 2, "y": "second"}]|] - `shouldRespondWith` - [json|[{"x": 1, "y": "first"}, {"x": 2, "y": "second"}]|] - { matchHeaders = [matchContentTypeJson] } + request methodPost "/rpc/overloaded" + [("Prefer","params=single-object")] + [json|[{"x": 1, "y": "first"}, {"x": 2, "y": "second"}]|] + `shouldRespondWith` + [json|[{"x": 1, "y": "first"}, {"x": 2, "y": "second"}]|] it "overloaded(int, int)" $ get "/rpc/overloaded?a=1&b=2" `shouldRespondWith` [str|3|] @@ -786,6 +798,17 @@ spec actualPgVersion = , matchHeaders = ["Content-Type" <:> "text/plain; charset=utf-8"] } + context "Proc that returns set of scalars" $ + it "can query without selecting column" $ + request methodGet "/rpc/welcome_twice" + (acceptHdrs "text/plain") + "" + `shouldRespondWith` + "Welcome to PostgRESTWelcome to PostgREST" + { matchStatus = 200 + , matchHeaders = ["Content-Type" <:> "text/plain; charset=utf-8"] + } + context "Proc that returns rows" $ do it "can query if a single column is selected" $ request methodPost "/rpc/ret_rows_with_base64_bin?select=img" (acceptHdrs "application/octet-stream") "" @@ -795,12 +818,11 @@ spec actualPgVersion = } it "fails if a single column is not selected" $ - request methodPost "/rpc/ret_rows_with_base64_bin" (acceptHdrs "application/octet-stream") "" + request methodPost "/rpc/ret_rows_with_base64_bin" + (acceptHdrs "application/octet-stream") "" `shouldRespondWith` - [json| {"message":"application/octet-stream requested but more than one column was selected"} |] - { matchStatus = 406 - , matchHeaders = [matchContentTypeJson] - } + [json| {"message":"application/octet-stream requested but more than one column was selected"} |] + { matchStatus = 406 } context "only for GET rpc" $ do it "should fail on mutating procs" $ do @@ -896,11 +918,11 @@ spec actualPgVersion = it "can set the same http header twice" $ get "/rpc/set_cookie_twice" - `shouldRespondWith` "null" - {matchHeaders = [ - matchContentTypeJson, - "Set-Cookie" <:> "sessionid=38afes7a8; HttpOnly; Path=/", - "Set-Cookie" <:> "id=a3fWa; Expires=Wed, 21 Oct 2015 07:28:00 GMT; Secure; HttpOnly"]} + `shouldRespondWith` + "" + { matchHeaders = [ matchContentTypeJson + , "Set-Cookie" <:> "sessionid=38afes7a8; HttpOnly; Path=/" + , "Set-Cookie" <:> "id=a3fWa; Expires=Wed, 21 Oct 2015 07:28:00 GMT; Secure; HttpOnly" ]} it "can override the Location header on a trigger" $ post "/stuff" diff --git a/test/fixtures/schema.sql b/test/fixtures/schema.sql index 35b90fd47..f4c613113 100644 --- a/test/fixtures/schema.sql +++ b/test/fixtures/schema.sql @@ -1636,6 +1636,12 @@ create or replace function welcome() returns text as $$ select 'Welcome to PostgREST'::text; $$ language sql; +create or replace function welcome_twice() returns setof text as $$ +select 'Welcome to PostgREST' +union all +select 'Welcome to PostgREST'; +$$ language sql; + create or replace function "welcome.html"() returns text as $_$ select $$