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..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,107 @@ 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): + """ + 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 and comments, so identifiers such as + valid_from, and comments such as /* from */, do not end the select + list early. + """ + segments = [''] + 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 + 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): """ Check and process definition data. @@ -925,9 +1026,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 = re.sub(r"\W", "", data['definition']).split('FROM') - old_def = re.sub(r"\W", "", 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 b65e34cb83f..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,119 @@ 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 + * 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 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 = ['']; + tokeniseViewDefinition(definition ?? '').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 { constructor(getPrivilegeRoleSchema, nodeInfo, fieldOptions={}, initValues={}) { @@ -147,15 +260,11 @@ export default class ViewSchema extends BaseUISchema { return false; } - let old_def = obj.origData.definition?.replace( - /\s/gi, '' - ).split('FROM'), + let old_def = splitViewDefinition(obj.origData.definition), new_def = []; if (!_.isUndefined(state.definition)) { - new_def = state.definition.replace( - /\s/gi, '' - ).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_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/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..e3c0f75a9fd --- /dev/null +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/views/tests/test_views_split_definition.py @@ -0,0 +1,85 @@ +########################################################################## +# +# 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)), + ('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): + 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 c2a00d15c38..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 @@ -1530,6 +1530,66 @@ "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 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\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": {}, + "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 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 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": {}, + "expected_data": { + "status_code": 200, + "error_msg": null, + "test_result_data": { + "sql_contains": "DROP VIEW" + } + } + }, + { + "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 832ce0a24fb..33f4165d9b5 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, { 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'; @@ -75,4 +75,114 @@ 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('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(['']); + }); + + 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(); + }); + }); });