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")