From 28f2e4155df5eb4501ec90f9d3f99b49fbf99053 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Thu, 15 Oct 2026 16:03:03 +0000 Subject: [PATCH] test: setting an attribute on pgc_conn or its cursors raises instead of doing nothing (#1420) The watched connection and cursor forward reads through __getattr__ but had no __setattr__, so `pgc_conn.autocommit = False` made an attribute on the wrapper and left the real connection in autocommit: each statement committed and rollback() undid nothing, with no error. Both proxies now keep their own underscore state and refuse any other assignment with an AttributeError that names the attribute and says to use BEGIN and ROLLBACK or a connection of one's own. Red first: the two stub arms in test_writes_wrote_rows.py and the wiring arm in test_connection.py each got 'set' where they wanted 'refused'. No test in the corpus assigns a public attribute on either proxy (an AST scan of all 114 files), and the full corpus passes on PG17: 435 guard tests and 567 cluster tests, the latter serially as CI runs them. Co-Authored-By: Claude Opus 5.5 --- test/pytest/TESTS.md | 3 ++ test/pytest/expected_tests.txt | 8 +++-- test/pytest/pgc_vacuity.py | 25 +++++++++++++ test/pytest/test_connection.py | 16 +++++++++ test/pytest/test_writes_wrote_rows.py | 51 +++++++++++++++++++++++++++ 5 files changed, 101 insertions(+), 2 deletions(-) diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 9b4853390..3b34dc341 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -1266,6 +1266,7 @@ written. | `test_the_cluster_refuses_a_foreign_server` | the identity check can return False | | `test_the_connection_the_tests_use_is_watched` | writes through `pgc_conn` reach the zero-row guard, on both the connection and a handed-out cursor | | `test_the_acknowledgement_is_by_cursor_against_the_real_driver` | the write is stamped on the object the caller holds, which a stub cannot prove | +| `test_setting_autocommit_on_the_connection_the_tests_use_is_refused` | the wiring arm for #1420: `pgc_conn` itself refuses `autocommit = False` and stays in autocommit, against the real driver | Two of these deserve their reasoning stated. @@ -2281,6 +2282,8 @@ separately, so a reader sees a green test beside an error. | `test_wrote_refuses_a_statement_that_is_not_a_write` | `expect.wrote` on a `SELECT 0` is refused, not compared | | `test_the_acknowledgement_names_one_write_and_not_its_twin` | naming one zero does not acknowledge a different identical zero | | `test_acknowledging_both_identical_zeros_passes` | the control for that arm: naming both is legitimate | +| `test_a_setting_on_the_watched_connection_is_refused_not_dropped` | setting `autocommit` on the watched connection raises, naming the attribute and BEGIN/ROLLBACK, and the real connection keeps its value (#1420) | +| `test_a_setting_on_a_watched_cursor_is_refused_not_dropped` | the same refusal for an attribute set on a watched cursor | ### Two properties, two files, on purpose diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 6516a347c..db6bdedfb 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -283,7 +283,9 @@ # premise is refused, and its control), by collection over the NO_CLUSTER files on this tree. # guard_tests 429 -> 433 for #1382's test_every_suite_job_installs_the_extensions.py, four # tests, the twin of selftest part 640, by collection over the NO_CLUSTER files. -guard_tests 433 +# guard_tests 433 -> 435 for #1420's two arms in test_writes_wrote_rows.py (a setting +# on a watched connection or cursor is refused), by collection over NO_CLUSTER. +guard_tests 435 # The complement: tests that need the driver and a throwaway cluster. Until #1016 these ran # in no CI job at all -- a quarter of the corpus, green when somebody ran them by hand and @@ -669,4 +671,6 @@ guard_tests 433 # test_collation_change.py, by collection over the cluster files on this tree. # cluster_tests 565 -> 566 for #1360's test_a_read_inside_an_aborted_inner_block_keeps_the_outer_statements_rows # in test_am_contracts.py, by collection over the cluster files on this tree. -cluster_tests 566 +# cluster_tests 566 -> 567 for #1420's wiring arm in test_connection.py, by collection +# over the cluster files on this tree. +cluster_tests 567 diff --git a/test/pytest/pgc_vacuity.py b/test/pytest/pgc_vacuity.py index 44d41d8e4..753656cc9 100644 --- a/test/pytest/pgc_vacuity.py +++ b/test/pytest/pgc_vacuity.py @@ -1152,6 +1152,25 @@ def expect(request): _RECORDERS.pop(request.node.nodeid, None) +def _refuse_set(proxy, attr, value, inner): + """__setattr__ for the watched proxies: their own state is set, anything else refused. + + The proxies forward READS through __getattr__, so without this a setting such + as `pgc_conn.autocommit = False` lands on the proxy, the real object keeps its + value, and the test goes on as if it had changed (#1420). Refused rather than + forwarded, so a test that wants a transaction says so with BEGIN and ROLLBACK + or opens a connection of its own, as the existing twins do. + """ + if attr.startswith("_"): + object.__setattr__(proxy, attr, value) + return + raise AttributeError( + f"{type(proxy).__name__} does not forward setting {attr!r}: the {inner} " + f"would keep its value while the test went on as if it had changed (#1420). " + f"For a transaction, run BEGIN and ROLLBACK, or open a connection of your own." + ) + + class _WatchedCursor: """A psycopg cursor that reports every write it runs to the guard. @@ -1188,6 +1207,9 @@ def executemany(self, *args, **kwargs): def __getattr__(self, attr): return getattr(self._cur, attr) + def __setattr__(self, attr, value): + _refuse_set(self, attr, value, "cursor") + def __iter__(self): return iter(self._cur) @@ -1220,6 +1242,9 @@ def cursor(self, *args, **kwargs): def __getattr__(self, attr): return getattr(self._conn, attr) + def __setattr__(self, attr, value): + _refuse_set(self, attr, value, "connection") + def __enter__(self): self._conn.__enter__() return self diff --git a/test/pytest/test_connection.py b/test/pytest/test_connection.py index 7e1023bcd..46dfe5e0d 100644 --- a/test/pytest/test_connection.py +++ b/test/pytest/test_connection.py @@ -334,6 +334,22 @@ def test_the_acknowledgement_is_by_cursor_against_the_real_driver(pgc_conn, expe expect.wrote(first, 0, "so name the first explicitly too") +def test_setting_autocommit_on_the_connection_the_tests_use_is_refused(pgc_conn, expect): + """The wiring arm for #1420: the stub arms in test_writes_wrote_rows.py prove the + wrapper refuses, and this proves `pgc_conn` IS that wrapper, against the real + driver. Before the fix the assignment succeeded on the wrapper and the server + went on committing every statement. + """ + try: + pgc_conn.autocommit = False + outcome = "set" + except AttributeError as exc: + outcome = f"refused: {exc}" + expect.text(outcome.split(":")[0], "refused", + "setting autocommit on pgc_conn is refused, not silently ignored") + expect.text(str(pgc_conn.autocommit), "True", "and pgc_conn is still in autocommit") + + def test_the_block_compression_default_is_pinned_to_its_measurement(pgc_conn, expect): """#890: zstd:3 stays the default until a measurement says otherwise. diff --git a/test/pytest/test_writes_wrote_rows.py b/test/pytest/test_writes_wrote_rows.py index 82ff35cee..8b05c52ef 100644 --- a/test/pytest/test_writes_wrote_rows.py +++ b/test/pytest/test_writes_wrote_rows.py @@ -273,3 +273,54 @@ def test_two_named_zeros(expect, request): expect.wrote(second, 0, "and so did the second") """) expect.outcomes(result, "naming both zeros passes", passed=1, failed=0) + + +class _Conn: + """A connection's one setting this guard reads, and the cursor it hands out.""" + + def __init__(self): + self.autocommit = True + + def cursor(self): + return _Cur("SELECT 0", 0) + + +def _try_set(obj, attr, value): + """Set `attr` on `obj` -> 'set', or 'refused: '.""" + try: + setattr(obj, attr, value) + except AttributeError as exc: + return f"refused: {exc}" + return "set" + + +def test_a_setting_on_the_watched_connection_is_refused_not_dropped(expect, request): + """Setting an attribute on the wrapper must not pass for setting it on the connection. + + The wrapper forwards reads, so `pgc_conn.autocommit` reads the real value. With + no `__setattr__`, `pgc_conn.autocommit = False` made a new attribute on the + wrapper and left the real connection in autocommit, so each statement committed + and `rollback()` undid nothing (#1420). Refusing it makes the mistake loud. + """ + real = _Conn() + watched = pgc_vacuity.watch_writes(real, request.node.nodeid + "::conn") + expect.text(str(watched.autocommit), "True", + "premise: a setting read through the watched connection is the real one") + outcome = _try_set(watched, "autocommit", False) + expect.text(outcome.split(":")[0], "refused", + "setting autocommit on the watched connection is refused, not kept on the wrapper") + expect.text("yes" if "autocommit" in outcome and "BEGIN" in outcome else outcome, "yes", + "and the refusal names the attribute and what to do instead") + expect.text(str(real.autocommit), "True", "and the real connection is unchanged") + + +def test_a_setting_on_a_watched_cursor_is_refused_not_dropped(expect, request): + """The same for the cursor proxy: `cur.row_factory = ...` would be dropped too.""" + watched = pgc_vacuity.watch_writes(_Conn(), request.node.nodeid + "::cur").cursor() + expect.text(type(watched).__name__, "_WatchedCursor", + "premise: the connection hands out a watched cursor") + outcome = _try_set(watched, "row_factory", None) + expect.text(outcome.split(":")[0], "refused", + "setting an attribute on a watched cursor is refused, not kept on the wrapper") + expect.text("yes" if "row_factory" in outcome else outcome, "yes", + "and the refusal names the attribute")