From: Mike Bayer Date: Sun, 4 Dec 2011 20:47:13 +0000 (-0500) Subject: -re-document the is_modified method, and place significant caveats X-Git-Tag: rel_0_7_4~28 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=cf41a6a04116f2ca40771aa1d69b81fd67b918f0;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git -re-document the is_modified method, and place significant caveats about the "passive" flag which is now known to be wrong. Add new is_modified tests illustrating the passive flag. [ticket:2320] --- diff --git a/lib/sqlalchemy/orm/__init__.py b/lib/sqlalchemy/orm/__init__.py index db8a596abe..4c6d30d324 100644 --- a/lib/sqlalchemy/orm/__init__.py +++ b/lib/sqlalchemy/orm/__init__.py @@ -667,7 +667,8 @@ def column_property(*cols, **kw): simple non-primary-key scalar values only needs to be aware of the "new" value in order to perform a flush. This flag is available for applications that make use of - :func:`.attributes.get_history` which also need to know + :func:`.attributes.get_history` or :meth:`.Session.is_modified` + which also need to know the "previous" value of the attribute. (new in 0.6.6) :param comparator_factory: a class which extends diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index 4044e83e35..06dc8dcb42 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -1637,29 +1637,41 @@ class Session(object): def is_modified(self, instance, include_collections=True, passive=attributes.PASSIVE_OFF): - """Return ``True`` if instance has modified attributes. + """Return ``True`` if the given instance has locally + modified attributes. - This method retrieves a history instance for each instrumented + This method retrieves the history for each instrumented attribute on the instance and performs a comparison of the current - value to its previously committed value. - - ``include_collections`` indicates if multivalued collections should be - included in the operation. Setting this to False is a way to detect - only local-column based properties (i.e. scalar columns or many-to-one - foreign keys) that would result in an UPDATE for this instance upon - flush. - - The ``passive`` flag indicates if unloaded attributes and collections - should not be loaded in the course of performing this test. - Allowed values include :attr:`.PASSIVE_OFF`, :attr:`.PASSIVE_NO_INITIALIZE`. - + value to its previously committed value, if any. + + It is in effect a more expensive and accurate + version of checking for the given instance in the + :attr:`.Session.dirty` collection; a full test for + each attribute's net "dirty" status is performed. + + E.g.:: + + return session.is_modified(someobject, passive=True) + + .. note:: In SQLAlchemy 0.7 and earlier, the ``passive`` + flag should **always** be explicitly set to ``True``. + The current default value of :data:`.attributes.PASSIVE_OFF` + for this flag is incorrect, in that it loads unloaded + collections and attributes which by definition + have no modified state, and furthermore trips off + autoflush which then causes all subsequent, possibly + modified attributes to lose their modified state. + The default value of the flag will be changed in 0.8. + A few caveats to this method apply: - * Instances present in the 'dirty' collection may result in a value - of ``False`` when tested with this method. This because while - the object may have received attribute set events, there may be - no net changes on its state. - * Scalar attributes may not have recorded the "previously" set + * Instances present in the :attr:`.Session.dirty` collection may report + ``False`` when tested with this method. This is because + the object may have received change events via attribute + mutation, thus placing it in :attr:`.Session.dirty`, + but ultimately the state is the same as that loaded from + the database, resulting in no net change here. + * Scalar attributes may not have recorded the previously set value when a new value was applied, if the attribute was not loaded, or was expired, at the time the new value was received - in these cases, the attribute is assumed to have a change, even if there is @@ -1671,9 +1683,33 @@ class Session(object): expensive on average than issuing a defensive SELECT. The "old" value is fetched unconditionally only if the attribute - container has the "active_history" flag set to ``True``. This flag - is set typically for primary key attributes and scalar references - that are not a simple many-to-one. + container has the ``active_history`` flag set to ``True``. This flag + is set typically for primary key attributes and scalar object references + that are not a simple many-to-one. To set this flag for + any arbitrary mapped column, use the ``active_history`` argument + with :func:`.column_property`. + + :param instance: mapped instance to be tested for pending changes. + :param include_collections: Indicates if multivalued collections should be + included in the operation. Setting this to ``False`` is a way to detect + only local-column based properties (i.e. scalar columns or many-to-one + foreign keys) that would result in an UPDATE for this instance upon + flush. + :param passive: Indicates if unloaded attributes and + collections should be loaded in the course of performing + this test. If set to ``False``, or left at its default + value of :data:`.PASSIVE_OFF`, unloaded attributes + will be loaded. If set to ``True`` or + :data:`.PASSIVE_NO_INITIALIZE`, unloaded + collections and attributes will remain unloaded. As + noted previously, the existence of this flag here + is a bug, as unloaded attributes by definition have + no changes, and the load operation also triggers an + autoflush which then cancels out subsequent changes. + This flag should **always be set to + True**. In 0.8 the flag will be deprecated and the default + set to ``True``. + """ try: @@ -1681,10 +1717,12 @@ class Session(object): except exc.NO_STATE: raise exc.UnmappedInstanceError(instance) dict_ = state.dict + if passive is True: passive = attributes.PASSIVE_NO_INITIALIZE elif passive is False: passive = attributes.PASSIVE_OFF + for attr in state.manager.attributes: if \ ( @@ -1731,6 +1769,10 @@ class Session(object): @property def dirty(self): """The set of all persistent instances considered dirty. + + E.g.:: + + some_mapped_object in session.dirty Instances are considered dirty when they were modified but not deleted. @@ -1745,7 +1787,7 @@ class Session(object): it's only done at flush time). To check if an instance has actionable net changes to its - attributes, use the is_modified() method. + attributes, use the :meth:`.Session.is_modified` method. """ return util.IdentitySet( diff --git a/test/orm/test_session.py b/test/orm/test_session.py index a723e7cfa5..a90e15497b 100644 --- a/test/orm/test_session.py +++ b/test/orm/test_session.py @@ -916,53 +916,6 @@ class SessionTest(_fixtures.FixtureTest): assert user not in s assert s.query(User).count() == 0 - def test_is_modified(self): - User, Address, addresses, users = (self.classes.User, - self.classes.Address, - self.tables.addresses, - self.tables.users) - - s = create_session() - - mapper(User, users, properties={'addresses':relationship(Address)}) - mapper(Address, addresses) - - # save user - u = User(name='fred') - s.add(u) - s.flush() - s.expunge_all() - - user = s.query(User).one() - assert user not in s.dirty - assert not s.is_modified(user) - user.name = 'fred' - assert user in s.dirty - assert not s.is_modified(user) - user.name = 'ed' - assert user in s.dirty - assert s.is_modified(user) - s.flush() - assert user not in s.dirty - assert not s.is_modified(user) - - a = Address() - user.addresses.append(a) - assert user in s.dirty - assert s.is_modified(user) - assert not s.is_modified(user, include_collections=False) - - def test_is_modified_syn(self): - User, users = self.classes.User, self.tables.users - - s = sessionmaker()() - - mapper(User, users, properties={'uname':sa.orm.synonym('name')}) - u = User(uname='fred') - assert s.is_modified(u) - s.add(u) - s.commit() - assert not s.is_modified(u) def test_weak_ref(self): """test the weak-referencing identity map, which strongly- @@ -1368,6 +1321,113 @@ class SessionDataTest(_fixtures.FixtureTest): # pending objects dont get expired assert newad.email_address == 'a new address' +class IsModifiedTest(_fixtures.FixtureTest): + run_inserts = None + + def _default_mapping_fixture(self): + User, Address = self.classes.User, self.classes.Address + users, addresses = self.tables.users, self.tables.addresses + mapper(User, users, properties={ + "addresses":relationship(Address) + }) + mapper(Address, addresses) + return User, Address + + def test_is_modified(self): + User, Address = self._default_mapping_fixture() + + s = create_session() + + # save user + u = User(name='fred') + s.add(u) + s.flush() + s.expunge_all() + + user = s.query(User).one() + assert user not in s.dirty + assert not s.is_modified(user) + user.name = 'fred' + assert user in s.dirty + assert not s.is_modified(user) + user.name = 'ed' + assert user in s.dirty + assert s.is_modified(user) + s.flush() + assert user not in s.dirty + assert not s.is_modified(user) + + a = Address() + user.addresses.append(a) + assert user in s.dirty + assert s.is_modified(user) + assert not s.is_modified(user, include_collections=False) + + def test_is_modified_passive_off(self): + User, Address = self._default_mapping_fixture() + + s = Session() + u = User(name='fred', addresses=[ + Address(email_address='foo')]) + s.add(u) + s.commit() + + u.id + def go(): + assert not s.is_modified(u) + self.assert_sql_count( + testing.db, + go, + 1 + ) + + s.expire_all() + u.name = 'newname' + + # can't predict result here + # deterministically, depending on if + # 'name' or 'addresses' is tested first + mod = s.is_modified(u) + addresses_loaded = 'addresses' in u.__dict__ + assert mod is not addresses_loaded + + def test_is_modified_passive_on(self): + User, Address = self._default_mapping_fixture() + + s = Session() + u = User(name='fred', addresses=[Address(email_address='foo')]) + s.add(u) + s.commit() + + u.id + def go(): + assert not s.is_modified(u, passive=True) + self.assert_sql_count( + testing.db, + go, + 0 + ) + + u.name = 'newname' + def go(): + assert s.is_modified(u, passive=True) + self.assert_sql_count( + testing.db, + go, + 0 + ) + + def test_is_modified_syn(self): + User, users = self.classes.User, self.tables.users + + s = sessionmaker()() + + mapper(User, users, properties={'uname':sa.orm.synonym('name')}) + u = User(uname='fred') + assert s.is_modified(u) + s.add(u) + s.commit() + assert not s.is_modified(u) class DisposedStates(fixtures.MappedTest): run_setup_mappers = 'once'