changing primary key value on expired instance fails to load the old PK version

[Mike Bayer]

from sqlalchemy import create_engine, Table, Column, Integer, String, MetaData
from sqlalchemy.orm import mapper, sessionmaker
engine = create_engine("sqlite:///:memory:",echo=True)

class User(object):
    def __init__(self,name="anonymous"):
        self.id = 2
        self.name = name

    def __repr__(self):
        return "<User ID : %s, Name : %s>" % (repr(self.id),self.name)

metadata = MetaData()
usertable = Table('users',metadata,
                  Column('id',Integer,primary_key=True),
                  Column('name',String(50))
                  )

metadata.create_all(engine)

mapper(User,usertable)
Session = sessionmaker(bind=engine,autocommit=False,autoflush=False) #expireoncommit is True
session = Session()

u = User("One")
session.add(u)
session.commit()
#session.refresh(u)
u.id = 1
session.add(u)
session.commit()

[Mike Bayer]

Here's the fix. The approach is to switch off the behavior of attributes such that it doesn't actively load the "old" version of a scalar attribute when it changes; we need it to actively load in some cases. For starters, we turn the flag on for primary key based scalar attributes. In the future we can also add ForeignKeys and columns that are marked as a primary key at the mapper level only. This fix requires a functional test against the above test case, as well as attribute-level unit tests.

Index: lib/sqlalchemy/orm/strategies.py
===================================================================
--- lib/sqlalchemy/orm/strategies.py    (revision 5050)
+++ lib/sqlalchemy/orm/strategies.py    (working copy)
@@ -20,7 +20,7 @@


 class DefaultColumnLoader(LoaderStrategy):
-    def _register_attribute(self, compare_function, copy_function, mutable_scalars, comparator_factory, callable_=None, proxy_property=None):
+    def _register_attribute(self, compare_function, copy_function, mutable_scalars, comparator_factory, callable_=None, proxy_property=None, active_history=False):
         self.logger.info("%s register managed attribute" % self)

         for mapper in self.parent.polymorphic_iterator():
@@ -36,7 +36,8 @@
                     comparator=comparator_factory(self.parent_property, mapper), 
                     parententity=mapper,
                     callable_=callable_,
-                    proxy_property=proxy_property
+                    proxy_property=proxy_property,
+                    active_history=active_history
                     )

 DefaultColumnLoader.logger = log.class_logger(DefaultColumnLoader)
@@ -57,12 +58,15 @@
     def init_class_attribute(self):
         self.is_class_level = True
         coltype = self.columns[0](0).type
+        active_history = self.columns[0](0).primary_key  # TODO: check all columns ?  check for foreign Key as well?

         self._register_attribute(
             coltype.compare_values,
             coltype.copy_value,
             self.columns[0](0).type.is_mutable(),
-            self.parent_property.comparator_factory
+            self.parent_property.comparator_factory,
+            active_history = active_history
+            
        )

     def create_row_processor(self, selectcontext, path, mapper, row, adapter):
Index: lib/sqlalchemy/orm/attributes.py
===================================================================
--- lib/sqlalchemy/orm/attributes.py    (revision 5050)
+++ lib/sqlalchemy/orm/attributes.py    (working copy)
@@ -197,7 +197,7 @@
 class AttributeImpl(object):
     """internal implementation for instrumented attributes."""

-    def __init__(self, class_, key, callable_, class_manager, trackparent=False, extension=None, compare_function=None, **kwargs):
+    def __init__(self, class_, key, callable_, class_manager, trackparent=False, extension=None, compare_function=None, active_history=False, **kwargs):
         """Construct an AttributeImpl.

         \class_
@@ -231,6 +231,7 @@
         self.callable_ = callable_
         self.class_manager = class_manager
         self.trackparent = trackparent
+        self.active_history = active_history
         if compare_function is None:
             self.is_equal = operator.eq
         else:
@@ -364,11 +365,16 @@
     uses_objects = False

     def delete(self, state):
-        state.modified_event(self, False, state.dict.get(self.key, NO_VALUE))

         # TODO: catch key errors, convert to attributeerror?
+        if self.active_history or self.extensions:
+            old = self.get(state)
+        else:
+            old = state.dict.get(self.key, NO_VALUE)
+
+        state.modified_event(self, False, old)
+
         if self.extensions:
-            old = self.get(state)
             del state.dict[self.key](self.key)
             self.fire_remove_event(state, old, None)
         else:
@@ -382,10 +388,14 @@
         if initiator is self:
             return

-        state.modified_event(self, False, state.dict.get(self.key, NO_VALUE))
+        if self.active_history or self.extensions:
+            old = self.get(state)
+        else:
+            old = state.dict.get(self.key, NO_VALUE)
+            
+        state.modified_event(self, False, old)

         if self.extensions:
-            old = self.get(state)
             state.dict[self.key](self.key) = value
             self.fire_replace_event(state, value, old, initiator)
         else:

[Mike Bayer]

the patch itself is in c03b6c2e41f309b122e300b2625a9f744ad690b8. tests and CHANGES are still required.

[Mike Bayer]

tests in 24ee97c6100988a2ef80bd18d70418eb76070e4d

[Mike Bayer]

Removing milestone: 0.5.0 (automated comment)