feat: RPC POST for function w/single unnamed param #1927

steve-chavez · 2021-08-25T00:48:39Z

Closes #1735. For POST on RPC, allows:

Passing a json object without using Prefer: params=single-object. The function must be defined with a single unnamed json or jsonb param and Content-Type: application/json must be specified.
Uploading binary to a function. The function must be defined with a single unnamed bytea param and Content-Type: application/octet-stream must be specified.
Uploading raw text to a function. The function must be defined with a single unnamed text param and Content-Type: text/plain must be specified.

BREAKING CHANGE for Overloaded Functions If there's a function my_func having a single unnamed json param and other overloaded pairs(with any number of params), PostgREST won't be able to resolve a POST request to my_func. For solving this, you can name the unnamed json param.

my_func(json) -> my_func(prm json)

steve-chavez

Explanation of the breaking change

steve-chavez · 2021-08-25T01:09:30Z

test/fixtures/schema.sql

+  select $1;
+$$ language sql;
+
+create or replace function test.overloaded_unnamed_param(x int, y int) returns int as $$
+  select x + y;
+$$ language sql;


Having a func with an unnamed json parameter plus any number of overloaded pairs

Somehow it doesn't show where, but the above function is:

create or replace function test.overloaded_unnamed_param(json) returns int as $$ select $1; $$ language sql;

steve-chavez · 2021-08-25T01:14:20Z

test/Feature/RpcSpec.hs

+        it "will not be able to resolve when a single unnamed json parameter exists and other overloaded functions exist" $
+          request methodPost "/rpc/overloaded_unnamed_param" [("Content-Type", "application/json")]
+              [json|{"x": 1, "y": 2}|]
+            `shouldRespondWith`
+              [json| {
+                "hint":"Try changing the parameters' names or the function name so function overloading can be resolved",
+                "message":"Could not choose the best candidate function between: test.overloaded_unnamed_param( => json), test.overloaded_unnamed_param(x => integer, y => integer)"}|]
+              { matchStatus  = 300
+              , matchHeaders = [matchContentTypeJson]
+              }


PostgREST won't be able to resolve the function called with POST because we match the function's parameters with the json keys and the unnamed parameter basically absorbs all the json keys into one parameter.

Unfortunately I don't think there's a way to avoid this breaking change.

Unfortunately I don't think there's a way to avoid this breaking change.

It might be possible if a special route is added for the unnamed param, like /rpc/my_func/raw, but that would be too ugly I think.

It might be possible if a special route is added for the unnamed param, like /rpc/my_func/raw, but that would be too ugly I think.

I agree on the ugliness. I think this should be left for another PR on supporting overloaded functions in general.

What about the following:

get the keys from the json payload like we do

if there is any function in the cache that matches the keys (i.e. has all keys as arguments and does not require any other arguments, that are not in the payload) -> call function via named arguments

if no function is found, fallback to passing via unnamed json object

This would be non-breaking, but still support the same use-case we do right now: Without overloaded functions, it's the same. But additionally, it would allow to define kind of a "catch-all" function, that matches everything that is not defined specifically via a different overload.

@wolfgangwalther I think it's a nice approach to overcome the issue. I've opened the Draft PR #1996 to implement it, we could continue the idea there.

steve-chavez · 2021-08-25T01:28:45Z

src/PostgREST/Request/ApiRequest.hs

+      else if isInvPost && length params == 1 && (ppName <$> headMay params) == Just mempty
+        then case headMay params of
+          Just prm | contentType == CTApplicationJSON -> ppType prm `elem` ["json", "jsonb"]
+                   | contentType == CTTextPlain       -> ppType prm == "text"
+                   | contentType == CTOctetStream     -> ppType prm == "bytea"
+                   | otherwise                        -> False
+          Nothing  -> False


Match the function with the single unnamed parameter and content types.

