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..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 @@ -1035,7 +1035,7 @@ def _update_arguments_for_get_sql(data, 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) @@ -1194,6 +1194,36 @@ 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 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'): + 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 # SQL statement 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..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 @@ -142,6 +142,87 @@ class FunctionGetmsqlTestCase(BaseTestGenerator): } ) ), + ( + 'Fetch Function msql with newly added IN 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": [], + # 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 + # producing SQL that orphans a routine. + "arguments": json.dumps({ + "added": [{ + "argname": "new_arg", + "argtype": "integer", + "argmode": "IN", + "argdefval": "1" + }] + }) + }, + mock_data={}, + expected_data={ + "status_code": 500, + "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" + } + ), + ), ( 'Fetch Function msql fetch properties not found', dict( @@ -222,5 +303,11 @@ 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']) + 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) 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)