test: setting an attribute on pgc_conn or its cursors raises instead of doing nothing (#1420)

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
 

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

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

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.
 

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: <message>'."""
+    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")