Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow calling variadic functions with repeated parameters #1603

Merged

Conversation

wolfgangwalther
Copy link
Member

@wolfgangwalther wolfgangwalther commented Oct 4, 2020

Remake of #1552 - new PR for a clean history and easier discussion. Resolves #1470.

This implements variadic arguments for RPCs by either:

  • using JSON arrays in the function body or
  • using repeated query params

See the test cases for some examples.

To be able to implement this, I had to do the following in 3 commits

  • The first commit just improved the "accepts a variety of arguments" test case to include arrays. I had that done in the other PR already and included them here mainly to make sure I'm not breaking regular arrays with everything that follows.
  • The second commit is "just" a refactor. The basic idea is to move the column parsing and findProc from App to ApiRequest, to be able to later use the result of findProc already when parsing the query string - to make sure to only include repeated params for the variadic case and not for regular functions. This also lead to a change in TargetProc, which is now passing the ProcDescription instead of only a QualifiedIdentifier around. To still support the fallback case (i.e. when the schema cache is stale or something) I added a fallback ProcDescription to findProc which basically serves only as a container for the identifier which was passed around in that case before. This change also has the side-effect of providing some defaults (volatility, return type), so the places where those were used do not need to make a decision about fallbacks anymore.
  • The third commit is the actual implementation of the variadic stuff. Tests and most stuff is almost identical to the changes in the other PR, where they had already been reviewed. The new stuff is all in ApiRequest's payloadColumns and mapFromListWithMultiple. The latter just appends all params into a list and passes them either as a JSON Array (if variadic) or takes the first element of that list (if non-variadic). For the variadic decision, the result of findProc is needed, which in turn is based on payloadColumns. To avoid a circular dependency I had to change payloadColumns a bit, so that it does not depend on payload (which depends on mapFromListWithMultiple...) anymore, at least in the cases where we parse the query string.

Note:
The n=1 and n>1 cases are handled through mapFromListWithMultiple or an array in the JSON body. For n=0:

  • with a JSON body an empty array can be used for the variadic argument
  • to call it without arguments through either the query string or by omitting the argument in the json body, one needs to add a DEFAULT '{}' to the functions variadic argument definition. This is in line with the default behaviour of postgres.

@wolfgangwalther wolfgangwalther force-pushed the fix-variadic-repeated-params branch from 01b2812 to 2c14ddd Compare October 12, 2020 21:10
@wolfgangwalther
Copy link
Member Author

Rebased on current master and refactored:

  • ApiRequest.hs: mapFromListWithMultiple renamed to paramsFromList with a lot less logic inside. returns RpcParams. Moved part of the isVariadic function to a helper in Types.hs.
  • Types.hs: Added RpcParams and RpcParamValue types, including toJSON instance and some helper functions

src/PostgREST/Types.hs Outdated Show resolved Hide resolved
src/PostgREST/ApiRequest.hs Show resolved Hide resolved
@wolfgangwalther wolfgangwalther force-pushed the fix-variadic-repeated-params branch from 2c14ddd to 15a0570 Compare October 13, 2020 08:17
@wolfgangwalther wolfgangwalther force-pushed the fix-variadic-repeated-params branch 2 times, most recently from 9ac55c2 to a559f53 Compare October 14, 2020 10:00
@wolfgangwalther wolfgangwalther force-pushed the fix-variadic-repeated-params branch from a559f53 to bb3a481 Compare October 14, 2020 14:38
CHANGELOG.md Outdated Show resolved Hide resolved
@steve-chavez
Copy link
Member

Only one thing left to do for me: run a load test and see if the PR has an impact on non-variadic RPC.

(Once #1609 is finished, we'll check this automatically)

@wolfgangwalther
Copy link
Member Author

Only one thing left to do for me: run a load test and see if the PR has an impact on non-variadic RPC.

In the meantime I will have to dig out the piece of documentation I had already started and update that.

@wolfgangwalther wolfgangwalther force-pushed the fix-variadic-repeated-params branch from bb3a481 to 35da734 Compare October 16, 2020 06:24
@wolfgangwalther wolfgangwalther changed the title Fix variadic repeated params Allow calling variadic functions with repeated parameters Oct 23, 2020
@wolfgangwalther wolfgangwalther force-pushed the fix-variadic-repeated-params branch from 35da734 to 62893c6 Compare October 23, 2020 20:54
@wolfgangwalther
Copy link
Member Author

rebased on master with merge conflict resolved; docs PR is done.

@steve-chavez
Copy link
Member

I think I can refactor this a bit, but I'll do it on another PR.

Looks good to merge 🚀

(Docs on PostgREST/postgrest-docs#357)

@steve-chavez steve-chavez merged commit 302d4e1 into PostgREST:master Oct 26, 2020
@wolfgangwalther wolfgangwalther deleted the fix-variadic-repeated-params branch October 28, 2020 18:26
@steve-chavez steve-chavez mentioned this pull request Oct 4, 2022
27 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

VARIADIC (Functions with variable numbers of arguments) is not supported with any array type
2 participants