From 5a46a1678f9192a6aa8b7339b30163d78d8aa6bb Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 13:45:28 +0100 Subject: [PATCH 1/3] fix: allow deleting/adding UDF argument rows added in the current edit session canDeleteRow for the function/procedure Arguments grid checked whether the whole function was new rather than whether the individual row was new, so once a function was saved the delete icon was disabled for every argument row, including ones added but not yet saved (same bug pattern already fixed for enum values in #8208). canAdd had the same whole-object gate, hiding the "+" button entirely once a function was saved, so there was no way to add a row in the first place. Pre-existing (already persisted) arguments remain non-deletable, since PostgreSQL has no way to remove an argument from a function via CREATE OR REPLACE. Also fixes _update_arguments_for_get_sql, which only merged the 'changed' key of the arguments diff and silently dropped (or, without a 'changed' key at all, raised a 500) any newly added argument, so a row added via the now-enabled "+" button actually survives into the generated SQL. Closes #10252 --- .../databases/schemas/functions/__init__.py | 10 ++++- .../functions/static/js/function.ui.js | 12 +++--- .../functions/tests/test_function_get_msql.py | 40 +++++++++++++++++++ 3 files changed, 54 insertions(+), 8 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/__init__.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/__init__.py index 571afd7ef01..5d1dcd0772f 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/__init__.py @@ -1029,17 +1029,23 @@ def msql(self, gid, sid, did, scid, fnid=None): def _update_arguments_for_get_sql(data, old_data): """ If Function Definition/Arguments are changed then merge old - Arguments with changed ones for Create/Replace Function SQL statement + Arguments with changed/added ones for Create/Replace Function SQL + statement :param data: :param old_data: :return: """ if 'arguments' in data and len(data['arguments']) > 0: - for arg in data['arguments']['changed']: + for arg in data['arguments'].get('changed', []): for old_arg in old_data['arguments']: if arg['argid'] == old_arg['argid']: old_arg.update(arg) break + # Newly added arguments (not yet saved) need to be appended, + # otherwise they would be silently dropped as only pre-existing + # arguments are present in old_data. + for arg in data['arguments'].get('added', []): + old_data['arguments'].append(arg) data['arguments'] = old_data['arguments'] elif data['change_func']: data['arguments'] = old_data['arguments'] diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/static/js/function.ui.js b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/static/js/function.ui.js index c6f1b649441..1e57e9d92f8 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/static/js/function.ui.js +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/static/js/function.ui.js @@ -304,16 +304,16 @@ export default class FunctionSchema extends BaseUISchema { }, { id: 'arguments', label: gettext('Arguments'), cell: 'string', - group: gettext('Definition'), type: 'collection', canAdd: function(){ - return obj.isNew(); - }, + group: gettext('Definition'), type: 'collection', canDelete: true, mode: ['create', 'edit'], columns: ['argtype', 'argmode', 'argname', 'argdefval'], schema : new DefaultArgumentSchema(this.node_info, this.fieldOptions.getTypes), disabled: obj.inCatalog(), - canDeleteRow: function() { - return obj.isNew(); - }, + // Existing (already saved) arguments cannot be removed here, as + // PostgreSQL has no way to drop an argument from a function via + // CREATE OR REPLACE. Only rows added in the current session (not + // yet saved) can be deleted. + canDeleteRow: (state) => (this.isNew(state)), },{ id: 'prosrc', label: gettext('Code'), cell: 'text', type: 'sql', mode: ['properties', 'create', 'edit'], diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_function_get_msql.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_function_get_msql.py index d574f7d46a8..333d8bca270 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_function_get_msql.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_function_get_msql.py @@ -142,6 +142,43 @@ class FunctionGetmsqlTestCase(BaseTestGenerator): } ) ), + ( + 'Fetch Function msql with newly added argument', + dict( + url='/browser/function/msql/', + is_positive_test=True, + mocking_required=False, + with_function_id=True, + is_mock_local_function=False, + test_data={ + "name": "Test Function", + "funcowner": "", + "pronamespace": 2200, + "prorettypename": "character varying", + "lanname": "sql", + "prosrc": "select '1'", + "probin": "$libdir/", + "variables": [], + "seclabels": [], + "acl": [], + # A newly added (not yet saved) argument must survive + # into the generated SQL, and not be silently dropped. + "arguments": json.dumps({ + "added": [{ + "argname": "new_arg", + "argtype": "integer", + "argmode": "IN", + "argdefval": "1" + }] + }) + }, + mock_data={}, + expected_data={ + "status_code": 200, + "check_string": "new_arg" + } + ), + ), ( 'Fetch Function msql fetch properties not found', dict( @@ -222,5 +259,8 @@ def _get_sql(self, **kwargs): self.assertEqual(response.status_code, self.expected_data['status_code']) + if 'check_string' in self.expected_data: + self.assertIn(self.expected_data['check_string'], + response.json['data']) # Disconnect the database database_utils.disconnect_database(self, self.server_id, self.db_id) From abb615d5d6f66536de97335d4738c803726c5b97 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 20 Aug 2026 05:04:37 +0100 Subject: [PATCH 2/3] fix: reject adding a new argument to an existing function/procedure CREATE OR REPLACE FUNCTION cannot add an input argument to an existing routine: PostgreSQL treats a changed argument list as a distinct signature, so it creates a separate, orphaned overloaded routine instead of replacing this one, verified against a live PostgreSQL 18 instance. The previous commit's _update_arguments_for_get_sql change merged a newly added argument straight into the CREATE OR REPLACE statement, which would have silently done exactly that. Reject the edit explicitly instead, with a clear error, rather than letting it silently leave a phantom routine behind. Updates the msql test added in the previous commit to assert the rejection instead of successful SQL generation. --- .../databases/schemas/functions/__init__.py | 23 +++++++++++++------ .../functions/tests/test_function_get_msql.py | 16 +++++++++---- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/__init__.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/__init__.py index 5d1dcd0772f..80ba1787bd6 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/__init__.py @@ -1029,8 +1029,7 @@ def msql(self, gid, sid, did, scid, fnid=None): def _update_arguments_for_get_sql(data, old_data): """ If Function Definition/Arguments are changed then merge old - Arguments with changed/added ones for Create/Replace Function SQL - statement + Arguments with changed ones for Create/Replace Function SQL statement :param data: :param old_data: :return: @@ -1041,11 +1040,6 @@ def _update_arguments_for_get_sql(data, old_data): if arg['argid'] == old_arg['argid']: old_arg.update(arg) break - # Newly added arguments (not yet saved) need to be appended, - # otherwise they would be silently dropped as only pre-existing - # arguments are present in old_data. - for arg in data['arguments'].get('added', []): - old_data['arguments'].append(arg) data['arguments'] = old_data['arguments'] elif data['change_func']: data['arguments'] = old_data['arguments'] @@ -1200,6 +1194,21 @@ def _get_sql_for_edit_mode(self, data, parallel_dict, all_ids_dict, data[arg]) > 0) or arg in data: data['change_func'] = True + # PostgreSQL cannot add an input argument to an existing + # function/procedure via CREATE OR REPLACE: a changed argument + # list is a different signature, so PostgreSQL creates a new, + # separate overloaded routine instead of replacing this one. + # Reject such edits explicitly, rather than silently leaving an + # orphaned routine behind. + if 'arguments' in data and isinstance(data['arguments'], dict) \ + and data['arguments'].get('added'): + return False, gettext( + "Adding a new argument to an existing function/procedure " + "is not supported, as PostgreSQL would create a separate, " + "overloaded routine rather than replacing this one. " + "Please create a new function/procedure instead." + ), '' + # If Function Definition/Arguments are changed then merge old # Arguments with changed ones for Create/Replace Function # SQL statement diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_function_get_msql.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_function_get_msql.py index 333d8bca270..8902052b23f 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_function_get_msql.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_function_get_msql.py @@ -143,7 +143,7 @@ class FunctionGetmsqlTestCase(BaseTestGenerator): ) ), ( - 'Fetch Function msql with newly added argument', + 'Fetch Function msql with newly added argument is rejected', dict( url='/browser/function/msql/', is_positive_test=True, @@ -161,8 +161,11 @@ class FunctionGetmsqlTestCase(BaseTestGenerator): "variables": [], "seclabels": [], "acl": [], - # A newly added (not yet saved) argument must survive - # into the generated SQL, and not be silently dropped. + # PostgreSQL cannot add an argument to an existing + # function via CREATE OR REPLACE (it would create a + # separate, overloaded routine instead), so this must + # be rejected with a clear error rather than silently + # producing SQL that orphans a routine. "arguments": json.dumps({ "added": [{ "argname": "new_arg", @@ -174,8 +177,8 @@ class FunctionGetmsqlTestCase(BaseTestGenerator): }, mock_data={}, expected_data={ - "status_code": 200, - "check_string": "new_arg" + "status_code": 500, + "check_errormsg": "not supported" } ), ), @@ -262,5 +265,8 @@ def _get_sql(self, **kwargs): if 'check_string' in self.expected_data: self.assertIn(self.expected_data['check_string'], response.json['data']) + if 'check_errormsg' in self.expected_data: + self.assertIn(self.expected_data['check_errormsg'], + response.json['errormsg']) # Disconnect the database database_utils.disconnect_database(self, self.server_id, self.db_id) From f0bfe4342fc696e5fd9b2fcf6a0bb87908efdb22 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Tue, 25 Aug 2026 09:57:53 +0100 Subject: [PATCH 3/3] fix: distinguish argmode in the added-argument edit-mode guard Split the CREATE OR REPLACE guard for newly added function/procedure arguments by argmode: an added IN/INOUT/VARIADIC argument changes the routine's signature, so PostgreSQL creates a separate overloaded routine instead of replacing this one, while an added OUT argument does not affect the signature but changes the shape of the returned row, which PostgreSQL rejects outright (SQLSTATE 42P13). Each case now gets its own accurate rejection message, and both are covered by new regression tests for functions and procedures. --- .../databases/schemas/functions/__init__.py | 39 +++++--- .../functions/tests/test_function_get_msql.py | 47 +++++++++- .../functions/tests/test_procedure_put.py | 88 ++++++++++++++++--- 3 files changed, 149 insertions(+), 25 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/__init__.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/__init__.py index 80ba1787bd6..13c38f05483 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/__init__.py @@ -1194,20 +1194,35 @@ def _get_sql_for_edit_mode(self, data, parallel_dict, all_ids_dict, data[arg]) > 0) or arg in data: data['change_func'] = True - # PostgreSQL cannot add an input argument to an existing - # function/procedure via CREATE OR REPLACE: a changed argument - # list is a different signature, so PostgreSQL creates a new, - # separate overloaded routine instead of replacing this one. - # Reject such edits explicitly, rather than silently leaving an - # orphaned routine behind. + # PostgreSQL cannot add an argument to an existing function/ + # procedure via CREATE OR REPLACE. Adding an IN/INOUT/VARIADIC + # argument changes the routine's signature, so PostgreSQL + # creates a new, separate overloaded routine instead of + # replacing this one. Adding an OUT argument does not affect + # the signature, but it changes the shape of the returned row, + # which PostgreSQL rejects outright (SQLSTATE 42P13). Reject + # both cases explicitly, rather than silently leaving an + # orphaned routine behind or letting the database error surface. if 'arguments' in data and isinstance(data['arguments'], dict) \ and data['arguments'].get('added'): - return False, gettext( - "Adding a new argument to an existing function/procedure " - "is not supported, as PostgreSQL would create a separate, " - "overloaded routine rather than replacing this one. " - "Please create a new function/procedure instead." - ), '' + added_args = data['arguments']['added'] + if any( + (a.get('argmode') or 'IN') != 'OUT' for a in added_args + ): + return False, gettext( + "Adding a new IN/INOUT/VARIADIC argument to an " + "existing function/procedure is not supported, as " + "PostgreSQL would create a separate, overloaded " + "routine rather than replacing this one. Please " + "create a new function/procedure instead." + ), '' + else: + return False, gettext( + "Adding a new OUT argument to an existing function/" + "procedure is not supported, as it would change the " + "shape of the returned row. Please create a new " + "function/procedure instead." + ), '' # If Function Definition/Arguments are changed then merge old # Arguments with changed ones for Create/Replace Function diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_function_get_msql.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_function_get_msql.py index 8902052b23f..030ed380694 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_function_get_msql.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_function_get_msql.py @@ -143,7 +143,7 @@ class FunctionGetmsqlTestCase(BaseTestGenerator): ) ), ( - 'Fetch Function msql with newly added argument is rejected', + 'Fetch Function msql with newly added IN argument is rejected', dict( url='/browser/function/msql/', is_positive_test=True, @@ -161,7 +161,7 @@ class FunctionGetmsqlTestCase(BaseTestGenerator): "variables": [], "seclabels": [], "acl": [], - # PostgreSQL cannot add an argument to an existing + # PostgreSQL cannot add an IN argument to an existing # function via CREATE OR REPLACE (it would create a # separate, overloaded routine instead), so this must # be rejected with a clear error rather than silently @@ -178,7 +178,48 @@ class FunctionGetmsqlTestCase(BaseTestGenerator): mock_data={}, expected_data={ "status_code": 500, - "check_errormsg": "not supported" + "check_errormsg": "overloaded" + } + ), + ), + ( + 'Fetch Function msql with newly added OUT argument is ' + 'rejected', + dict( + url='/browser/function/msql/', + is_positive_test=True, + mocking_required=False, + with_function_id=True, + is_mock_local_function=False, + test_data={ + "name": "Test Function", + "funcowner": "", + "pronamespace": 2200, + "prorettypename": "character varying", + "lanname": "sql", + "prosrc": "select '1'", + "probin": "$libdir/", + "variables": [], + "seclabels": [], + "acl": [], + # Unlike an added IN/INOUT/VARIADIC argument, an added + # OUT argument does not change the function's + # identity/signature, but it does change the shape of + # the returned row, which PostgreSQL rejects outright + # (SQLSTATE 42P13). This must be rejected with a + # distinct, accurate error message. + "arguments": json.dumps({ + "added": [{ + "argname": "new_out_arg", + "argtype": "integer", + "argmode": "OUT" + }] + }) + }, + mock_data={}, + expected_data={ + "status_code": 500, + "check_errormsg": "returned row" } ), ), diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_procedure_put.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_procedure_put.py index 038cee4eb8c..e1c44c4c2fb 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_procedure_put.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions/tests/test_procedure_put.py @@ -21,10 +21,77 @@ class ProcedurePutTestCase(BaseTestGenerator): """ This class will update new procedure under schema node. """ scenarios = [ # Fetching default URL for procedure node. - ('Fetch Procedure Node URL', - dict(url='/browser/procedure/obj/')) + ('Fetch Procedure Node URL', dict( + url='/browser/procedure/obj/', + is_add_argument=False, + expected_data={ + "status_code": 200 + } + )), + ( + 'Fetch Procedure update with newly added IN argument is ' + 'rejected', + dict( + url='/browser/procedure/obj/', + # PostgreSQL cannot add an IN argument to an existing + # procedure via CREATE OR REPLACE (it would create a + # separate, overloaded routine instead), so this must be + # rejected with a clear error rather than silently + # producing SQL that orphans a routine. + is_add_argument=True, + test_data={ + "arguments": { + "added": [{ + "argname": "new_arg", + "argtype": "integer", + "argmode": "IN", + }] + } + }, + expected_data={ + "status_code": 500, + "check_errormsg": "overloaded" + } + ), + ), + ( + 'Fetch Procedure update with newly added OUT argument is ' + 'rejected', + dict( + url='/browser/procedure/obj/', + # Unlike an added IN/INOUT/VARIADIC argument, an added + # OUT argument does not change the procedure's + # identity/signature, but it does change the shape of the + # returned row, which PostgreSQL rejects outright + # (SQLSTATE 42P13). This must be rejected with a + # distinct, accurate error message. + is_add_argument=True, + test_data={ + "arguments": { + "added": [{ + "argname": "new_out_arg", + "argtype": "integer", + "argmode": "OUT", + }] + } + }, + expected_data={ + "status_code": 500, + "check_errormsg": "returned row" + } + ), + ), ] + def update_procedure(self, proc_id, data): + return self.tester.put( + self.url + str(utils.SERVER_GROUP) + + '/' + str(self.server_id) + '/' + str(self.db_id) + '/' + + str(self.schema_id) + '/' + + str(proc_id), + data=json.dumps(data), + follow_redirects=True) + def runTest(self): """ This function will update procedure under database node. """ super().setUp() @@ -47,14 +114,15 @@ def runTest(self): "dependsonextensions": ["plpgsql"] } - put_response = self.tester.put( - self.url + str(utils.SERVER_GROUP) + - '/' + str(self.server_id) + '/' + str(self.db_id) + '/' + - str(self.schema_id) + '/' + - str(proc_id), - data=json.dumps(data), - follow_redirects=True) - self.assertEqual(put_response.status_code, 200) + if getattr(self, 'is_add_argument', False): + data['arguments'] = self.test_data['arguments'] + + response = self.update_procedure(proc_id, data) + self.assertEqual(response.status_code, + self.expected_data['status_code']) + if 'check_errormsg' in self.expected_data: + self.assertIn(self.expected_data['check_errormsg'], + response.json['errormsg']) # Disconnect the database database_utils.disconnect_database(self, self.server_id, self.db_id)