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.
2020-12-10 19:55:42 +01:00
parent 093fd3c8f6
commit 0c25f12825
8 changed files with 150 additions and 98 deletions
@@ -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 "*"
@@ -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
@@ -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 <> ", ''), '')"
@@ -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 =
@@ -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
@@ -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") ""
@@ -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"
@@ -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 $$
 <html>