From: Mike Bayer Date: Fri, 21 Jan 2022 15:19:02 +0000 (-0500) Subject: Skip PK returned as None for RETURNING, server side default X-Git-Tag: rel_2_0_0b1~520^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8a1931601d3b105ad585ef39840c8251ebdb44a2;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Skip PK returned as None for RETURNING, server side default Fixed regression where the ORM exception that is to be raised when an INSERT silently fails to actually insert a row (such as from a trigger) would not be reached, due to a runtime exception raised ahead of time due to the missing primary key value, thus raising an uninformative exception rather than the correct one. For 1.4 and above, a new ``FlushError`` is added for this case that's raised earlier than the previous "null identity" exception was for 1.3, as a situation where the number of rows actually INSERTed does not match what was expected is a more critical situation in 1.4 as it prevents batching of multiple objects from working correctly. This is separate from the case where a newly fetched primary key is fetched as NULL, which continues to raise the existing "null identity" exception. Fixes: #7594 Change-Id: Ie8e181e3472f09f389cca757c5e58e61b15c7d79 --- diff --git a/doc/build/changelog/unreleased_14/7594.rst b/doc/build/changelog/unreleased_14/7594.rst new file mode 100644 index 0000000000..427bac97e3 --- /dev/null +++ b/doc/build/changelog/unreleased_14/7594.rst @@ -0,0 +1,16 @@ +.. change:: + :tags: bug, orm, regression + :tickets: 7594 + + Fixed regression where the ORM exception that is to be raised when an + INSERT silently fails to actually insert a row (such as from a trigger) + would not be reached, due to a runtime exception raised ahead of time due + to the missing primary key value, thus raising an uninformative exception + rather than the correct one. For 1.4 and above, a new ``FlushError`` is + added for this case that's raised earlier than the previous "null identity" + exception was for 1.3, as a situation where the number of rows actually + INSERTed does not match what was expected is a more critical situation in + 1.4 as it prevents batching of multiple objects from working correctly. + This is separate from the case where a newly fetched primary key is + fetched as NULL, which continues to raise the existing "null identity" + exception. \ No newline at end of file diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py index 6696b34d59..b3381b0390 100644 --- a/lib/sqlalchemy/orm/persistence.py +++ b/lib/sqlalchemy/orm/persistence.py @@ -1177,6 +1177,22 @@ def _emit_insert_statements( c.inserted_primary_key_rows, c.returned_defaults_rows or (), ): + if inserted_primary_key is None: + # this is a real problem and means that we didn't + # get back as many PK rows. we can't continue + # since this indicates PK rows were missing, which + # means we likely mis-populated records starting + # at that point with incorrectly matched PK + # values. + raise orm_exc.FlushError( + "Multi-row INSERT statement for %s did not " + "produce " + "the correct number of INSERTed rows for " + "RETURNING. Ensure there are no triggers or " + "special driver issues preventing INSERT from " + "functioning properly." % mapper_rec + ) + for pk, col in zip( inserted_primary_key, mapper._pks_by_table[table], @@ -1225,6 +1241,15 @@ def _emit_insert_statements( ) primary_key = result.inserted_primary_key + if primary_key is None: + raise orm_exc.FlushError( + "Single-row INSERT statement for %s " + "did not produce a " + "new primary key result " + "being invoked. Ensure there are no triggers or " + "special driver issues preventing INSERT from " + "functioning properly." % (mapper_rec,) + ) for pk, col in zip( primary_key, mapper._pks_by_table[table] ): diff --git a/test/orm/test_unitofwork.py b/test/orm/test_unitofwork.py index c0a347e240..39223a3550 100644 --- a/test/orm/test_unitofwork.py +++ b/test/orm/test_unitofwork.py @@ -25,6 +25,8 @@ from sqlalchemy.testing import assert_raises from sqlalchemy.testing import assert_raises_message from sqlalchemy.testing import eq_ from sqlalchemy.testing import fixtures +from sqlalchemy.testing import is_true +from sqlalchemy.testing.assertions import expect_raises_message from sqlalchemy.testing.assertsql import AllOf from sqlalchemy.testing.assertsql import CompiledSQL from sqlalchemy.testing.assertsql import Conditional @@ -3493,6 +3495,99 @@ class PartialNullPKTest(fixtures.MappedTest): s.commit() +class NoRowInsertedTest(fixtures.TestBase): + """test #7594. + + failure modes when INSERT doesnt actually insert a row. + """ + + __backend__ = True + __requires__ = ("returning",) + + @testing.fixture + def null_server_default_fixture(self, registry, connection): + @registry.mapped + class MyClass(object): + __tablename__ = "my_table" + + id = Column(Integer, primary_key=True) + data = Column(String(50)) + + registry.metadata.create_all(connection) + + @event.listens_for(connection, "before_cursor_execute", retval=True) + def revert_insert( + conn, cursor, statement, parameters, context, executemany + ): + if statement.startswith("INSERT"): + if statement.endswith("RETURNING my_table.id"): + if executemany: + # remove some rows, so the count is wrong + parameters = parameters[0:1] + else: + # statement should return no rows + statement = ( + "UPDATE my_table SET id=NULL WHERE 1!=1 " + "RETURNING my_table.id" + ) + parameters = {} + else: + assert not testing.against( + "postgresql" + ), "this test has to at least run on PostgreSQL" + testing.config.skip_test( + "backend doesn't support the expected form of " + "RETURNING for this test to work" + ) + return statement, parameters + + return MyClass + + def test_insert_single_no_pk_correct_exception( + self, null_server_default_fixture, connection + ): + MyClass = null_server_default_fixture + + sess = fixture_session(bind=connection) + + m1 = MyClass(data="data") + sess.add(m1) + + with expect_raises_message( + orm_exc.FlushError, + "Single-row INSERT statement for .*MyClass.* did not produce", + ): + sess.flush() + + is_true(inspect(m1).transient) + sess.rollback() + is_true(inspect(m1).transient) + + def test_insert_multi_no_pk_correct_exception( + self, null_server_default_fixture, connection + ): + MyClass = null_server_default_fixture + + sess = fixture_session(bind=connection) + + m1, m2, m3 = MyClass(data="d1"), MyClass(data="d2"), MyClass(data="d3") + sess.add_all([m1, m2, m3]) + + is_multi_row = connection.dialect.insert_executemany_returning + with expect_raises_message( + orm_exc.FlushError, + "%s INSERT statement for .*MyClass.* did not produce" + % ("Multi-row" if is_multi_row else "Single-row"), + ): + sess.flush() + + for m in m1, m2, m3: + is_true(inspect(m).transient) + sess.rollback() + for m in m1, m2, m3: + is_true(inspect(m).transient) + + class EnsurePKSortableTest(fixtures.MappedTest): class SomeEnum: # Implements PEP 435 in the minimal fashion needed by SQLAlchemy