It's necessary to only consider POST(isInvPost) because otherwise we'd also break GET requests that also have overloaded functions with a single unnamed json param. contentType isn't enough to determine we're in a POST because a GET can also send a Content-Type header despite having no body. Also, we default to application/json for all requests included GET.

If the POST condition is removed, some tests fail already.

laurenceisla · 2021-08-25T20:13:18Z

src/PostgREST/Error.hs

+    "hint"    .= ("Try changing the parameters' names or the function name so function overloading can be resolved" :: Text),
+    "message" .= ("Could not choose the best candidate function between: " <> T.intercalate ", " [pdSchema p <> "." <> pdName p <> "(" <> T.intercalate ", " [ppName a <> " => " <> ppType a | a <- pdParams p] <> ")" | p <- procs])]


Suggested change

"hint" .= ("Try changing the parameters' names or the function name so function overloading can be resolved" :: Text),

"message" .= ("Could not choose the best candidate function between: " <> T.intercalate ", " [pdSchema p <> "." <> pdName p <> "(" <> T.intercalate ", " [ppName a <> " => " <> ppType a | a <- pdParams p] <> ")" | p <- procs])]

"hint" .= ("Try renaming the parameters or the function itself in the database to resolve the function overloading issue" :: Text),

"details" .= ("Overloaded functions with the same argument name but different types are not supported"),

"message" .= ("Could not choose the best candidate function between: " <> T.intercalate ", " [pdSchema p <> "." <> pdName p <> "(" <> T.intercalate ", " [ppName a <> " => " <> ppType a | a <- pdParams p] <> ")" | p <- procs])]

I think it should be specified that the change should be done in the database, otherwise, at first glance, it may look like the change should be done on the query.
Also, I think the old hint should be kept and moved to the "detail" field.
WDYT?

I think it should be specified that the change should be done in the database, otherwise, at first glance, it may look like the change should be done on the query

True, I agree.

Also, I think the old hint should be kept and moved to the "detail" field. WDYT?

The thing is that the "Overloaded functions with the same argument name but different types are not supported" error is no longer accurate now. This can be noted on the test I've added:

it "will not be able to resolve when a single unnamed json parameter exists and other overloaded functions exist" $ request methodPost "/rpc/overloaded_unnamed_param" [("Content-Type", "application/json")] [json|{"x": 1, "y": 2}|] `shouldRespondWith` [json| { "hint":"Try changing the parameters' names or the function name so function overloading can be resolved", "message":"Could not choose the best candidate function between: test.overloaded_unnamed_param( => json), test.overloaded_unnamed_param(x => integer, y => integer)"}|] { matchStatus = 300 , matchHeaders = [matchContentTypeJson] }

The above is not related to having the same parameter name with different types.

Clarify the difference between arguments and parameters. Parameters are part of the function definition, arguments are the values passed to the function. Also clarify the findProc function comments and error message.

Shortens requestToCallProcQuery, which should generate SQL in a more direct way.

For POST on RPC, allows: * passing a json object without using `Prefer: params=single-object` The function must be defined with a single unnamed json param and `Content-Type: application/json` must be specified. * uploading binary to a function The function must be defined with a single unnamed bytea param and `Content-Type: application/octet-stream` must be specified. * uploading raw text to a function The function must be defined with a single unnamed text param and `Content-Type: text/plain` must be specified. BREAKING CHANGE If there's a function "my_func" having a single unnamed json param and other overloaded pairs(with any number of params), PostgREST won't be able to resolve a POST request to "my_func". For solving this, you can name the unnamed json param. my_func(json) -> my_func(prm json)

steve-chavez · 2021-08-30T23:16:44Z

stack on FreeBSD is failing with a really weird error:

/tmp/cirrus-ci-build/src/PostgREST/Request/Types.hs:44:44: error:
    Module ‘PostgREST.DbStructure.Proc’ does not export ‘ProcParam(..)’
   |
44 | import PostgREST.DbStructure.Proc         (ProcParam (..))

