From 6bb0c709300783c409a65af2fc12d79a5c3db574 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 15 Oct 2026 02:33:01 +0100 Subject: [PATCH 1/4] Fix formatting a view's SQL being treated as a column change that drops and re-creates the view. #9838 The check for whether an edited view definition changes the view's columns compared the text before the first FROM case-sensitively, so a pure reformat that changed the case of function names, data types or keywords (as the SQL formatter does) triggered the drop-and-recreate warning, and the generated SQL dropped the view instead of using CREATE OR REPLACE VIEW. Both the frontend warning and the backend now lower-case the definition outside quoted identifiers and string literals before comparing. Closes #9838 --- .../databases/schemas/views/__init__.py | 23 ++++++-- .../schemas/views/static/js/view.ui.js | 26 ++++++--- .../views/tests/test_views_get_msql.py | 13 ++++- .../schemas/views/tests/view_test_data.json | 40 ++++++++++++++ .../schema_ui_files/view.ui.spec.js | 53 ++++++++++++++++++- 5 files changed, 143 insertions(+), 12 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py index 5b73863bf9a..e7ac0bfe6d5 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py @@ -912,6 +912,22 @@ def _get_create_view_sql(self, data): return False, '', sql + @staticmethod + def _normalise_definition(definition): + """ + Normalise a view definition so that only changes that can affect the + view's columns are noticed. Everything outside quoted identifiers and + string literals is lower-cased (keywords, function names, type names + and unquoted identifiers are case-insensitive), so a pure reformat, + such as one from the SQL formatter, is not mistaken for a change to + the column list. Non-word characters are then removed. + """ + parts = re.split(r'("(?:[^"]|"")*"|\'(?:[^\']|\'\')*\')', + definition or '') + return re.sub(r"\W", "", ''.join( + part if i % 2 else part.lower() for i, part in enumerate(parts) + )) + def _get_definition_data(self, vid, data, old_data, res, acls): """ Check and process definition data. @@ -925,9 +941,10 @@ def _get_definition_data(self, vid, data, old_data, res, acls): if not retun False with error msg empty('') """ if 'definition' in data and self.manager.server_type == 'pg': - new_def = re.sub(r"\W", "", data['definition']).split('FROM') - old_def = re.sub(r"\W", "", res['rows'][0]['definition'] - ).split('FROM') + new_def = ViewNode._normalise_definition( + data['definition']).split('from') + old_def = ViewNode._normalise_definition( + res['rows'][0]['definition']).split('from') if 'definition' in data and ( len(old_def) > 1 or len(new_def) > 1 ) and ( diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/view.ui.js b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/view.ui.js index b65e34cb83f..421fb66a26f 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/view.ui.js +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/view.ui.js @@ -14,6 +14,22 @@ import SecLabelSchema from '../../../../../static/js/sec_label.ui'; import { isEmptyString } from 'sources/validators'; import { getPrivilegesForTableAndLikeObjects } from '../../../tables/static/js/table.ui'; +/* + * Normalise a view definition so that only changes that can affect the + * view's columns are noticed. Whitespace is removed, and everything outside + * quoted identifiers and string literals is lower-cased, because SQL keywords, + * function names, type names and unquoted identifiers are case-insensitive; + * this stops a pure reformat (for example with the SQL formatter, which can + * change keyword, function and data type case) from looking like a change to + * the column list. + */ +export function normaliseViewDefinition(definition) { + return (definition ?? '').split( + /("(?:[^"]|"")*"|'(?:[^']|'')*')/ + ).map( + (part, i) => (i % 2 == 0 ? part.toLowerCase() : part) + ).join('').replace(/\s/g, ''); +} export default class ViewSchema extends BaseUISchema { constructor(getPrivilegeRoleSchema, nodeInfo, fieldOptions={}, initValues={}) { @@ -147,15 +163,13 @@ export default class ViewSchema extends BaseUISchema { return false; } - let old_def = obj.origData.definition?.replace( - /\s/gi, '' - ).split('FROM'), + let old_def = normaliseViewDefinition( + obj.origData.definition + ).split('from'), new_def = []; if (!_.isUndefined(state.definition)) { - new_def = state.definition.replace( - /\s/gi, '' - ).split('FROM'); + new_def = normaliseViewDefinition(state.definition).split('from'); } if ((old_def.length != new_def.length) || ( diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/test_views_get_msql.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/test_views_get_msql.py index 3a4af7673bb..7ca2cf83544 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/test_views_get_msql.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/test_views_get_msql.py @@ -68,13 +68,22 @@ def runTest(self): schema node.""" if self.is_positive_test: - url_encode_data = {"oid": self.view_id, - "comment": self.data['comment']} + url_encode_data = {"oid": self.view_id} + for key in ('comment', 'definition'): + if key in self.data: + url_encode_data[key] = self.data[key] response = views_utils.api_get_msql(self, url_encode_data) # Assert response utils.assert_status_code(self, response) + + result = self.expected_data.get('test_result_data', {}) + sql = json.loads(response.data.decode('utf-8'))['data'] + if 'sql_contains' in result: + self.assertIn(result['sql_contains'], sql) + if 'sql_not_contains' in result: + self.assertNotIn(result['sql_not_contains'], sql) else: if 'view_id' in self.data: # Non-existing view id diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json index c2a00d15c38..01c768a8e5e 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json @@ -1530,6 +1530,46 @@ "test_result_data": {} } }, + { + "name": "Get view msql: Reformatted definition does not drop the view.", + "url": "/browser/view/msql/", + "is_positive_test": true, + "inventory_data": { + "query": "\"CREATE OR REPLACE VIEW %s.%s AS SELECT id, concat(a, ' x ', b) AS ab, (d)::text AS ds FROM (VALUES (1, 'a', 'b', now()::date)) t(id, a, b, d); ALTER TABLE %s.%s OWNER TO %s\" % (schema_name, view_name, schema_name, view_name, server['username'])" + }, + "test_data": { + "definition": "SELECT\n\tid,\n\tCONCAT(a, ' x ', b) AS ab,\n\t(d)::TEXT AS ds\nFROM\n\t(VALUES (1, 'a', 'b', NOW()::DATE)) t (id, a, b, d);" + }, + "mocking_required": false, + "mock_data": {}, + "expected_data": { + "status_code": 200, + "error_msg": null, + "test_result_data": { + "sql_not_contains": "DROP VIEW" + } + } + }, + { + "name": "Get view msql: Changing the columns drops the view.", + "url": "/browser/view/msql/", + "is_positive_test": true, + "inventory_data": { + "query": "\"CREATE OR REPLACE VIEW %s.%s AS SELECT id, concat(a, ' x ', b) AS ab, (d)::text AS ds FROM (VALUES (1, 'a', 'b', now()::date)) t(id, a, b, d); ALTER TABLE %s.%s OWNER TO %s\" % (schema_name, view_name, schema_name, view_name, server['username'])" + }, + "test_data": { + "definition": "SELECT id, concat(a, ' x ', b) AS ab FROM (VALUES (1, 'a', 'b', now()::date)) t(id, a, b, d)" + }, + "mocking_required": false, + "mock_data": {}, + "expected_data": { + "status_code": 200, + "error_msg": null, + "test_result_data": { + "sql_contains": "DROP VIEW" + } + } + }, { "name": "Get view msql: With Non-existing view.", "url": "/browser/view/msql/", diff --git a/web/regression/javascript/schema_ui_files/view.ui.spec.js b/web/regression/javascript/schema_ui_files/view.ui.spec.js index 832ce0a24fb..7838352b748 100644 --- a/web/regression/javascript/schema_ui_files/view.ui.spec.js +++ b/web/regression/javascript/schema_ui_files/view.ui.spec.js @@ -9,7 +9,7 @@ import BaseUISchema from 'sources/SchemaView/base_schema.ui'; -import ViewSchema from '../../../pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/view.ui'; +import ViewSchema, { normaliseViewDefinition } from '../../../pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/view.ui'; import {genericBeforeEach, getCreateView, getEditView, getPropertiesView} from '../genericFunctions'; import { initializeSchemaWithData } from './utils'; @@ -75,4 +75,55 @@ describe('ViewSchema', ()=>{ expect(setError).toHaveBeenCalledWith('definition', null); }); + + describe('column change warning', ()=>{ + // As returned by pg_get_viewdef() + let origDef = ` SELECT id, + "Name", + concat(a, ' x ', b) AS ab, + (d)::text AS ds + FROM t1 t + WHERE (id > 1);`; + + let validateWith = (definition)=>{ + initializeSchemaWithData(schemaObj, {oid: 1, definition: origDef}); + schemaObj.validate({definition: definition}, jest.fn()); + return schemaObj.warningText; + }; + + it('does not warn when the definition has only been reformatted', ()=>{ + // As produced by the SQL formatter with upper case keywords, + // functions and data types + expect(validateWith(`SELECT + id, + "Name", + CONCAT(a, ' x ', b) AS ab, + (d)::TEXT AS ds +FROM + t1 t +WHERE + (id > 1);`)).toBeNull(); + + // Lower case keywords + expect(validateWith(`select id, "Name", concat(a, ' x ', b) as ab, + (d)::text as ds from t1 t where (id > 1);`)).toBeNull(); + }); + + it('warns when the columns change', ()=>{ + expect(validateWith(`SELECT id, "Name", concat(a, ' x ', b) AS ab +FROM t1 t WHERE (id > 1);`)).not.toBeNull(); + }); + + it('warns when the case of a quoted identifier changes', ()=>{ + expect(validateWith(`SELECT id, "NAME", concat(a, ' x ', b) AS ab, + (d)::text AS ds FROM t1 t WHERE (id > 1);`)).not.toBeNull(); + }); + + it('preserves quoted identifiers and literals when normalising', ()=>{ + expect(normaliseViewDefinition( + 'SELECT "MixedCase", \'It\'\'s A Value\' AS X FROM T' + )).toBe('select"MixedCase",\'It\'\'sAValue\'asxfromt'); + expect(normaliseViewDefinition(undefined)).toBe(''); + }); + }); }); From 910d104289e3aaacc495817fc54ba220a42942f9 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 15 Oct 2026 10:25:35 +0100 Subject: [PATCH 2/4] Keep quoted identifiers intact and split at the FROM keyword when checking view column changes. #9838 Address review: punctuation was still being stripped from quoted identifiers, so renaming a column from "a-b" to "ab" went unnoticed, and the select list was cut at any occurrence of 'from', so a column removed after one named like valid_from was missed and CREATE OR REPLACE VIEW failed with "cannot drop columns from view". The contents of quoted identifiers and string literals are now left exactly as written (only the double quotes around an identifier are dropped, so "name" still matches name), and the definition is split only at FROM as a whole word outside quotes. --- .../databases/schemas/views/__init__.py | 43 ++++++++------ .../schemas/views/static/js/view.ui.js | 45 +++++++++------ .../tests/test_views_split_definition.py | 57 +++++++++++++++++++ .../schemas/views/tests/view_test_data.json | 20 +++++++ .../schema_ui_files/view.ui.spec.js | 44 ++++++++++++-- 5 files changed, 171 insertions(+), 38 deletions(-) create mode 100644 web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/test_views_split_definition.py diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py index e7ac0bfe6d5..0b1ea20f104 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py @@ -913,20 +913,32 @@ def _get_create_view_sql(self, data): return False, '', sql @staticmethod - def _normalise_definition(definition): - """ - Normalise a view definition so that only changes that can affect the - view's columns are noticed. Everything outside quoted identifiers and - string literals is lower-cased (keywords, function names, type names - and unquoted identifiers are case-insensitive), so a pure reformat, - such as one from the SQL formatter, is not mistaken for a change to - the column list. Non-word characters are then removed. - """ + def _split_definition(definition): + """ + Split a view definition at each FROM keyword and normalise the parts + so that only changes that can affect the view's columns are noticed. + The contents of quoted identifiers and string literals are kept + exactly as written, although the double quotes around an identifier + are dropped so that "name" still matches name. Everything else is + lower-cased (keywords, function names, type names and unquoted + identifiers are case-insensitive) and has its non-word characters + removed, so a pure reformat, such as one from the SQL formatter, is + not mistaken for a change to the column list. FROM is only matched as + a whole word outside quotes, so identifiers such as valid_from do not + end the select list early. + """ + segments = [''] parts = re.split(r'("(?:[^"]|"")*"|\'(?:[^\']|\'\')*\')', definition or '') - return re.sub(r"\W", "", ''.join( - part if i % 2 else part.lower() for i, part in enumerate(parts) - )) + for i, part in enumerate(parts): + if i % 2: + segments[-1] += part[1:-1] if part[0] == '"' else part + continue + pieces = [re.sub(r"\W", "", piece) + for piece in re.split(r'\bfrom\b', part.lower())] + segments[-1] += pieces[0] + segments.extend(pieces[1:]) + return segments def _get_definition_data(self, vid, data, old_data, res, acls): """ @@ -941,10 +953,9 @@ def _get_definition_data(self, vid, data, old_data, res, acls): if not retun False with error msg empty('') """ if 'definition' in data and self.manager.server_type == 'pg': - new_def = ViewNode._normalise_definition( - data['definition']).split('from') - old_def = ViewNode._normalise_definition( - res['rows'][0]['definition']).split('from') + new_def = ViewNode._split_definition(data['definition']) + old_def = ViewNode._split_definition( + res['rows'][0]['definition']) if 'definition' in data and ( len(old_def) > 1 or len(new_def) > 1 ) and ( diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/view.ui.js b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/view.ui.js index 421fb66a26f..b82065ce818 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/view.ui.js +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/view.ui.js @@ -15,20 +15,35 @@ import { isEmptyString } from 'sources/validators'; import { getPrivilegesForTableAndLikeObjects } from '../../../tables/static/js/table.ui'; /* - * Normalise a view definition so that only changes that can affect the - * view's columns are noticed. Whitespace is removed, and everything outside - * quoted identifiers and string literals is lower-cased, because SQL keywords, - * function names, type names and unquoted identifiers are case-insensitive; - * this stops a pure reformat (for example with the SQL formatter, which can - * change keyword, function and data type case) from looking like a change to - * the column list. + * Split a view definition at each FROM keyword and normalise the parts so + * that only changes that can affect the view's columns are noticed. The + * contents of quoted identifiers and string literals are kept exactly as + * written, although the double quotes around an identifier are dropped so + * that "name" still matches name. Everything else is lower-cased (SQL + * keywords, function names, type names and unquoted identifiers are + * case-insensitive) and has its whitespace removed, so a pure reformat (for + * example with the SQL formatter, which can change keyword, function and data + * type case) does not look like a change to the column list. FROM is only + * matched as a whole word outside quotes, so identifiers such as valid_from + * do not end the select list early. */ -export function normaliseViewDefinition(definition) { - return (definition ?? '').split( +export function splitViewDefinition(definition) { + let segments = ['']; + (definition ?? '').split( /("(?:[^"]|"")*"|'(?:[^']|'')*')/ - ).map( - (part, i) => (i % 2 == 0 ? part.toLowerCase() : part) - ).join('').replace(/\s/g, ''); + ).forEach((part, i) => { + if (i % 2 == 1) { + segments[segments.length - 1] += part.startsWith('"') ? + part.slice(1, -1) : part; + return; + } + let pieces = part.toLowerCase().split(/\bfrom\b/).map( + (piece) => piece.replace(/\s/g, '') + ); + segments[segments.length - 1] += pieces[0]; + segments.push(...pieces.slice(1)); + }); + return segments; } export default class ViewSchema extends BaseUISchema { @@ -163,13 +178,11 @@ export default class ViewSchema extends BaseUISchema { return false; } - let old_def = normaliseViewDefinition( - obj.origData.definition - ).split('from'), + let old_def = splitViewDefinition(obj.origData.definition), new_def = []; if (!_.isUndefined(state.definition)) { - new_def = normaliseViewDefinition(state.definition).split('from'); + new_def = splitViewDefinition(state.definition); } if ((old_def.length != new_def.length) || ( diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/test_views_split_definition.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/test_views_split_definition.py new file mode 100644 index 00000000000..7e629314b63 --- /dev/null +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/test_views_split_definition.py @@ -0,0 +1,57 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +from pgadmin.utils.route import BaseTestGenerator +from pgadmin.browser.server_groups.servers.databases.schemas.views import \ + ViewNode + + +class ViewsSplitDefinitionTestCase(BaseTestGenerator): + """The select list compared to decide whether a view must be dropped + and re-created ignores reformatting, but notices changes to quoted + identifiers and is not cut short by identifiers containing 'from'.""" + + scenarios = [ + ('Reformat with different case and whitespace is ignored', dict( + old=' SELECT id,\n upper(a) AS ua,\n (d)::text AS ds\n' + ' FROM t1 t;', + new='SELECT\n\tid,\n\tUPPER(a) AS ua,\n\t(d)::TEXT AS ds\n' + 'FROM\n\tt1 t;', + same=True)), + ('Case change in a quoted identifier is noticed', dict( + old='SELECT 1 AS "Name" FROM t1', + new='SELECT 1 AS "NAME" FROM t1', + same=False)), + ('Punctuation change in a quoted identifier is noticed', dict( + old='SELECT 1 AS "a-b" FROM t1', + new='SELECT 1 AS "ab" FROM t1', + same=False)), + ('Quoting an identifier that needs no quotes is ignored', dict( + old='SELECT id, name FROM t1', + new='SELECT "id", "name" FROM t1', + same=True)), + ('Column removed after one named like FROM is noticed', dict( + old='SELECT a AS fromage, b FROM t1', + new='SELECT a AS fromage FROM t1', + same=False)), + ('Column removed after valid_from is noticed', dict( + old='SELECT id, valid_from, name FROM t1', + new='SELECT id, valid_from FROM t1', + same=False)), + ('FROM inside a string literal does not split', dict( + old="SELECT 'x FROM y' AS a, b FROM t1", + new="SELECT 'x FROM y' AS a FROM t1", + same=False)), + ] + + def runTest(self): + old_def = ViewNode._split_definition(self.old) + new_def = ViewNode._split_definition(self.new) + self.assertEqual(len(old_def), 2) + self.assertEqual(old_def[0] == new_def[0], self.same) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json index 01c768a8e5e..9645c5580c5 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json @@ -1570,6 +1570,26 @@ } } }, + { + "name": "Get view msql: Removing a column after valid_from drops the view.", + "url": "/browser/view/msql/", + "is_positive_test": true, + "inventory_data": { + "query": "\"CREATE OR REPLACE VIEW %s.%s AS SELECT id, valid_from, name FROM (VALUES (1, now()::date, 'a')) t(id, valid_from, name); ALTER TABLE %s.%s OWNER TO %s\" % (schema_name, view_name, schema_name, view_name, server['username'])" + }, + "test_data": { + "definition": "SELECT id, valid_from FROM (VALUES (1, now()::date, 'a')) t(id, valid_from, name)" + }, + "mocking_required": false, + "mock_data": {}, + "expected_data": { + "status_code": 200, + "error_msg": null, + "test_result_data": { + "sql_contains": "DROP VIEW" + } + } + }, { "name": "Get view msql: With Non-existing view.", "url": "/browser/view/msql/", diff --git a/web/regression/javascript/schema_ui_files/view.ui.spec.js b/web/regression/javascript/schema_ui_files/view.ui.spec.js index 7838352b748..abd0745fd79 100644 --- a/web/regression/javascript/schema_ui_files/view.ui.spec.js +++ b/web/regression/javascript/schema_ui_files/view.ui.spec.js @@ -9,7 +9,7 @@ import BaseUISchema from 'sources/SchemaView/base_schema.ui'; -import ViewSchema, { normaliseViewDefinition } from '../../../pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/view.ui'; +import ViewSchema, { splitViewDefinition } from '../../../pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/view.ui'; import {genericBeforeEach, getCreateView, getEditView, getPropertiesView} from '../genericFunctions'; import { initializeSchemaWithData } from './utils'; @@ -119,11 +119,43 @@ FROM t1 t WHERE (id > 1);`)).not.toBeNull(); (d)::text AS ds FROM t1 t WHERE (id > 1);`)).not.toBeNull(); }); - it('preserves quoted identifiers and literals when normalising', ()=>{ - expect(normaliseViewDefinition( - 'SELECT "MixedCase", \'It\'\'s A Value\' AS X FROM T' - )).toBe('select"MixedCase",\'It\'\'sAValue\'asxfromt'); - expect(normaliseViewDefinition(undefined)).toBe(''); + it('warns when punctuation inside a quoted identifier changes', ()=>{ + expect(validateWith(`SELECT id, "Na me", concat(a, ' x ', b) AS ab, + (d)::text AS ds FROM t1 t WHERE (id > 1);`)).not.toBeNull(); + }); + + it('warns when a column is removed after one named like FROM', ()=>{ + initializeSchemaWithData(schemaObj, { + oid: 1, definition: 'SELECT a AS fromage, b FROM t1;', + }); + schemaObj.validate({definition: 'SELECT a AS fromage FROM t1;'}, + jest.fn()); + expect(schemaObj.warningText).not.toBeNull(); + }); + + it('warns when a column is removed after valid_from', ()=>{ + initializeSchemaWithData(schemaObj, { + oid: 1, definition: 'SELECT id, valid_from, name FROM t1;', + }); + schemaObj.validate({definition: 'SELECT id, valid_from FROM t1;'}, + jest.fn()); + expect(schemaObj.warningText).not.toBeNull(); + }); + + it('does not warn when an identifier is merely quoted', ()=>{ + expect(validateWith(`SELECT "id", "Name", concat(a, ' x ', b) AS "ab", + (d)::text AS ds FROM t1 t WHERE (id > 1);`)).toBeNull(); + }); + + it('splits and normalises definitions', ()=>{ + expect(splitViewDefinition( + 'SELECT "Mixed Case", \'It\'\'s A FROM Value\' AS X, fromage FROM T' + )).toEqual([ + 'selectMixed Case,\'It\'\'s A FROM Value\'asx,fromage', 't' + ]); + expect(splitViewDefinition('SELECT "a-b" FROM t')).not.toEqual( + splitViewDefinition('SELECT "ab" FROM t')); + expect(splitViewDefinition(undefined)).toEqual(['']); }); }); }); From d98bd48a2bd340c7d0e1a2d556116398fb8603aa Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 15 Oct 2026 12:14:13 +0100 Subject: [PATCH 3/4] Make the view reformat test independent of the PostgreSQL version. #9838 Before PostgreSQL 16, pg_get_viewdef() qualifies column references with the range alias even when the FROM clause has a single relation, so on older servers the test's unqualified definition differed from the stored one in its select list and correctly caused a drop and re-create. Base the reformat and column-change scenarios on a two-relation view, which deparses identically on all supported versions. --- .../databases/schemas/views/tests/view_test_data.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json index 9645c5580c5..69100065e5d 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/view_test_data.json @@ -1535,10 +1535,10 @@ "url": "/browser/view/msql/", "is_positive_test": true, "inventory_data": { - "query": "\"CREATE OR REPLACE VIEW %s.%s AS SELECT id, concat(a, ' x ', b) AS ab, (d)::text AS ds FROM (VALUES (1, 'a', 'b', now()::date)) t(id, a, b, d); ALTER TABLE %s.%s OWNER TO %s\" % (schema_name, view_name, schema_name, view_name, server['username'])" + "query": "\"CREATE OR REPLACE VIEW %s.%s AS SELECT t.id, concat(t.a, ' x ', t.b) AS ab, (t.d)::text AS ds, u.n FROM (VALUES (1, 'a', 'b', now()::date)) t(id, a, b, d) CROSS JOIN (VALUES (2)) u(n); ALTER TABLE %s.%s OWNER TO %s\" % (schema_name, view_name, schema_name, view_name, server['username'])" }, "test_data": { - "definition": "SELECT\n\tid,\n\tCONCAT(a, ' x ', b) AS ab,\n\t(d)::TEXT AS ds\nFROM\n\t(VALUES (1, 'a', 'b', NOW()::DATE)) t (id, a, b, d);" + "definition": "SELECT\n\tt.id,\n\tCONCAT(t.a, ' x ', t.b) AS ab,\n\t(t.d)::TEXT AS ds,\n\tu.n\nFROM\n\t(VALUES (1, 'a', 'b', NOW()::DATE)) t (id, a, b, d)\n\tCROSS JOIN (VALUES (2)) u (n);" }, "mocking_required": false, "mock_data": {}, @@ -1555,10 +1555,10 @@ "url": "/browser/view/msql/", "is_positive_test": true, "inventory_data": { - "query": "\"CREATE OR REPLACE VIEW %s.%s AS SELECT id, concat(a, ' x ', b) AS ab, (d)::text AS ds FROM (VALUES (1, 'a', 'b', now()::date)) t(id, a, b, d); ALTER TABLE %s.%s OWNER TO %s\" % (schema_name, view_name, schema_name, view_name, server['username'])" + "query": "\"CREATE OR REPLACE VIEW %s.%s AS SELECT t.id, concat(t.a, ' x ', t.b) AS ab, (t.d)::text AS ds, u.n FROM (VALUES (1, 'a', 'b', now()::date)) t(id, a, b, d) CROSS JOIN (VALUES (2)) u(n); ALTER TABLE %s.%s OWNER TO %s\" % (schema_name, view_name, schema_name, view_name, server['username'])" }, "test_data": { - "definition": "SELECT id, concat(a, ' x ', b) AS ab FROM (VALUES (1, 'a', 'b', now()::date)) t(id, a, b, d)" + "definition": "SELECT t.id, concat(t.a, ' x ', t.b) AS ab, u.n FROM (VALUES (1, 'a', 'b', now()::date)) t(id, a, b, d) CROSS JOIN (VALUES (2)) u(n)" }, "mocking_required": false, "mock_data": {}, From 5d687fe2921fbb5e4fe527c06e5d97e4aa051004 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Thu, 15 Oct 2026 19:53:54 +0100 Subject: [PATCH 4/4] Skip SQL comments and dollar-quoted strings when checking view column changes. #9838 The view definition splitters on the server and in the editor treated a FROM inside a comment as the end of the select list, so removing a column after a comment such as /* from */ went unnoticed: the view was not dropped and re-created, and CREATE OR REPLACE VIEW then failed. Replace the quote-matching regex with a small scanner on both sides that follows PostgreSQL's lexer: comments (including nested block comments) are skipped, dollar-quoted strings and E'' strings with backslash escapes are kept intact, and a $ that continues an identifier does not start a dollar quote. --- .../databases/schemas/views/__init__.py | 81 +++++++++++++++- .../schemas/views/static/js/view.ui.js | 92 ++++++++++++++++++- .../tests/test_views_split_definition.py | 28 ++++++ .../schema_ui_files/view.ui.spec.js | 27 ++++++ 4 files changed, 219 insertions(+), 9 deletions(-) diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py index 0b1ea20f104..0828d8aa9af 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/__init__.py @@ -912,6 +912,79 @@ def _get_create_view_sql(self, data): return False, '', sql + # A character that may appear in an unquoted identifier, and so cannot + # immediately precede the start of a dollar-quoted string or an E'' + # string's prefix. + _IDENT_CHAR = re.compile(r'[A-Za-z0-9_$\u0080-\uffff]') + _DOLLAR_TAG = re.compile( + r'\$(?:[A-Za-z_\u0080-\uffff][A-Za-z0-9_\u0080-\uffff]*)?\$') + + @staticmethod + def _tokenise_definition(definition): + """ + Split a view definition into plain SQL and quoted parts, in the same + shape as re.split() with one capturing group: even indexes hold plain + SQL and odd indexes hold a quoted identifier, string literal or + dollar-quoted string exactly as written. This follows PostgreSQL's + lexer: '' and "" are escaped quotes, E'' strings also take backslash + escapes, block comments nest, and a $ that continues an identifier + does not start a dollar-quoted string. Comments are replaced with a + space in the plain SQL. A construct left unterminated runs to the + end of the definition. + """ + def follows_ident(pos): + return pos > 0 and \ + ViewNode._IDENT_CHAR.match(definition[pos - 1]) is not None + + parts = [''] + i, n = 0, len(definition) + while i < n: + ch = definition[i] + pair = definition[i:i + 2] + if pair == '--': + end = definition.find('\n', i) + i = n if end < 0 else end + parts[-1] += ' ' + elif pair == '/*': + depth, i = 1, i + 2 + while i < n and depth: + pair = definition[i:i + 2] + if pair == '/*': + depth, i = depth + 1, i + 2 + elif pair == '*/': + depth, i = depth - 1, i + 2 + else: + i += 1 + parts[-1] += ' ' + elif ch in '"\'': + escapes = ch == "'" and i > 0 and \ + definition[i - 1] in 'eE' and not follows_ident(i - 1) + j = i + 1 + while j < n: + if escapes and definition[j] == '\\': + j += 2 + elif definition[j] != ch: + j += 1 + elif definition[j + 1:j + 2] == ch: + j += 2 + else: + break + j = min(j + 1, n) + parts.extend([definition[i:j], '']) + i = j + else: + tag = None if ch != '$' or follows_ident(i) else \ + ViewNode._DOLLAR_TAG.match(definition, i) + if tag is None: + parts[-1] += ch + i += 1 + continue + end = definition.find(tag.group(0), tag.end()) + j = n if end < 0 else end + len(tag.group(0)) + parts.extend([definition[i:j], '']) + i = j + return parts + @staticmethod def _split_definition(definition): """ @@ -924,12 +997,12 @@ def _split_definition(definition): identifiers are case-insensitive) and has its non-word characters removed, so a pure reformat, such as one from the SQL formatter, is not mistaken for a change to the column list. FROM is only matched as - a whole word outside quotes, so identifiers such as valid_from do not - end the select list early. + a whole word outside quotes and comments, so identifiers such as + valid_from, and comments such as /* from */, do not end the select + list early. """ segments = [''] - parts = re.split(r'("(?:[^"]|"")*"|\'(?:[^\']|\'\')*\')', - definition or '') + parts = ViewNode._tokenise_definition(definition or '') for i, part in enumerate(parts): if i % 2: segments[-1] += part[1:-1] if part[0] == '"' else part diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/view.ui.js b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/view.ui.js index b82065ce818..caf673ce450 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/view.ui.js +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/static/js/view.ui.js @@ -14,6 +14,89 @@ import SecLabelSchema from '../../../../../static/js/sec_label.ui'; import { isEmptyString } from 'sources/validators'; import { getPrivilegesForTableAndLikeObjects } from '../../../tables/static/js/table.ui'; +// A character that may appear in an unquoted identifier, and so cannot +// immediately precede the start of a dollar-quoted string or an E'' string's +// prefix. +const IDENT_CHAR = /[A-Za-z0-9_$\u0080-\uffff]/; +const DOLLAR_TAG = /\$(?:[A-Za-z_\u0080-\uffff][A-Za-z0-9_\u0080-\uffff]*)?\$/y; + +/* + * Split a view definition into plain SQL and quoted parts, in the same shape + * as String.split() with one capturing group: even indexes hold plain SQL and + * odd indexes hold a quoted identifier, string literal or dollar-quoted string + * exactly as written. This follows PostgreSQL's lexer: '' and "" are escaped + * quotes, E'' strings also take backslash escapes, block comments nest, and a + * $ that continues an identifier does not start a dollar-quoted string. + * Comments are replaced with a space in the plain SQL. A construct left + * unterminated runs to the end of the definition. This must stay in step with + * ViewNode._tokenise_definition() on the server. + */ +function tokeniseViewDefinition(definition) { + const followsIdent = (pos) => pos > 0 && IDENT_CHAR.test(definition[pos - 1]); + const n = definition.length; + let parts = ['']; + let i = 0; + while (i < n) { + const ch = definition[i]; + let pair = definition.slice(i, i + 2); + if (pair == '--') { + const end = definition.indexOf('\n', i); + i = end < 0 ? n : end; + parts[parts.length - 1] += ' '; + } else if (pair == '/*') { + let depth = 1; + i += 2; + while (i < n && depth) { + pair = definition.slice(i, i + 2); + if (pair == '/*') { + depth += 1; + i += 2; + } else if (pair == '*/') { + depth -= 1; + i += 2; + } else { + i += 1; + } + } + parts[parts.length - 1] += ' '; + } else if (ch == '"' || ch == '\'') { + const escapes = ch == '\'' && i > 0 && 'eE'.includes(definition[i - 1]) && + !followsIdent(i - 1); + let j = i + 1; + while (j < n) { + if (escapes && definition[j] == '\\') { + j += 2; + } else if (definition[j] != ch) { + j += 1; + } else if (definition[j + 1] == ch) { + j += 2; + } else { + break; + } + } + j = Math.min(j + 1, n); + parts.push(definition.slice(i, j), ''); + i = j; + } else { + let tag = null; + if (ch == '$' && !followsIdent(i)) { + DOLLAR_TAG.lastIndex = i; + tag = DOLLAR_TAG.exec(definition); + } + if (tag === null) { + parts[parts.length - 1] += ch; + i += 1; + continue; + } + const end = definition.indexOf(tag[0], i + tag[0].length); + const j = end < 0 ? n : end + tag[0].length; + parts.push(definition.slice(i, j), ''); + i = j; + } + } + return parts; +} + /* * Split a view definition at each FROM keyword and normalise the parts so * that only changes that can affect the view's columns are noticed. The @@ -24,14 +107,13 @@ import { getPrivilegesForTableAndLikeObjects } from '../../../tables/static/js/t * case-insensitive) and has its whitespace removed, so a pure reformat (for * example with the SQL formatter, which can change keyword, function and data * type case) does not look like a change to the column list. FROM is only - * matched as a whole word outside quotes, so identifiers such as valid_from - * do not end the select list early. + * matched as a whole word outside quotes and comments, so identifiers such as + * valid_from, and comments such as /* from *\/, do not end the select list + * early. */ export function splitViewDefinition(definition) { let segments = ['']; - (definition ?? '').split( - /("(?:[^"]|"")*"|'(?:[^']|'')*')/ - ).forEach((part, i) => { + tokeniseViewDefinition(definition ?? '').forEach((part, i) => { if (i % 2 == 1) { segments[segments.length - 1] += part.startsWith('"') ? part.slice(1, -1) : part; diff --git a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/test_views_split_definition.py b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/test_views_split_definition.py index 7e629314b63..e3c0f75a9fd 100644 --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/test_views_split_definition.py +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/test_views_split_definition.py @@ -48,6 +48,34 @@ class ViewsSplitDefinitionTestCase(BaseTestGenerator): old="SELECT 'x FROM y' AS a, b FROM t1", new="SELECT 'x FROM y' AS a FROM t1", same=False)), + ('FROM inside a block comment does not split', dict( + old='SELECT a /* from */, b FROM t1', + new='SELECT a /* from */ FROM t1', + same=False)), + ('FROM inside a nested block comment does not split', dict( + old='SELECT a /* x /* y */ from */, b FROM t1', + new='SELECT a /* x /* y */ from */ FROM t1', + same=False)), + ('FROM inside a line comment does not split', dict( + old='SELECT a -- from\n, b FROM t1', + new='SELECT a -- from\nFROM t1', + same=False)), + ('Adding or removing a comment is ignored', dict( + old='SELECT a, /* note */ b -- the end\nFROM t1', + new='SELECT a, b FROM t1', + same=True)), + ('FROM inside a dollar-quoted string does not split', dict( + old='SELECT $q$ from $$ \' $q$ AS a, b FROM t1', + new='SELECT $q$ from $$ \' $q$ AS a FROM t1', + same=False)), + ('FROM inside an E string with escapes does not split', dict( + old="SELECT E'it\\'s from' AS a, b FROM t1", + new="SELECT E'it\\'s from' AS a FROM t1", + same=False)), + ('A $ inside an identifier does not start a dollar quote', dict( + old='SELECT a$q$, b FROM t1', + new='SELECT a$q$ FROM t1', + same=False)), ] def runTest(self): diff --git a/web/regression/javascript/schema_ui_files/view.ui.spec.js b/web/regression/javascript/schema_ui_files/view.ui.spec.js index abd0745fd79..33f4165d9b5 100644 --- a/web/regression/javascript/schema_ui_files/view.ui.spec.js +++ b/web/regression/javascript/schema_ui_files/view.ui.spec.js @@ -157,5 +157,32 @@ FROM t1 t WHERE (id > 1);`)).not.toBeNull(); splitViewDefinition('SELECT "ab" FROM t')); expect(splitViewDefinition(undefined)).toEqual(['']); }); + + it('does not split at FROM inside comments or quoted strings', ()=>{ + [ + ['SELECT a /* from */, b FROM t1', 'SELECT a /* from */ FROM t1'], + ['SELECT a /* x /* y */ from */, b FROM t1', + 'SELECT a /* x /* y */ from */ FROM t1'], + ['SELECT a -- from\n, b FROM t1', 'SELECT a -- from\nFROM t1'], + ['SELECT $q$ from $$ \' $q$ AS a, b FROM t1', + 'SELECT $q$ from $$ \' $q$ AS a FROM t1'], + ['SELECT E\'it\\\'s from\' AS a, b FROM t1', + 'SELECT E\'it\\\'s from\' AS a FROM t1'], + ['SELECT a$q$, b FROM t1', 'SELECT a$q$ FROM t1'], + ].forEach(([oldDef, newDef])=>{ + expect(splitViewDefinition(oldDef)).toHaveLength(2); + initializeSchemaWithData(schemaObj, {oid: 1, definition: oldDef}); + schemaObj.validate({definition: newDef}, jest.fn()); + expect(schemaObj.warningText).not.toBeNull(); + }); + }); + + it('does not warn when only a comment is added or removed', ()=>{ + initializeSchemaWithData(schemaObj, { + oid: 1, definition: 'SELECT a, /* note */ b -- the end\nFROM t1;', + }); + schemaObj.validate({definition: 'SELECT a, b FROM t1;'}, jest.fn()); + expect(schemaObj.warningText).toBeNull(); + }); }); });