merge with object has positive None set on Pk attributes keys on None

[Haim Raitsev]

When trying to update an object with nested objects, merge will only insert the last one of the nested objects.

This is the code we ran with the following input:

modal:

class Order(Base):
    __tablename__ = 'orders'
    id = Column(Integer, primary_key=True)
    status = Column(Enum(OrderStatus), nullable=False)
    suppression_files = relationship('File', lazy='joined', cascade="all, delete-orphan")

class File(Base):
    __tablename__ = 'files'
    id = Column(Integer, primary_key=True)
    order_id = Column(Integer, ForeignKey('orders.id'), nullable=False)
    file_name = Column(String, nullable=False)
    file_url = Column(String, nullable=False)

The object we are trying to merge:

id = {int} 59
status = {OrderStatus} OrderStatus.draft
suppression_files = {InstrumentedList} [
{File} 
file_name = {str} 'test'
file_url = {str} 'https://docs.sqlalchemy.org/en/rel_1_1/contents.html'
id = {NoneType} None
order_id = {NoneType} None, 
{File} 
file_name = {str} 'test1'
file_url = {str} 'https://docs.sqlalchemy.org/en/rel_1_1/contents.html'
id = {NoneType} None
order_id = {NoneType} None]

Expected: All 2 new files in the array('suppression_files') should be created

Actual: Only the last file (named 'test1') will be inserted to the db, the first file is ignored.

Database: PostgresSQL 9.6 SQLAlchemy:1.1.13

[Mike Bayer]

Hi there -

I would need MUCH more detail, specifically a complete example as in MCVE. Please let me see an example Order and File structure and how you are populating them that reproduces the issue, as it sounds like the File objects probably have the wrong state (like dupe primary keys or something).

[Mike Bayer]

here is an MCVE showing what you describe as working:

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base

Base = declarative_base()

class Order(Base):
    __tablename__ = 'orders'
    id = Column(Integer, primary_key=True)
    status = Column(String, nullable=False)
    suppression_files = relationship('File', lazy='joined', cascade="all, delete-orphan")

class File(Base):
    __tablename__ = 'files'
    id = Column(Integer, primary_key=True)
    order_id = Column(Integer, ForeignKey('orders.id'), nullable=False)
    file_name = Column(String, nullable=False)
    file_url = Column(String, nullable=False)

e = create_engine("sqlite://", echo=True)
Base.metadata.create_all(e)

s = Session(e)

order = Order(
    id = 59,
    status = "draft",
    suppression_files=[
        File(file_name="foo1", file_url="bar1"),
        File(file_name="foo2", file_url="bar2")
    ]
)

order = s.merge(order)

s.commit()

assert s.query(func.count(File.id)).join(Order, File.order_id == Order.id).scalar() == 2

[Haim Raitsev]

We found the problem in our code we write the order object as

order = Order(
    id = 59,
    status = "draft",
    suppression_files=[
        File(id=None, file_name="foo1", file_url="bar1"),
        File(id=None, file_name="foo2", file_url="bar2")
    ]
)

and after merge the files are merged into one file with the same attributes, we omit the id from the order and its worked well but we still think that primary key cannot be null and it still bug if we set primary key equals to None it still has to work.

[Mike Bayer]

OK now that's more like a bug, thanks

test:

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base

Base = declarative_base()

class A(Base):
    __tablename__ = 'a'
    id = Column(Integer, primary_key=True)
    bs = relationship("B")

class B(Base):
    __tablename__ = 'b'
    id = Column(Integer, primary_key=True)
    a_id = Column(ForeignKey('a.id'))

e = create_engine("sqlite://", echo=True)
Base.metadata.create_all(e)

s = Session(e)

a1 = s.merge(A(bs=[B(id=None), B(id=None)]))

print a1.bs

s.commit()

print a1.bs

the key of (None, ) is treated as persistent which it should not be, this is likely safe for 1.1.x:

diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index 6ecef17f1..5488c3031 100644
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -1912,7 +1912,10 @@ class Session(_SessionClassMethods):
                     "all changes on mapped instances before merging with "
                     "load=False.")
             key = mapper._identity_key_from_state(state)
-            key_is_persistent = attributes.NEVER_SET not in key[1]
+            key_is_persistent = attributes.NEVER_SET not in key[1] and (
+                not _none_set.intersection(key[1]) or
+                (mapper.allow_partial_pks and not _none_set.issuperset(key[1]))
+            )
         else:
             key_is_persistent = True

@@ -1933,10 +1936,7 @@ class Session(_SessionClassMethods):
             self._update_impl(merged_state)
             new_instance = True

-        elif key_is_persistent and (
-            not _none_set.intersection(key[1]) or
-            (mapper.allow_partial_pks and
-             not _none_set.issuperset(key[1]))):
+        elif key_is_persistent:
             merged = self.query(mapper.class_).get(key[1])
         else:
             merged = None

[Haim Raitsev]

Thanks alot :)

[Mike Bayer]

Consider merge key with (None, ) as non-persistent

Fixed bug in :meth:.Session.merge where objects in a collection that had the primary key attribute set to None for a key that is typically autoincrementing would be considered to be a database-persisted key for part of the internal deduplication process, causing only one object to actually be inserted in the database.

Change-Id: I0a6e00043be0b2979cda33740e1be3b430ecf8c7 Fixes: #4056 (cherry picked from commit 5243341ed886e10a0d3f7fef8ae3d071e0ffdcf0)

→ <<cset 3feea4503ff2>>

[Mike Bayer]

Consider merge key with (None, ) as non-persistent

Fixed bug in :meth:.Session.merge where objects in a collection that had the primary key attribute set to None for a key that is typically autoincrementing would be considered to be a database-persisted key for part of the internal deduplication process, causing only one object to actually be inserted in the database.

Change-Id: I0a6e00043be0b2979cda33740e1be3b430ecf8c7 Fixes: #4056

→ <<cset 5243341ed886>>