Which doesn't make sense because PostgREST.DbStructure.Proc does export ProcParam.

I'll merge now and revisit that one(if it keeps failing) later.

steve-chavez · 2021-08-31T00:19:39Z

Same failure happening on master. Must be something related to stack. I see Nix being supported on FreeBSD, wonder if we can switch to it there.

Avoids the breaking change in #1927: If there's a function "my_func" having a single unnamed json param and other overloaded pairs(with any number of params), PostgREST won't be able to resolve a POST request to "my_func".

steve-chavez commented Aug 25, 2021

View reviewed changes

laurenceisla reviewed Aug 25, 2021

View reviewed changes

steve-chavez force-pushed the unnamed-single-param branch from fef5837 to bd8cb60 Compare August 30, 2021 19:23

steve-chavez added 3 commits August 30, 2021 17:47

refactor: PgArg to ProcParam

5c2ba1e

Clarify the difference between arguments and parameters. Parameters are part of the function definition, arguments are the values passed to the function. Also clarify the findProc function comments and error message.

refactor: add CallQuery type

3293c7a

Shortens requestToCallProcQuery, which should generate SQL in a more direct way.

steve-chavez force-pushed the unnamed-single-param branch from bd8cb60 to a7eab89 Compare August 30, 2021 22:47

steve-chavez merged commit d4c6abb into PostgREST:main Aug 30, 2021

steve-chavez mentioned this pull request Aug 30, 2021

Document calling json with single unnamed param PostgREST/postgrest-docs#431

Closed

steve-chavez mentioned this pull request Sep 4, 2021

Changes to allow limit 0 #1929

Closed

This was referenced Oct 26, 2021

Allow rpc POST with single unnamed parameter PostgREST/postgrest-docs#451

Merged

Allow overloaded functions if one has a single unnamed JSON param #1996

Merged

laurenceisla mentioned this pull request Nov 17, 2021

Stored Procedure that returns a table type is colliding with another homonym stored procedure though having different parameters signature, resulting in unexpected errors #1672

Closed

fjf2002 mentioned this pull request Mar 9, 2022

feat: support XML? #2188

Open

fjf2002 mentioned this pull request Jun 2, 2022

RPC POST for function w/single unnamed XML param #2300

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: RPC POST for function w/single unnamed param #1927

feat: RPC POST for function w/single unnamed param #1927

steve-chavez commented Aug 25, 2021 •

edited

Loading

steve-chavez left a comment

steve-chavez Aug 25, 2021

steve-chavez Aug 25, 2021

steve-chavez Aug 25, 2021

steve-chavez Aug 25, 2021

steve-chavez Aug 25, 2021

laurenceisla Aug 25, 2021

wolfgangwalther Oct 27, 2021 •

edited

Loading

laurenceisla Oct 28, 2021

steve-chavez Aug 25, 2021 •

edited

Loading

laurenceisla Aug 25, 2021

steve-chavez Aug 27, 2021

steve-chavez commented Aug 30, 2021

steve-chavez commented Aug 31, 2021

		"hint" .= ("Try changing the parameters' names or the function name so function overloading can be resolved" :: Text),
		"message" .= ("Could not choose the best candidate function between: " <> T.intercalate ", " [pdSchema p <> "." <> pdName p <> "(" <> T.intercalate ", " [ppName a <> " => " <> ppType a \| a <- pdParams p] <> ")" \| p <- procs])]

feat: RPC POST for function w/single unnamed param #1927

feat: RPC POST for function w/single unnamed param #1927

Conversation

steve-chavez commented Aug 25, 2021 • edited Loading

steve-chavez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolfgangwalther Oct 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steve-chavez Aug 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steve-chavez commented Aug 30, 2021

steve-chavez commented Aug 31, 2021

steve-chavez commented Aug 25, 2021 •

edited

Loading

wolfgangwalther Oct 27, 2021 •

edited

Loading

steve-chavez Aug 25, 2021 •

edited

Loading