From bdfecb955461fda83938995c3d1aebbd95e77291 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 19 Aug 2026 11:45:31 +0100 Subject: [PATCH] fix: stop Schema Diff injecting whitespace into applied function/procedure bodies (#10302) Generating a script for a function or procedure that only exists on the source ("Source Only") routed through FunctionView.sql(), which always calls reformat_prosrc_code() to wrap the body with a leading and trailing newline around $BODY$...$BODY$ purely for readability in the SQL panel. That reformatted body was then executed verbatim against the target, so PostgreSQL stored the extra newlines in prosrc: the target's body no longer matched the source's byte for byte, and re-comparing kept reporting a whitespace-only difference unless "Ignore whitespace" was ticked. Schema Diff's update path (CREATE OR REPLACE against an existing target object) already suppressed this formatting via allow_code_formatting, added for #7102; this extends the same guard to the create/"Source Only" path so Schema Diff never adds whitespace of its own to the SQL it actually applies. Added a dedicated regression test covering both the "Source Only" and "Different" cases for a function and a procedure: applying the generated script now leaves every object comparing as Identical without needing "Ignore whitespace". --- .../databases/schemas/functions/__init__.py | 16 +- .../tests/test_schema_diff_function_body.py | 236 ++++++++++++++++++ 2 files changed, 250 insertions(+), 2 deletions(-) create mode 100644 web/pgadmin/tools/schema_diff/tests/test_schema_diff_function_body.py 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..017ec29df40 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 @@ -913,6 +913,7 @@ def sql(self, gid, sid, did, scid, fnid=None, **kwargs): """ json_resp = kwargs.get('json_resp', True) target_schema = kwargs.get('target_schema', None) + allow_code_formatting = kwargs.get('allow_code_formatting', True) resp_data = self._fetch_properties(gid, sid, did, scid, fnid) # Most probably this is due to error @@ -936,7 +937,8 @@ def sql(self, gid, sid, did, scid, fnid=None, **kwargs): resp_data['func_args_without'] = ', '.join(args_without_name) - self.reformat_prosrc_code(resp_data) + if allow_code_formatting: + self.reformat_prosrc_code(resp_data) if self.node_type == 'procedure': object_type = 'procedure' @@ -1670,8 +1672,18 @@ def get_sql_from_diff(self, **kwargs): sql = self.delete(gid=gid, sid=sid, did=did, scid=scid, fnid=oid, only_sql=True) elif target_schema: + # This SQL is the one that will actually be executed + # against the target (e.g. to create a "Source Only" + # object there), so it must reproduce the body exactly + # as stored, without the cosmetic newlines added around + # $BODY$ for readability in the SQL panel. Otherwise the + # target's prosrc would differ from the source's by + # nothing but pgAdmin's own formatting, and a subsequent + # comparison would keep reporting a (whitespace-only) + # difference (see #10302). sql = self.sql(gid=gid, sid=sid, did=did, scid=scid, fnid=oid, - target_schema=target_schema, json_resp=False) + target_schema=target_schema, json_resp=False, + allow_code_formatting=False) else: sql = self.sql(gid=gid, sid=sid, did=did, scid=scid, fnid=oid, json_resp=False) diff --git a/web/pgadmin/tools/schema_diff/tests/test_schema_diff_function_body.py b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_function_body.py new file mode 100644 index 00000000000..b93895dafab --- /dev/null +++ b/web/pgadmin/tools/schema_diff/tests/test_schema_diff_function_body.py @@ -0,0 +1,236 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +"""Schema Diff tests for function/procedure body whitespace (#10302). + +Schema Diff's own reverse-engineered CREATE OR REPLACE wraps a function or +procedure body in a leading and trailing newline around $BODY$...$BODY$ so +that it reads nicely in the SQL panel. That cosmetic reformatting must not +leak into the SQL that Schema Diff actually applies to the target: doing so +changes the stored prosrc (PostgreSQL keeps the body exactly as written +between the dollar quotes), so the target would forever compare as +different from the source by nothing but pgAdmin's own added whitespace, +even though "Ignore whitespace" is unticked. + +This covers both ways a body ends up applied through Schema Diff: creating +an object that only exists in the source ("Source Only"), and replacing one +that exists in both but with a different body ("Different"). +""" + +import json +import secrets +import uuid + +from pgadmin.utils.route import BaseSocketTestGenerator +from regression import parent_node_dict +from regression.python_test_utils import test_utils as utils + +SCHEMA_NAME = 'test_func_body_diff' + +# proc_added/func_added exist only in the source (Source Only status). +# proc_changed/func_changed exist in both, with a different body +# (Different status), so applying the diff is a CREATE OR REPLACE. +SOURCE_DDL = """ +CREATE SCHEMA {0}; + +CREATE PROCEDURE {0}.proc_added(arg1 bigint) + LANGUAGE sql + AS $$select 1;$$; + +CREATE FUNCTION {0}.func_added(arg1 bigint) + RETURNS bigint + LANGUAGE sql + AS $$select arg1;$$; + +CREATE PROCEDURE {0}.proc_changed(arg1 bigint) + LANGUAGE sql + AS $$select 1;$$; + +CREATE FUNCTION {0}.func_changed(arg1 bigint) + RETURNS bigint + LANGUAGE sql + AS $$select arg1;$$; +""" + +TARGET_DDL = """ +CREATE SCHEMA {0}; + +CREATE PROCEDURE {0}.proc_changed(arg1 bigint) + LANGUAGE sql + AS $$select 2;$$; + +CREATE FUNCTION {0}.func_changed(arg1 bigint) + RETURNS bigint + LANGUAGE sql + AS $$select arg1 + 1;$$; +""" + + +class SchemaDiffFunctionBodyTestCase(BaseSocketTestGenerator): + """ This class will test Schema Diff for function/procedure bodies. """ + scenarios = [ + ('Schema diff comparison of function/procedure bodies', dict()) + ] + SOCKET_NAMESPACE = '/schema_diff' + + def setUp(self): + super().setUp() + self.src_database = "db_func_body_diff_src_%s" % str( + uuid.uuid4())[1:8] + self.tar_database = "db_func_body_diff_tar_%s" % str( + uuid.uuid4())[1:8] + + self.src_db_id = utils.create_database(self.server, self.src_database) + self.tar_db_id = utils.create_database(self.server, self.tar_database) + + self.server = parent_node_dict["server"][-1]["server"] + self.server_id = parent_node_dict["server"][-1]["server_id"] + + self.execute_sql(self.src_database, SOURCE_DDL.format(SCHEMA_NAME)) + self.execute_sql(self.tar_database, TARGET_DDL.format(SCHEMA_NAME)) + + def execute_sql(self, db_name, sql): + """ + Run a statement batch against one of the test databases. + + :param db_name: Database to run against + :param sql: SQL to execute + """ + connection = utils.get_db_connection(db_name, + self.server['username'], + self.server['db_password'], + self.server['host'], + self.server['port'], + self.server['sslmode']) + old_isolation_level = connection.isolation_level + utils.set_isolation_level(connection, 0) + pg_cursor = connection.cursor() + pg_cursor.execute(sql) + utils.set_isolation_level(connection, old_isolation_level) + connection.commit() + connection.close() + + def compare(self): + """ + Compare the two test databases and return the result. + + :return: List of compared objects + """ + data = { + 'trans_id': self.trans_id, + 'source_sid': self.server_id, + 'source_did': self.src_db_id, + 'target_sid': self.server_id, + 'target_did': self.tar_db_id, + 'ignore_owner': 0, + 'ignore_whitespaces': 0, + 'ignore_tablespace': 0, + 'ignore_grants': 0 + } + self.socket_client.emit('compare_database', data, + namespace=self.SOCKET_NAMESPACE) + received = self.socket_client.get_received(self.SOCKET_NAMESPACE) + response_data = received[-1]['args'][0] + self.assertEqual(received[-1]['name'], "compare_database_success", + response_data) + return response_data + + def find_object(self, response_data, node_type, title_prefix): + """ + Pick a single compared object out of the comparison result. + + :param response_data: Result of compare() + :param node_type: Node type, e.g. 'procedure' + :param title_prefix: Object name (without argument list) + :return: The compared object + """ + for diff in response_data: + if diff.get('type') == node_type and \ + diff.get('title', '').startswith(title_prefix + '('): + return diff + + self.fail('{0} {1} was not compared'.format(node_type, title_prefix)) + + def assert_no_injected_whitespace(self, diff_ddl): + """ + The applied SQL must reproduce the body exactly as stored: no + leading/trailing newline injected around $BODY$ purely for + readability in the SQL panel (#10302). + """ + self.assertNotIn('$BODY$\n', diff_ddl, + 'Schema Diff injected a leading newline into the ' + 'function/procedure body: {0}'.format(diff_ddl)) + self.assertNotIn('\n$BODY$;', diff_ddl, + 'Schema Diff injected a trailing newline into the ' + 'function/procedure body: {0}'.format(diff_ddl)) + + def runTest(self): + """ Test Schema Diff for function/procedure body whitespace. """ + self.trans_id = str(secrets.choice(range(1, 99999))) + response = self.tester.get( + 'schema_diff/initialize/{}'.format(self.trans_id)) + self.assertEqual(response.status_code, 200) + + received = self.socket_client.get_received(self.SOCKET_NAMESPACE) + self.assertEqual(received[0]['name'], 'connected') + + self.tester.post( + 'schema_diff/server/connect/{}'.format(self.server_id), + data=json.dumps({'password': self.server['db_password']}), + content_type='html/json') + self.tester.post('schema_diff/database/connect/{0}/{1}'.format( + self.server_id, self.src_db_id)) + self.tester.post('schema_diff/database/connect/{0}/{1}'.format( + self.server_id, self.tar_db_id)) + + response_data = self.compare() + + cases = [ + ('procedure', 'proc_added', 'Source Only'), + ('function', 'func_added', 'Source Only'), + ('procedure', 'proc_changed', 'Different'), + ('function', 'func_changed', 'Different'), + ] + + diff_ddls = {} + for node_type, name, expected_status in cases: + diff = self.find_object(response_data, node_type, name) + self.assertEqual(diff['status'], expected_status, + '{0} {1} was reported as {2}, expected {3}' + .format(node_type, name, diff['status'], + expected_status)) + self.assert_no_injected_whitespace(diff['diff_ddl']) + diff_ddls[name] = diff['diff_ddl'] + + # Apply every generated script to the target, then re-compare: all + # four objects must now settle as identical, without needing + # "Ignore whitespace" ticked. + for name in diff_ddls: + self.execute_sql(self.tar_database, diff_ddls[name]) + + response_data = self.compare() + for node_type, name, _ in cases: + diff = self.find_object(response_data, node_type, name) + self.assertEqual(diff['status'], 'Identical', + '{0} {1} still compares as {2} after applying ' + 'the generated script: {3}' + .format(node_type, name, diff['status'], + diff.get('diff_ddl'))) + + def tearDown(self): + """This function drops the added databases""" + super().tearDown() + for db_name in (self.src_database, self.tar_database): + connection = utils.get_db_connection(self.server['db'], + self.server['username'], + self.server['db_password'], + self.server['host'], + self.server['port'], + self.server['sslmode']) + utils.drop_database(connection, db_name)