diff --git a/src/informix_db/cursors.py b/src/informix_db/cursors.py index 70cdc5f..c8240d3 100644 --- a/src/informix_db/cursors.py +++ b/src/informix_db/cursors.py @@ -102,6 +102,39 @@ def _make_socket_reader(sock): return _SocketReader(sock) +# Statement types from the DESCRIBE response's first field. The server +# tells us exactly what it prepared; these are the two values that mean +# "this will produce rows". +_ST_SELECT = 2 +_ST_ROUTINE = 56 # EXECUTE PROCEDURE / EXECUTE FUNCTION + + +def _produces_result_set(statement_type: int, ncolumns: int) -> bool: + """Whether a prepared statement needs a cursor opened for it. + + This is JDBC's ``IfxSqli.isResultSet`` predicate, and it replaces a + first-word check for ``SELECT`` that got five ordinary forms wrong. + A leading comment of any of the three Informix flavours, a + parenthesized select, a parenthesized UNION, and a CTE were all + classified as DML, so the driver sent SQ_EXECUTE where the server + expected a cursor. The report back was ``-260 Cursor name already in + use``, which describes neither the cause nor anything the caller did. + + The original comment justified the heuristic on the grounds that + ``nfields`` can't distinguish these, because ``INSERT INTO t VALUES + (?)`` also describes a column. True, and irrelevant: ``statement_type`` + distinguishes them exactly. That INSERT reports 6 with one field; every + SELECT form above reports 2. The value was being parsed into the + describe metadata and thrown away. + + ``EXECUTE PROCEDURE``/``FUNCTION`` (56) is the one type that depends on + the column count, since a routine may or may not return rows. + """ + if statement_type == _ST_SELECT: + return True + return statement_type == _ST_ROUTINE and ncolumns > 0 + + def _finalize_cursor( conn_ref: weakref.ReferenceType, state: list, @@ -283,6 +316,9 @@ class Cursor: # from. Empirically the server accepts 0 here even when a real # ID was assigned, so this is best-effort tracking. self._statement_id: int = 0 + # DESCRIBE's statement-type field. Decides whether a cursor is + # opened -- see _produces_result_set. + self._statement_type: int = 0 # Phase 10: smart-LOB read via ``lotofile(col, path, 'client')``. # The server orchestrates a SQ_FILE (98) protocol where it tells # us to "open file X, write these bytes, close". We emulate the @@ -384,6 +420,7 @@ class Cursor: self._rowcount = -1 self._rows = [] self._row_index = -1 # before-first-row + self._statement_type = 0 self._statement_already_done = False # On a logged DB in non-autocommit mode, the server requires an @@ -401,15 +438,9 @@ class Cursor: ) self._read_describe_response() - # Branch on the SQL keyword. We can't use ``self._columns`` / - # ``nfields`` here because a parameterized INSERT also returns - # a non-empty DESCRIBE (server describes the would-be inserted - # row's columns). The SQL-keyword heuristic is what JDBC effectively - # does too via its IfxStatement / IfxPreparedStatement subclassing. - first_word = sql.lstrip().split(None, 1)[0].upper() if sql.strip() else "" - is_select = first_word == "SELECT" - - if is_select: + # Ask the server what it just prepared, rather than guessing from + # the first word of the SQL. + if _produces_result_set(self._statement_type, len(self._columns)): if params: self._execute_select_with_params(params) else: @@ -1074,7 +1105,10 @@ class Cursor: f"expected {first_len} (matching set [0])" ) - # Detect SELECT — not supported in executemany. + # Cheap pre-flight reject for the obvious case, so the common + # mistake costs no round-trip. The authoritative check is after + # PREPARE, below — this one shares the first-word heuristic's + # blind spots (leading comments, CTEs, parenthesized selects). first_word = operation.lstrip().split(None, 1)[0].upper() if operation.strip() else "" if first_word == "SELECT": raise NotSupportedError("executemany on SELECT is not supported") @@ -1095,6 +1129,7 @@ class Cursor: self._rowcount = -1 self._rows = [] self._row_index = -1 + self._statement_type = 0 self._statement_already_done = False # Logged-DB transaction guard — same as execute(). Idempotent @@ -1108,6 +1143,18 @@ class Cursor: ) self._read_describe_response() + # Now the server has told us what it prepared. A result-set + # statement that slipped past the first-word check above -- + # a CTE, a leading comment, a parenthesized select -- would + # otherwise be executed N times down the DML path, which + # opens no cursor and answers -260. + if _produces_result_set(self._statement_type, len(self._columns)): + self._release_after_failure() + raise NotSupportedError( + "executemany on a statement that returns rows is not " + "supported" + ) + # Phase 33: pipeline — build all BIND+EXECUTE PDUs first # (Python work, no I/O), then send them back-to-back, then # drain all responses. Eliminates the per-row round-trip @@ -1735,6 +1782,7 @@ class Cursor: elif tag == MessageType.SQ_DESCRIBE: self._columns, meta = parse_describe(reader) self._statement_id = meta.get("statement_id", 0) + self._statement_type = meta.get("statement_type", 0) self._description = ( [c.to_description_tuple() for c in self._columns] if self._columns else None ) diff --git a/tests/test_statement_classification.py b/tests/test_statement_classification.py new file mode 100644 index 0000000..2b9a644 --- /dev/null +++ b/tests/test_statement_classification.py @@ -0,0 +1,216 @@ +"""Deciding whether a statement needs a cursor, by asking rather than guessing. + +The driver chose between "open a cursor and fetch" and "execute and +release" by checking whether the first word of the SQL was ``SELECT``. +That gets five ordinary forms wrong — a leading comment in any of the +three Informix flavours, a parenthesized select, a parenthesized UNION, +and a CTE. All five are perfectly good queries, and all five failed with +``-260 Cursor name already in use``, an error that describes neither the +cause nor anything the caller did. It says "cursor" because the driver +sent SQ_EXECUTE where the server was waiting to open one. + +The server had been telling us the answer the whole time. +``statement_type`` is the first field of the DESCRIBE response, and +``parse_describe`` has always parsed it into the metadata dict, where +nothing read it. Every SELECT form above reports 2. + +The comment that justified the heuristic said ``nfields`` couldn't +distinguish these cases, because ``INSERT INTO t VALUES (?)`` also +describes a column. That much was true — and it argued for the wrong +conclusion, because ``statement_type`` distinguishes them exactly. That +INSERT reports 6. + +The predicate is now JDBC's ``IfxSqli.isResultSet``: type 2, or type 56 +(``EXECUTE PROCEDURE``/``FUNCTION``) with at least one column, since a +routine may or may not return rows. That last clause fixes +``EXECUTE FUNCTION`` as a side effect — it used to run down the DML path +and discard its return value. +""" + +from __future__ import annotations + +import contextlib + +import pytest + +import informix_db +from informix_db.cursors import _produces_result_set +from tests.conftest import ConnParams + +# --------------------------------------------------------------------------- +# The predicate +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("statement_type", "ncolumns", "expected", "why"), + [ + (2, 1, True, "SELECT"), + (2, 0, True, "SELECT describing no columns is still a SELECT"), + (6, 1, False, "INSERT ... VALUES (?) describes a column but is DML"), + (6, 0, False, "INSERT with literals"), + (32, 0, False, "DELETE"), + (33, 0, False, "UPDATE"), + (45, 0, False, "CREATE"), + (56, 0, False, "EXECUTE PROCEDURE returning nothing"), + (56, 2, True, "EXECUTE FUNCTION returning rows"), + (0, 0, False, "unknown type defaults to the non-cursor path"), + ], +) +def test_result_set_predicate( + statement_type: int, ncolumns: int, expected: bool, why: str +) -> None: + assert _produces_result_set(statement_type, ncolumns) is expected, why + + +# --------------------------------------------------------------------------- +# Against a real server +# --------------------------------------------------------------------------- + + +def _connect(conn_params: ConnParams) -> informix_db.Connection: + return informix_db.connect( + host=conn_params.host, + port=conn_params.port, + user=conn_params.user, + password=conn_params.password, + database=conn_params.database, + server=conn_params.server, + connect_timeout=10.0, + read_timeout=25.0, + autocommit=True, + ) + + +_ONE = "SELECT FIRST 1 tabid FROM systables" + + +@pytest.mark.integration +@pytest.mark.parametrize( + ("label", "sql"), + [ + pytest.param("plain", _ONE, id="plain"), + pytest.param("lowercase", _ONE.lower(), id="lowercase"), + pytest.param("leading-whitespace", f" \n\t {_ONE}", id="whitespace"), + pytest.param("line-comment", f"-- pick one\n{_ONE}", id="line-comment"), + pytest.param("block-comment", f"/* pick one */ {_ONE}", id="block-comment"), + pytest.param("brace-comment", f"{{ pick one }} {_ONE}", id="brace-comment"), + pytest.param( + "cte", + "WITH c AS (SELECT tabid FROM systables) " + "SELECT FIRST 1 tabid FROM c", + id="cte", + ), + pytest.param("parenthesized", f"({_ONE})", id="parenthesized"), + pytest.param( + "union", f"{_ONE} UNION SELECT 99 FROM systables", id="union" + ), + pytest.param( + "parenthesized-union", + f"({_ONE}) UNION (SELECT 99 FROM systables)", + id="paren-union", + ), + ], +) +def test_every_select_form_opens_a_cursor( + conn_params: ConnParams, label: str, sql: str +) -> None: + """The five non-``plain`` forms below the whitespace case all failed + with -260 under the first-word heuristic.""" + with _connect(conn_params) as conn: + cur = conn.cursor() + try: + cur.execute(sql) + except informix_db.ProgrammingError as exc: + # Informix 12.10 has no CTEs and rejects WITH at offset 1. + # A syntax error is the server declining the grammar, which + # is a different thing from the driver routing it wrongly — + # that produced -260, not -201. + if getattr(exc, "sqlcode", None) == -201: + pytest.skip(f"server does not support this syntax: {label}") + raise + rows = cur.fetchall() + assert rows, f"{label}: expected rows, got none" + assert cur.description is not None + + +@pytest.mark.integration +def test_parameterized_insert_is_not_mistaken_for_a_query( + conn_params: ConnParams, +) -> None: + """The case the old comment worried about, and the reason it kept the + heuristic: this DESCRIBEs a column. ``statement_type`` says 6.""" + with _connect(conn_params) as conn: + cur = conn.cursor() + cur.execute("CREATE TEMP TABLE t_cls (k INT)") + cur.execute("INSERT INTO t_cls VALUES (?)", (7,)) + assert cur.rowcount == 1 + cur.execute("SELECT k FROM t_cls") + assert cur.fetchall() == [(7,)] + + +@pytest.mark.integration +def test_execute_function_returns_its_value(conn_params: ConnParams) -> None: + """Type 56 with columns. Under the first-word heuristic this ran down + the DML path and the return value was discarded.""" + with _connect(conn_params) as conn: + cur = conn.cursor() + with contextlib.suppress(Exception): + cur.execute("DROP FUNCTION ifxdrv_dbl") + cur.execute( + "CREATE FUNCTION ifxdrv_dbl(n INT) RETURNING INT; " + "RETURN n * 2; END FUNCTION" + ) + try: + cur.execute("EXECUTE FUNCTION ifxdrv_dbl(21)") + assert cur.fetchall() == [(42,)] + finally: + with contextlib.suppress(Exception): + cur.execute("DROP FUNCTION ifxdrv_dbl") + + +@pytest.mark.integration +def test_dml_still_takes_the_non_cursor_path(conn_params: ConnParams) -> None: + with _connect(conn_params) as conn: + cur = conn.cursor() + cur.execute("CREATE TEMP TABLE t_cls2 (k INT)") + cur.execute("INSERT INTO t_cls2 VALUES (1)") + cur.execute("UPDATE t_cls2 SET k = 2") + assert cur.rowcount == 1 + cur.execute("DELETE FROM t_cls2") + assert cur.rowcount == 1 + assert cur.description is None + + +@pytest.mark.integration +def test_executemany_refuses_a_query_the_first_word_missed( + conn_params: ConnParams, +) -> None: + """The pre-flight check shares the heuristic's blind spots, so a + comment-prefixed SELECT reaches PREPARE. The post-DESCRIBE check + catches it, and the connection stays usable — the refusal releases + the statement. (A leading comment rather than a CTE, so this also + runs on 12.10, which has no CTEs.)""" + with _connect(conn_params) as conn: + cur = conn.cursor() + with pytest.raises(informix_db.NotSupportedError): + cur.executemany( + "/* batched? no */ SELECT FIRST 1 tabid FROM systables " + "WHERE tabid <> ?", + [(1,), (2,)], + ) + cur.execute("SELECT FIRST 1 tabid FROM systables") + assert cur.fetchone() is not None, "refusal leaked the statement" + + +@pytest.mark.integration +def test_executemany_still_refuses_a_plain_select( + conn_params: ConnParams, +) -> None: + with _connect(conn_params) as conn: + cur = conn.cursor() + with pytest.raises(informix_db.NotSupportedError): + cur.executemany( + "SELECT FIRST 1 tabid FROM systables WHERE tabid <> ?", + [(1,), (2,)], + )