diff --git a/web/pgadmin/tools/sqleditor/__init__.py b/web/pgadmin/tools/sqleditor/__init__.py index 8080d220a54..8cdc44bea7d 100644 --- a/web/pgadmin/tools/sqleditor/__init__.py +++ b/web/pgadmin/tools/sqleditor/__init__.py @@ -1150,7 +1150,8 @@ def poll(trans_id): 'transaction_status': transaction_status, 'explain_query_length': get_explain_query_length(conn._Connection__async_cursor._query) - if conn._Connection__async_cursor else 0 + if conn._Connection__async_cursor and + conn._Connection__async_cursor._query else 0 } return internal_server_error(result, query_len_data) elif status == ASYNC_OK: diff --git a/web/pgadmin/tools/sqleditor/tests/test_poll_explain_query_length_guard.py b/web/pgadmin/tools/sqleditor/tests/test_poll_explain_query_length_guard.py new file mode 100644 index 00000000000..7286af05a7b --- /dev/null +++ b/web/pgadmin/tools/sqleditor/tests/test_poll_explain_query_length_guard.py @@ -0,0 +1,90 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +"""Regression test for a review comment on PR #10321 (pgAdmin issue +#8991): poll()'s error-handling branch built the 'explain_query_length' +value with:: + + get_explain_query_length(conn._Connection__async_cursor._query) + if conn._Connection__async_cursor else 0 + +which only guarded against the cached async cursor itself being falsy, +not against its ``_query`` attribute being ``None``. PR #10321's own fix +runs BEGIN/COMMIT/ROLLBACK through a throwaway plain cursor under +"server cursor" mode; once that has happened the cached async cursor +that poll() sees next can be a cursor that has not yet executed a real +statement, so ``_query`` is still ``None``. get_explain_query_length() +then immediately does ``query_obj.query.decode()``, and with +``query_obj`` being ``None`` that crashes with:: + + AttributeError: 'NoneType' object has no attribute 'query' + +turning any query error that follows a commit under "server cursor" +mode into an unhandled 500 and leaving the Query Tool unusable, instead +of the normal JSON error response.""" + +import json +import secrets +from unittest.mock import MagicMock, patch + +from pgadmin.utils.route import BaseTestGenerator + + +class TestPollExplainQueryLengthGuard(BaseTestGenerator): + """poll() must not crash while building 'explain_query_length' when + the cached async cursor has not yet executed any statement.""" + + scenarios = [ + ('Cached async cursor has not executed a statement yet ' + '(_query is None) - poll() must not crash', dict()) + ] + + def runTest(self): + trans_id = secrets.choice(range(1, 9999999)) + + # A cursor left over from execute_void()'s throwaway plain + # cursor (or a freshly (re)created server-side cursor) that has + # not executed a real statement yet - exactly the state PR + # #10321's own fix can leave behind after a commit under + # "server cursor" mode. + async_cursor = MagicMock() + async_cursor._query = None + + conn = MagicMock() + conn.poll.return_value = (False, 'some query error') + conn.connected.return_value = True + conn.messages.return_value = [] + conn.transaction_status.return_value = 0 + conn._Connection__async_cursor = async_cursor + + trans_obj = MagicMock() + trans_obj.get_thread_native_id.return_value = None + + session_obj = {} + + with patch( + 'pgadmin.tools.sqleditor.check_transaction_status', + return_value=(True, None, conn, trans_obj, session_obj) + ): + response = self.tester.get( + '/sqleditor/poll/{0}'.format(trans_id)) + + # Before the fix this either raised AttributeError outright, or + # (via the app's generic exception handler) came back as a 500 + # whose errormsg was the raw AttributeError text instead of the + # intended query-error response. + response_text = response.data.decode('utf-8') + self.assertNotIn( + "'NoneType' object has no attribute 'query'", response_text) + + response_data = json.loads(response_text) + self.assertEqual(response.status_code, 500) + self.assertEqual(response_data['errormsg'], 'some query error') + self.assertEqual( + response_data['data']['explain_query_length'], 0) diff --git a/web/pgadmin/utils/driver/psycopg3/connection.py b/web/pgadmin/utils/driver/psycopg3/connection.py index d07a16cefcd..0229b417c75 100644 --- a/web/pgadmin/utils/driver/psycopg3/connection.py +++ b/web/pgadmin/utils/driver/psycopg3/connection.py @@ -14,6 +14,7 @@ """ import os +import re import secrets import datetime import asyncio @@ -59,6 +60,73 @@ configure_driver_encodings(encodings) +# Statements that leave no result set behind for poll() to report on, and +# which therefore need the cursor they ran on to become the async cursor. +# ROLLBACK covers ROLLBACK TO SAVEPOINT as well, and START covers START +# TRANSACTION, there being no other START in the grammar. +TRANSACTION_CONTROL_KEYWORDS = frozenset({ + 'abort', 'begin', 'commit', 'end', 'release', 'rollback', 'savepoint', + 'start' +}) + + +# A leading identifier or keyword, as PostgreSQL's lexer reads one. +_LEADING_WORD = re.compile(r'[^\W\d][\w$]*') + + +def _skip_leading_comments(query): + """ + Return the given statement with any leading whitespace and SQL comments + removed. Both -- line comments and /* */ block comments are skipped, + the latter nesting as they do in PostgreSQL. + + Args: + query: SQL statement + """ + pos = 0 + length = len(query) + + while pos < length: + if query[pos].isspace(): + pos += 1 + elif query.startswith('--', pos): + newline = query.find('\n', pos) + pos = length if newline == -1 else newline + 1 + elif query.startswith('/*', pos): + depth = 1 + pos += 2 + while pos < length and depth: + if query.startswith('/*', pos): + depth += 1 + pos += 2 + elif query.startswith('*/', pos): + depth -= 1 + pos += 2 + else: + pos += 1 + else: + break + + return query[pos:] + + +def _is_transaction_control(query): + """ + Report whether the given statement is a transaction-control statement, + judged by its leading keyword once any leading comments are skipped. + + Args: + query: SQL statement, as passed to execute_void() + """ + # Take the keyword as a whole word, so that a comment or semicolon + # written straight after it (COMMIT/* note */; or COMMIT;-- note) does + # not become part of it, whilst BEGINNING is still not BEGIN. + match = _LEADING_WORD.match(_skip_leading_comments(query)) + + return bool(match) and \ + match.group().lower() in TRANSACTION_CONTROL_KEYWORDS + + class Connection(BaseConnection): """ class Connection(object) @@ -1173,6 +1241,39 @@ def execute_void(self, query, params=None, formatted_exception_msg=False): if not status: return False, str(cur) + + if isinstance(cur, AsyncDictServerCursor): + # A named/server-side cursor's execute() always runs the query + # as `DECLARE ... CURSOR FOR `, which cannot express a + # transaction-control statement such as BEGIN/COMMIT/ROLLBACK. + # Run this one statement through a throwaway plain cursor + # instead, leaving the cursor cached for the connection in + # place for the next query to reuse. The connection's + # cursor_factory is AsyncDictCursor, so the throwaway carries + # ordered_description(), get_rowcount() and the rest of the + # API poll() calls. + cur = self.conn.cursor() + + if _is_transaction_control(query): + # For a transaction-control statement the throwaway also + # has to become the async cursor, because poll() and + # status_message() report on that rather than on whatever + # this call used: the cached server-side cursor still + # describes the previous query and reports itself open, so + # a following poll() would read straight past its "not cur + # or cur.closed" guard and put that query's column metadata + # and row count back over the "no result set" the + # statement leaves behind. + # + # Anything else keeps the cached cursor as the async + # cursor. A statement such as the SELECT pg_cancel_backend() + # issued by cancel_transaction() has no business detaching + # the cursor a result set is still being paged or + # downloaded from. + self.__async_cursor = cur + self.column_info = None + self.row_count = 0 + query_id = str(secrets.choice(range(1, 9999999))) current_app.logger.log( diff --git a/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py b/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py new file mode 100644 index 00000000000..4475c3c4e01 --- /dev/null +++ b/web/pgadmin/utils/driver/psycopg3/tests/test_execute_void_server_cursor.py @@ -0,0 +1,254 @@ +########################################################################## +# +# pgAdmin 4 - PostgreSQL Tools +# +# Copyright (C) 2013 - 2026, The pgAdmin Development Team +# This software is released under the PostgreSQL Licence +# +########################################################################## + +"""Regression test: ``execute_void()`` must not run a transaction-control +statement (BEGIN/COMMIT/ROLLBACK) through a cached named/server-side +cursor. + +A named cursor's ``execute()`` always wraps the statement as +``DECLARE ... CURSOR FOR ``, which cannot express BEGIN/COMMIT/ +ROLLBACK. Before the fix, the Commit/Rollback buttons under "server +cursor" mode silently did nothing: the DECLARE-wrapped call failed +(actually failing one step earlier, on a ``prepare`` keyword the +server-side cursor's ``execute()`` doesn't accept at all), the exception +was swallowed by the background query thread, and the next poll() then +reported the *previous* query's leftover column info, making the result +grid appear instead of the Messages tab (pgAdmin issue #8991). + +Clearing ``column_info``/``row_count`` in ``execute_void()`` is not enough +on its own, because ``poll()`` rebuilds both from whatever +``self.__async_cursor`` points at, and that is still the cached +server-side cursor: it reports itself open, so the ``not cur or +cur.closed`` guard lets it through and the previous query's metadata comes +straight back. The throwaway cursor therefore has to become the async +cursor as well, which also makes ``status_message()`` report the +transaction-control statement rather than the previous query. That +promotion is limited to transaction-control statements, for the reason +given in ``ExecuteVoidNonTransactionServerCursorTest`` below.""" + +from unittest.mock import MagicMock, patch + +from pgadmin.utils.driver.psycopg3.connection import ( + Connection, _is_transaction_control +) +from pgadmin.utils.driver.psycopg3.cursor import AsyncDictServerCursor +from pgadmin.utils.route import BaseTestGenerator + + +class ExecuteVoidServerCursorTest(BaseTestGenerator): + + scenarios = [ + ('COMMIT with a cached server-side cursor runs on a throwaway ' + 'plain cursor, and a following poll() reports no result set', + dict(sql='COMMIT;')), + ('ROLLBACK with a cached server-side cursor runs on a throwaway ' + 'plain cursor, and a following poll() reports no result set', + dict(sql='ROLLBACK;')), + ] + + def runTest(self): + manager = MagicMock(sid=1) + conn = Connection(manager, 'test-conn-id', 'testdb') + conn.python_encoding = 'utf-8' + + # Leftover state from a previous SELECT executed through the + # server-side cursor. + conn.column_info = [{'name': 'x'}] + conn.row_count = 1 + + # The cursor the previous SELECT ran on, which is both cached for + # the connection and still referenced as the async cursor. It + # reports itself open, and still describes that SELECT's result. + stale_column = MagicMock() + stale_column.to_dict.return_value = {'name': 'x'} + server_cursor = MagicMock(spec=AsyncDictServerCursor) + server_cursor.closed = False + server_cursor.description = [stale_column] + server_cursor.ordered_description.return_value = [stale_column] + # AsyncDictServerCursor.get_rowcount() answers 1 unconditionally. + server_cursor.get_rowcount.return_value = 1 + server_cursor.nextset.return_value = None + server_cursor.statusmessage = 'SELECT 1' + conn._Connection__async_cursor = server_cursor + + # The throwaway cursor execute_void() should use instead. A + # transaction-control statement leaves no result set behind, so it + # has no description and no rows. + plain_cursor = MagicMock() + plain_cursor.closed = False + # Values taken from what psycopg actually leaves on the cursor + # after a COMMIT/ROLLBACK: no description, and a result with no + # tuples in it, which AsyncDictCursor.get_rowcount() reports as 0. + plain_cursor.description = None + plain_cursor.get_rowcount.return_value = 0 + plain_cursor.nextset.return_value = None + plain_cursor.statusmessage = self.sql.rstrip(';') + + conn.conn = MagicMock() + conn.conn.cursor.return_value = plain_cursor + conn.conn.info.user = 'postgres' + conn.conn.info.host = 'localhost' + conn.conn.info.dbname = 'testdb' + # Not ACTIVE, and no connection level error, so poll() gets as far + # as reading the cursor rather than answering from either of those. + conn.conn.info.transaction_status = 2 + conn.conn.pgconn.error_message = None + + # current_user needs a real request context to resolve at all; + # patch it only once inside that context, to a stand-in with the + # attribute execute_void()'s log line reads. + with self.app.test_request_context(): + with patch( + 'pgadmin.utils.driver.psycopg3.connection.current_user', + MagicMock(email='test@example.com') + ), patch.object(Connection, '_Connection__cursor', + return_value=(True, server_cursor)): + status, result = conn.execute_void(self.sql) + + self.assertTrue(status) + self.assertIsNone(result) + + # The statement ran on the throwaway plain cursor, not the + # cached server-side one. + plain_cursor.execute.assert_called_once() + server_cursor.execute.assert_not_called() + + # Stale result-set state from the prior SELECT must not leak + # into whatever poll() call comes next. + self.assertIsNone(conn.column_info) + self.assertEqual(conn.row_count, 0) + + # ... and the poll() that the Query Tool makes next must not put it + # back. This is the call that made the result grid appear instead + # of the Messages tab, because it rebuilds column_info and + # row_count from the async cursor, which was still the server-side + # one describing the previous SELECT. + with self.app.test_request_context(): + status, result = conn.poll(no_result=True) + status_message = conn.status_message() + + self.assertEqual(status, 1) + self.assertIsNone(result) + self.assertIsNone(conn.column_info) + self.assertEqual(conn.row_count, 0) + server_cursor.ordered_description.assert_not_called() + + # The status message belongs to the statement just run, not to the + # previous query. + self.assertEqual(status_message, self.sql.rstrip(';')) + + +class ExecuteVoidNonTransactionServerCursorTest(BaseTestGenerator): + """A statement that is not transaction control must leave the cached + server-side cursor in place as the async cursor. + + ``execute_void()`` runs on a throwaway plain cursor whenever the cached + cursor is a server-side one, but only a transaction-control statement + has that throwaway become the async cursor. Promoting it for every + statement would let something like the ``SELECT pg_cancel_backend(...)`` + that ``cancel_transaction()`` issues detach the cursor a result set is + still being paged or downloaded from, so that the pagination and + download calls that follow read the throwaway and find no rows. + """ + + scenarios = [ + ('a non-transaction statement keeps the cached server-side cursor ' + 'as the async cursor', + dict(sql='SELECT pg_cancel_backend(1234);')), + ] + + def runTest(self): + manager = MagicMock(sid=1) + conn = Connection(manager, 'test-conn-id', 'testdb') + conn.python_encoding = 'utf-8' + + # State from the query whose result set is still being read. + conn.column_info = [{'name': 'x'}] + conn.row_count = 1 + + server_cursor = MagicMock(spec=AsyncDictServerCursor) + server_cursor.closed = False + conn._Connection__async_cursor = server_cursor + + plain_cursor = MagicMock() + plain_cursor.closed = False + + conn.conn = MagicMock() + conn.conn.cursor.return_value = plain_cursor + conn.conn.info.user = 'postgres' + conn.conn.info.host = 'localhost' + conn.conn.info.dbname = 'testdb' + + with self.app.test_request_context(): + with patch( + 'pgadmin.utils.driver.psycopg3.connection.current_user', + MagicMock(email='test@example.com') + ), patch.object(Connection, '_Connection__cursor', + return_value=(True, server_cursor)): + status, result = conn.execute_void(self.sql) + + self.assertTrue(status) + self.assertIsNone(result) + + # It still runs on the throwaway, since a server-side cursor cannot + # execute anything except through DECLARE ... CURSOR FOR. + plain_cursor.execute.assert_called_once() + server_cursor.execute.assert_not_called() + + # ... but the result set being read is left alone. + self.assertIs(conn._Connection__async_cursor, server_cursor) + self.assertEqual(conn.column_info, [{'name': 'x'}]) + self.assertEqual(conn.row_count, 1) + + +class IsTransactionControlTest(BaseTestGenerator): + """Unit tests for the leading-keyword check that decides whether a + statement is transaction control.""" + + scenarios = [ + ('BEGIN', dict(sql='BEGIN;', expected=True)), + ('COMMIT', dict(sql='COMMIT;', expected=True)), + ('ROLLBACK', dict(sql='ROLLBACK;', expected=True)), + ('lower case, no semicolon', dict(sql='commit', expected=True)), + ('leading whitespace', dict(sql=' \n\tROLLBACK;', expected=True)), + ('START TRANSACTION', dict(sql='START TRANSACTION;', expected=True)), + ('ROLLBACK TO SAVEPOINT', + dict(sql='ROLLBACK TO SAVEPOINT sp1;', expected=True)), + ('SAVEPOINT', dict(sql='SAVEPOINT sp1;', expected=True)), + ('RELEASE', dict(sql='RELEASE sp1;', expected=True)), + ('a SELECT', dict(sql='SELECT pg_cancel_backend(1234);', + expected=False)), + ('an INSERT', dict(sql='INSERT INTO t VALUES (1);', expected=False)), + # "beginx" is not "begin". + ('a keyword prefix', dict(sql='BEGINNING;', expected=False)), + ('an empty statement', dict(sql=' ', expected=False)), + ('a leading line comment', + dict(sql='-- finish up\nCOMMIT;', expected=True)), + ('a leading block comment', + dict(sql='/* finish up */ COMMIT;', expected=True)), + ('a nested block comment', + dict(sql='/* outer /* inner */ still outer */ROLLBACK;', + expected=True)), + ('several leading comments', + dict(sql=' -- one\n/* two */\n\t-- three\nBEGIN;', expected=True)), + ('a comment ahead of a SELECT', + dict(sql='/* COMMIT */ SELECT 1;', expected=False)), + ('only a comment', dict(sql='-- COMMIT', expected=False)), + ('an unterminated block comment', + dict(sql='/* COMMIT;', expected=False)), + ('a block comment straight after the keyword', + dict(sql='COMMIT/* note */;', expected=True)), + ('a line comment straight after the semicolon', + dict(sql='COMMIT;-- note', expected=True)), + ('a keyword followed by digits', + dict(sql='BEGIN1;', expected=False)), + ] + + def runTest(self): + self.assertEqual(_is_transaction_control(self.sql), self.expected)