From: Mike Bayer Date: Mon, 3 Apr 2017 21:25:26 +0000 (-0400) Subject: ResultProxy won't autoclose connection until state flag is set X-Git-Tag: rel_1_2_0b1~118 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9609f5ffb52ce8a4969059e299773ac7176dbb0d;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git ResultProxy won't autoclose connection until state flag is set Changed the mechanics of :class:`.ResultProxy` to unconditionally delay the "autoclose" step until the :class:`.Connection` is done with the object; in the case where Postgresql ON CONFLICT with RETURNING returns no rows, autoclose was occurring in this previously non-existent use case, causing the usual autocommit behavior that occurs unconditionally upon INSERT/UPDATE/DELETE to fail. Change-Id: I235a25daf4381b31f523331f810ea04450349722 Fixes: #3955 (cherry picked from commit 8ee363e4917b0dcd64a83b6d26e465c9e61e0ea5) (cherry picked from commit f52fb5282a046d26b6ee2778e03b995eb117c2ee) --- diff --git a/doc/build/changelog/changelog_11.rst b/doc/build/changelog/changelog_11.rst index 7a53540a5d..3a5b29decb 100644 --- a/doc/build/changelog/changelog_11.rst +++ b/doc/build/changelog/changelog_11.rst @@ -34,6 +34,17 @@ left-hand side type to be transferred directly to the right hand side so that bind-level rules can be applied to the expression's argument. + .. change:: 3955 + :tags: bug, sql, postgresql + :versions: 1.2.0b1 + :tickets: 3955 + + Changed the mechanics of :class:`.ResultProxy` to unconditionally + delay the "autoclose" step until the :class:`.Connection` is done + with the object; in the case where Postgresql ON CONFLICT with + RETURNING returns no rows, autoclose was occurring in this previously + non-existent use case, causing the usual autocommit behavior that + occurs unconditionally upon INSERT/UPDATE/DELETE to fail. .. changelog:: :version: 1.1.8 diff --git a/lib/sqlalchemy/engine/base.py b/lib/sqlalchemy/engine/base.py index f680edadaa..91f4493c2e 100644 --- a/lib/sqlalchemy/engine/base.py +++ b/lib/sqlalchemy/engine/base.py @@ -1203,14 +1203,22 @@ class Connection(Connectable): else: result = context.get_result_proxy() if result._metadata is None: - result._soft_close(_autoclose_connection=False) + result._soft_close() if context.should_autocommit and self._root.__transaction is None: self._root._commit_impl(autocommit=True) - if result._soft_closed and self.should_close_with_result: - self.close() - + # for "connectionless" execution, we have to close this + # Connection after the statement is complete. + if self.should_close_with_result: + # ResultProxy already exhausted rows / has no rows. + # close us now + if result._soft_closed: + self.close() + else: + # ResultProxy will close this Connection when no more + # rows to fetch. + result._autoclose_connection = True return result def _cursor_execute(self, cursor, statement, parameters, context=None): diff --git a/lib/sqlalchemy/engine/default.py b/lib/sqlalchemy/engine/default.py index 1c10f484f6..628e23c9e6 100644 --- a/lib/sqlalchemy/engine/default.py +++ b/lib/sqlalchemy/engine/default.py @@ -939,15 +939,15 @@ class DefaultExecutionContext(interfaces.ExecutionContext): row = result.fetchone() self.returned_defaults = row self._setup_ins_pk_from_implicit_returning(row) - result._soft_close(_autoclose_connection=False) + result._soft_close() result._metadata = None elif not self._is_explicit_returning: - result._soft_close(_autoclose_connection=False) + result._soft_close() result._metadata = None elif self.isupdate and self._is_implicit_returning: row = result.fetchone() self.returned_defaults = row - result._soft_close(_autoclose_connection=False) + result._soft_close() result._metadata = None elif result._metadata is None: @@ -955,7 +955,7 @@ class DefaultExecutionContext(interfaces.ExecutionContext): # (which requires open cursor on some drivers # such as kintersbasdb, mxodbc) result.rowcount - result._soft_close(_autoclose_connection=False) + result._soft_close() return result def _setup_ins_pk_from_lastrowid(self): diff --git a/lib/sqlalchemy/engine/result.py b/lib/sqlalchemy/engine/result.py index 903b2c2f9a..907dc7bd29 100644 --- a/lib/sqlalchemy/engine/result.py +++ b/lib/sqlalchemy/engine/result.py @@ -638,7 +638,7 @@ class ResultProxy(object): _process_row = RowProxy out_parameters = None - _can_close_connection = False + _autoclose_connection = False _metadata = None _soft_closed = False closed = False @@ -792,7 +792,7 @@ class ResultProxy(object): return self._saved_cursor.description - def _soft_close(self, _autoclose_connection=True): + def _soft_close(self): """Soft close this :class:`.ResultProxy`. This releases all DBAPI cursor resources, but leaves the @@ -820,8 +820,7 @@ class ResultProxy(object): self._soft_closed = True cursor = self.cursor self.connection._safe_close_cursor(cursor) - if _autoclose_connection and \ - self.connection.should_close_with_result: + if self._autoclose_connection: self.connection.close() self.cursor = None diff --git a/test/dialect/postgresql/test_on_conflict.py b/test/dialect/postgresql/test_on_conflict.py index 7c83f28262..c3e1b91584 100644 --- a/test/dialect/postgresql/test_on_conflict.py +++ b/test/dialect/postgresql/test_on_conflict.py @@ -12,6 +12,7 @@ class OnConflictTest(fixtures.TablesTest): __only_on__ = 'postgresql >= 9.5', __backend__ = True + run_define_tables = 'each' @classmethod def define_tables(cls, metadata): @@ -79,6 +80,7 @@ class OnConflictTest(fixtures.TablesTest): with testing.db.connect() as conn: result = conn.execute( insert(users).on_conflict_do_nothing(), + dict(id=1, name='name1') ) eq_(result.inserted_primary_key, [1]) @@ -96,6 +98,33 @@ class OnConflictTest(fixtures.TablesTest): [(1, 'name1')] ) + def test_on_conflict_do_nothing_connectionless(self): + users = self.tables.users_xtra + + with testing.db.connect() as conn: + result = conn.execute( + insert(users).on_conflict_do_nothing( + constraint='uq_login_email'), + + dict(name='name1', login_email='email1') + ) + eq_(result.inserted_primary_key, [1]) + eq_(result.returned_defaults, (1,)) + + result = testing.db.execute( + insert(users).on_conflict_do_nothing( + constraint='uq_login_email' + ), + dict(name='name2', login_email='email1') + ) + eq_(result.inserted_primary_key, None) + eq_(result.returned_defaults, None) + + eq_( + testing.db.execute(users.select().where(users.c.id == 1)).fetchall(), + [(1, 'name1', 'email1', None)] + ) + @testing.provide_metadata def test_on_conflict_do_nothing_target(self): users = self.tables.users diff --git a/test/sql/test_resultset.py b/test/sql/test_resultset.py index de677be9a4..48fe288613 100644 --- a/test/sql/test_resultset.py +++ b/test/sql/test_resultset.py @@ -489,6 +489,50 @@ class ResultProxyTest(fixtures.TablesTest): result.fetchone ) + def test_connectionless_autoclose_rows_exhausted(self): + users = self.tables.users + users.insert().execute( + dict(user_id=1, user_name='john'), + ) + + result = testing.db.execute("select * from users") + connection = result.connection + assert not connection.closed + eq_(result.fetchone(), (1, 'john')) + assert not connection.closed + eq_(result.fetchone(), None) + assert connection.closed + + @testing.requires.returning + def test_connectionless_autoclose_crud_rows_exhausted(self): + users = self.tables.users + stmt = users.insert().values(user_id=1, user_name='john').\ + returning(users.c.user_id) + result = testing.db.execute(stmt) + connection = result.connection + assert not connection.closed + eq_(result.fetchone(), (1, )) + assert not connection.closed + eq_(result.fetchone(), None) + assert connection.closed + + def test_connectionless_autoclose_no_rows(self): + result = testing.db.execute("select * from users") + connection = result.connection + assert not connection.closed + eq_(result.fetchone(), None) + assert connection.closed + + def test_connectionless_autoclose_no_metadata(self): + result = testing.db.execute("update users set user_id=5") + connection = result.connection + assert connection.closed + assert_raises_message( + exc.ResourceClosedError, + "This result object does not return rows.", + result.fetchone + ) + def test_row_case_sensitive(self): row = testing.db.execute( select([