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

add support for calling UDFs with default arguments #427

Merged
merged 6 commits into from
Oct 5, 2023
Merged

Conversation

imor
Copy link
Contributor

@imor imor commented Oct 4, 2023

What kind of change does this PR introduce?

Feature

What is the current behavior?

UDFs with default arguments were not supported.

What is the new behavior?

UDFs with default arguments can be called now. Arguments with default values can be omitted in the GraphQL field's args.

Additional context

N/A

@imor imor marked this pull request as ready for review October 4, 2023 08:09
Copy link
Contributor

@olirice olirice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

"type": { +
"name": "Int" +
}, +
"defaultValue": null+
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current the defaults do not render in the introspection schema
that's where IDEs like GraphiQL pull the info and do syntax highlighting from so we should support it there too

Copy link
Contributor Author

@imor imor Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The defaults can be extracted from the proargdefaults column of the pg_proc table. This column's type is pg_node_tree which can be tricky to parse as it looks like this: ({CONST :consttype 16 :consttypmod -1 :constcollid 0 :constlen 1 :constbyval true :constisnull false :location 132 :constvalue 1 [ 0 0 0 0 0 0 0 0 ]}).

Calling pg_get_expr built-in function on a pg_node_tree type returns a string representation which looks more manageable but can still be quite involved for complex values like json & jsonb.

Some example values for pg_get_expr(proargdefaults) below:

postgres=# select proname, pronargs, pg_get_expr(proargdefaults, 0) from pg_catalog.pg_proc where proargdefaults is not null;
               proname               | pronargs |            pg_get_expr
-------------------------------------+----------+------------------------------------
 jsonb_insert                        |        4 | false
 jsonb_path_query                    |        4 | '{}'::jsonb, false
 jsonb_path_exists_tz                |        4 | '{}'::jsonb, false
 jsonb_path_query_array_tz           |        4 | '{}'::jsonb, false
 pg_logical_slot_peek_changes        |        4 | '{}'::text[]
 pg_logical_slot_peek_binary_changes |        4 | '{}'::text[]
 pg_create_logical_replication_slot  |        4 | false, false
 is_normalized                       |        2 | 'NFC'::text
 jsonb_set_lax                       |        5 | true, 'use_json_null'::text
 parse_ident                         |        2 | true
 pg_terminate_backend                |        2 | 0
 json_populate_record                |        3 | false
 json_populate_recordset             |        3 | false
 pg_logical_slot_get_changes         |        4 | '{}'::text[]
 pg_logical_slot_get_binary_changes  |        4 | '{}'::text[]
 pg_create_physical_replication_slot |        3 | false, false
 make_interval                       |        7 | 0, 0, 0, 0, 0, 0, 0.0
 jsonb_set                           |        4 | true
 pg_backup_start                     |        2 | false
 jsonb_path_exists                   |        4 | '{}'::jsonb, false
 jsonb_path_match                    |        4 | '{}'::jsonb, false
 jsonb_path_query_array              |        4 | '{}'::jsonb, false
 jsonb_path_query_first              |        4 | '{}'::jsonb, false
 jsonb_path_match_tz                 |        4 | '{}'::jsonb, false
 jsonb_path_query_tz                 |        4 | '{}'::jsonb, false
 jsonb_path_query_first_tz           |        4 | '{}'::jsonb, false
 normalize                           |        2 | 'NFC'::text
 pg_backup_stop                      |        1 | true
 pg_promote                          |        2 | true, 60
 install_extension                   |        5 | NULL::text[]
 concat_text                         |        2 | 'hello, hi'::text, 'ok, bye'::text
 get_json_obj                        |        2 | '{"key": "value::json"}'::json
 add_floats                          |        2 | ((1)::numeric + 3.14)
(33 rows)

Defaults for UDFs contact_text, get_json_obj and add_floats above are create function concat_text(a text default 'hello, hi', b text default 'ok, bye'), create function get_json_obj(key text, input json default '{ "key": "value::json" }') and create function add_floats(a float, b float default (1 + 3.14)) respectively.

We can probably parse literal expressions but more complex default expressions will likely still fail.

Note that Postgraphile returns null for defaultValue for function args.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that Postgraphile returns null for defaultValue for function args.

Interesting, yeah, sounds like a hard problem to do perfectly.
It does seem like handling simple scalar cases like int/bigint/text/varchar/bool would make for easier to understand APIs but its a backwards compatible change to show the default value

does that scope seem more reasonable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be OOO so will approve this. Up to you if you'd like to handle those scalar cases here or merge it & add that to #410

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented a simple algo to parse some basic types on a best effort basis. This can be made more sophisticated over time.

@olirice olirice self-requested a review October 4, 2023 17:55
This is done on the best effort basis and only for a small set of types because
parsing default expressions from pg_catalog is tedious.
@imor imor merged commit aeb1801 into master Oct 5, 2023
4 checks passed
@imor imor deleted the feat/default-args branch October 5, 2023 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants