From: Mike Bayer Date: Tue, 7 May 2019 15:38:48 +0000 (-0400) Subject: Turn FlushError for identity already exists into a warning. X-Git-Tag: rel_1_4_0b1~873^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=05bcd4c6eb600b0ab7866183315c05384575db64;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Turn FlushError for identity already exists into a warning. The condition where a pending object being flushed with an identity that already exists in the identity map has been adjusted to emit a warning, rather than throw a :class:`.FlushError`. The rationale is so that the flush will proceed and raise a :class:`.IntegrityError` instead, in the same way as if the existing object were not present in the identity map already. This helps with schemes that are uinsg the :class:`.IntegrityError` as a means of catching whether or not a row already exists in the table. Fixes: #4662 Change-Id: I9314550b7b03d7f376ef35518da7542e0f2f7cb6 --- diff --git a/doc/build/changelog/migration_14.rst b/doc/build/changelog/migration_14.rst index 3b92d9308e..3f8bd3d0c4 100644 --- a/doc/build/changelog/migration_14.rst +++ b/doc/build/changelog/migration_14.rst @@ -6,3 +6,76 @@ What's New in SQLAlchemy 1.4? This document describes changes between SQLAlchemy version 1.3 and SQLAlchemy version 1.4. + + +Behavioral Changes - ORM +======================== + +.. _change_4662: + +The "New instance conflicts with existing identity" error is now a warning +--------------------------------------------------------------------------- + +SQLAlchemy has always had logic to detect when an object in the :class:`.Session` +to be inserted has the same primary key as an object that is already present:: + + class Product(Base): + __tablename__ = 'product' + + id = Column(Integer, primary_key=True) + + session = Session(engine) + + # add Product with primary key 1 + session.add(Product(id=1)) + session.flush() + + # add another Product with same primary key + session.add(Product(id=1)) + s.commit() # <-- will raise FlushError + +The change is that the :class:`.FlushError` is altered to be only a warning:: + + sqlalchemy/orm/persistence.py:408: SAWarning: New instance with identity key (, (1,), None) conflicts with persistent instance + + +Subsequent to that, the condition will attempt to insert the row into the +database which will emit :class:`.IntegrityError`, which is the same error that +would be raised if the primary key identity was not already present in the +:class:`.Session`:: + + sqlalchemy.exc.IntegrityError: (sqlite3.IntegrityError) UNIQUE constraint failed: product.id + +The rationale is to allow code that is using :class:`.IntegrityError` to catch +duplicates to function regardless of the existing state of the +:class:`.Session`, as is often done using savepoints:: + + + # add another Product with same primary key + try: + with session.begin_nested(): + session.add(Product(id=1)) + except exc.IntegrityError: + print("row already exists") + +The above logic was not fully feasible earlier, as in the case that the +``Product`` object with the existing identity were already in the +:class:`.Session`, the code would also have to catch :class:`.FlushError`, +which additionally is not filtered for the specific condition of integrity +issues. With the change, the above block behaves consistently with the +exception of the warning also being emitted. + +Since the logic in question deals with the primary key, all databases emit an +integrity error in the case of primary key conflicts on INSERT. The case +where an error would not be raised, that would have earlier, is the extremely +unusual scenario of a mapping that defines a primary key on the mapped +selectable that is more restrictive than what is actually configured in the +database schema, such as when mapping to joins of tables or when defining +additional columns as part of a composite primary key that is not actually +constrained in the database schema. However, these situations also work more +consistently in that the INSERT would theoretically proceed whether or not the +existing identity were still in the database. The warning can also be +configured to raise an exception using the Python warnings filter. + + +:ticket:`4662` diff --git a/doc/build/changelog/unreleased_14/4662.rst b/doc/build/changelog/unreleased_14/4662.rst new file mode 100644 index 0000000000..b297c9405e --- /dev/null +++ b/doc/build/changelog/unreleased_14/4662.rst @@ -0,0 +1,17 @@ +.. change:: + :tags: change, orm + :tickets: 4662 + + The condition where a pending object being flushed with an identity that + already exists in the identity map has been adjusted to emit a warning, + rather than throw a :class:`.FlushError`. The rationale is so that the + flush will proceed and raise a :class:`.IntegrityError` instead, in the + same way as if the existing object were not present in the identity map + already. This helps with schemes that are uinsg the + :class:`.IntegrityError` as a means of catching whether or not a row + already exists in the table. + + .. seealso:: + + :ref:`change_4662` + diff --git a/lib/sqlalchemy/orm/persistence.py b/lib/sqlalchemy/orm/persistence.py index 6345ee28a5..5c2d411625 100644 --- a/lib/sqlalchemy/orm/persistence.py +++ b/lib/sqlalchemy/orm/persistence.py @@ -402,24 +402,24 @@ def _organize_states_for_save(base_mapper, states, uowtransaction): if not uowtransaction.was_already_deleted(existing): if not uowtransaction.is_deleted(existing): - raise orm_exc.FlushError( + util.warn( "New instance %s with identity key %s conflicts " "with persistent instance %s" % (state_str(state), instance_key, state_str(existing)) ) + else: + base_mapper._log_debug( + "detected row switch for identity %s. " + "will update %s, remove %s from " + "transaction", + instance_key, + state_str(state), + state_str(existing), + ) - base_mapper._log_debug( - "detected row switch for identity %s. " - "will update %s, remove %s from " - "transaction", - instance_key, - state_str(state), - state_str(existing), - ) - - # remove the "delete" flag from the existing element - uowtransaction.remove_state_actions(existing) - row_switch = existing + # remove the "delete" flag from the existing element + uowtransaction.remove_state_actions(existing) + row_switch = existing if (has_identity or row_switch) and mapper.version_id_col is not None: update_version_id = mapper._get_committed_state_attr_by_column( diff --git a/test/orm/test_events.py b/test/orm/test_events.py index d913f3c774..49858c440d 100644 --- a/test/orm/test_events.py +++ b/test/orm/test_events.py @@ -1496,7 +1496,7 @@ class SessionEventsTest(_RemoveListeners, _fixtures.FixtureTest): u2 = User(name="u1", id=1) sess.add(u2) - assert_raises(sa.orm.exc.FlushError, sess.commit) + assert_raises(sa.exc.SAWarning, sess.commit) sess.rollback() eq_( canary, @@ -1548,7 +1548,7 @@ class SessionEventsTest(_RemoveListeners, _fixtures.FixtureTest): u2 = User(name="u1", id=1) sess.add(u2) - assert_raises(sa.orm.exc.FlushError, sess.commit) + assert_raises(sa.exc.SAWarning, sess.commit) sess.rollback() eq_(assertions, [True, True]) diff --git a/test/orm/test_transaction.py b/test/orm/test_transaction.py index 913c223213..367e474e6f 100644 --- a/test/orm/test_transaction.py +++ b/test/orm/test_transaction.py @@ -701,7 +701,9 @@ class SessionTransactionTest(fixtures.RemovesEvents, FixtureTest): sess.commit() sess.add(User(id=1, name="u2")) - assert_raises(orm_exc.FlushError, sess.flush) + + with expect_warnings("New instance"): + assert_raises(sa_exc.IntegrityError, sess.flush) return sess, u1 def test_execution_options_begin_transaction(self): @@ -1232,7 +1234,9 @@ class RollbackRecoverTest(_LocalFixture): u1.name = "edward" a1.email_address = "foober" s.add(u2) - assert_raises(orm_exc.FlushError, s.commit) + + with expect_warnings("New instance"): + assert_raises(sa_exc.IntegrityError, s.commit) assert_raises(sa_exc.InvalidRequestError, s.commit) s.rollback() assert u2 not in s @@ -1271,7 +1275,9 @@ class RollbackRecoverTest(_LocalFixture): a1.email_address = "foober" s.begin_nested() s.add(u2) - assert_raises(orm_exc.FlushError, s.commit) + + with expect_warnings("New instance"): + assert_raises(sa_exc.IntegrityError, s.commit) assert_raises(sa_exc.InvalidRequestError, s.commit) s.rollback() assert u2 not in s @@ -1683,7 +1689,8 @@ class NaturalPKRollbackTest(fixtures.MappedTest): u5 = User(name="u3") session.add(u5) - assert_raises(orm_exc.FlushError, session.flush) + with expect_warnings("New instance"): + assert_raises(sa_exc.IntegrityError, session.flush) assert u5 not in session assert u2 not in session.deleted