From 5c9fec286ed5388ebe4bdc9dda5cd033e437c7ce Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Sun, 19 Jul 2009 20:06:07 +0000 Subject: [PATCH] - the "dont_load=True" flag on Session.merge() is deprecated and is now "load=False". - changed two internal "no_"/"dont_" arguments to positive names --- 06CHANGES | 4 +- doc/build/session.rst | 2 +- examples/query_caching/query_caching.py | 2 +- lib/sqlalchemy/orm/attributes.py | 10 ++-- lib/sqlalchemy/orm/interfaces.py | 2 +- lib/sqlalchemy/orm/mapper.py | 6 +- lib/sqlalchemy/orm/properties.py | 24 ++++---- lib/sqlalchemy/orm/session.py | 26 +++++---- lib/sqlalchemy/orm/state.py | 2 +- lib/sqlalchemy/orm/strategies.py | 2 +- test/orm/test_merge.py | 78 +++++++++++++++---------- test/orm/test_pickled.py | 4 +- 12 files changed, 93 insertions(+), 69 deletions(-) diff --git a/06CHANGES b/06CHANGES index 3045721cba..ea75565519 100644 --- a/06CHANGES +++ b/06CHANGES @@ -7,7 +7,9 @@ There is no implicit fallback onto "fetch". Failure of evaluation is based on the structure of criteria, so success/failure is deterministic based on code structure. - + - the "dont_load=True" flag on Session.merge() is deprecated and is now + "load=False". + - sql - returning() support is native to insert(), update(), delete(). Implementations of varying levels of functionality exist for Postgresql, Firebird, MSSQL and diff --git a/doc/build/session.rst b/doc/build/session.rst index ed334c0993..c704dc7928 100644 --- a/doc/build/session.rst +++ b/doc/build/session.rst @@ -219,7 +219,7 @@ With ``merge()``, the given instance is not placed within the session, and can b * An application which reads an object structure from a file and wishes to save it to the database might parse the file, build up the structure, and then use ``merge()`` to save it to the database, ensuring that the data within the file is used to formulate the primary key of each element of the structure. Later, when the file has changed, the same process can be re-run, producing a slightly different object structure, which can then be ``merged()`` in again, and the ``Session`` will automatically update the database to reflect those changes. * A web application stores mapped entities within an HTTP session object. When each request starts up, the serialized data can be merged into the session, so that the original entity may be safely shared among requests and threads. -``merge()`` is frequently used by applications which implement their own second level caches. This refers to an application which uses an in memory dictionary, or an tool like Memcached to store objects over long running spans of time. When such an object needs to exist within a ``Session``, ``merge()`` is a good choice since it leaves the original cached object untouched. For this use case, merge provides a keyword option called ``dont_load=True``. When this boolean flag is set to ``True``, ``merge()`` will not issue any SQL to reconcile the given object against the current state of the database, thereby reducing query overhead. The limitation is that the given object and all of its children may not contain any pending changes, and it's also of course possible that newer information in the database will not be present on the merged object, since no load is issued. +``merge()`` is frequently used by applications which implement their own second level caches. This refers to an application which uses an in memory dictionary, or an tool like Memcached to store objects over long running spans of time. When such an object needs to exist within a ``Session``, ``merge()`` is a good choice since it leaves the original cached object untouched. For this use case, merge provides a keyword option called ``load=False``. When this boolean flag is set to ``False``, ``merge()`` will not issue any SQL to reconcile the given object against the current state of the database, thereby reducing query overhead. The limitation is that the given object and all of its children may not contain any pending changes, and it's also of course possible that newer information in the database will not be present on the merged object, since no load is issued. Deleting -------- diff --git a/examples/query_caching/query_caching.py b/examples/query_caching/query_caching.py index 92d48e2d78..00a4cc3ec2 100644 --- a/examples/query_caching/query_caching.py +++ b/examples/query_caching/query_caching.py @@ -27,7 +27,7 @@ class CachingQuery(Query): self.session.expunge(x) _cache[self.cachekey] = ret - return iter(self.session.merge(x, dont_load=True) for x in ret) + return iter(self.session.merge(x, load=False) for x in ret) else: return Query.__iter__(self) diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py index 97d0e42e91..09a05b56fe 100644 --- a/lib/sqlalchemy/orm/attributes.py +++ b/lib/sqlalchemy/orm/attributes.py @@ -159,7 +159,7 @@ class InstrumentedAttribute(QueryableAttribute): class _ProxyImpl(object): accepts_scalar_loader = False - dont_expire_missing = False + expire_missing = True def __init__(self, key): self.key = key @@ -231,7 +231,7 @@ class AttributeImpl(object): def __init__(self, class_, key, callable_, trackparent=False, extension=None, compare_function=None, active_history=False, parent_token=None, - dont_expire_missing=False, + expire_missing=True, **kwargs): """Construct an AttributeImpl. @@ -270,8 +270,8 @@ class AttributeImpl(object): Allows multiple AttributeImpls to all match a single owner attribute. - dont_expire_missing - if True, don't add an "expiry" callable to this attribute + expire_missing + if False, don't add an "expiry" callable to this attribute during state.expire_attributes(None), if no value is present for this key. @@ -287,7 +287,7 @@ class AttributeImpl(object): self.is_equal = compare_function self.extensions = util.to_list(extension or []) self.active_history = active_history - self.dont_expire_missing = dont_expire_missing + self.expire_missing = expire_missing def hasparent(self, state, optimistic=False): """Return the boolean value of a `hasparent` flag attached to the given item. diff --git a/lib/sqlalchemy/orm/interfaces.py b/lib/sqlalchemy/orm/interfaces.py index 9a9ebfcab2..c590f4323f 100644 --- a/lib/sqlalchemy/orm/interfaces.py +++ b/lib/sqlalchemy/orm/interfaces.py @@ -455,7 +455,7 @@ class MapperProperty(object): return not self.parent.non_primary - def merge(self, session, source, dest, dont_load, _recursive): + def merge(self, session, source, dest, load, _recursive): """Merge the attribute represented by this ``MapperProperty`` from source to destination object""" diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index 1502060f02..6417397b88 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -506,13 +506,13 @@ class Mapper(object): if self.polymorphic_on and self.polymorphic_on not in self._columntoproperty: col = self.mapped_table.corresponding_column(self.polymorphic_on) if not col: - dont_instrument = True + instrument = False col = self.polymorphic_on else: - dont_instrument = False + instrument = True if self._should_exclude(col.key, col.key, local=False): raise sa_exc.InvalidRequestError("Cannot exclude or override the discriminator column %r" % col.key) - self._configure_property(col.key, ColumnProperty(col, _no_instrument=dont_instrument), init=False, setparent=True) + self._configure_property(col.key, ColumnProperty(col, _instrument=instrument), init=False, setparent=True) def _adapt_inherited_property(self, key, prop, init): if not self.concrete: diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py index 0fa32f73f6..3489d81f2e 100644 --- a/lib/sqlalchemy/orm/properties.py +++ b/lib/sqlalchemy/orm/properties.py @@ -53,7 +53,7 @@ class ColumnProperty(StrategizedProperty): self.columns = [expression._labeled(c) for c in columns] self.group = kwargs.pop('group', None) self.deferred = kwargs.pop('deferred', False) - self.no_instrument = kwargs.pop('_no_instrument', False) + self.instrument = kwargs.pop('_instrument', True) self.comparator_factory = kwargs.pop('comparator_factory', self.__class__.Comparator) self.descriptor = kwargs.pop('descriptor', None) self.extension = kwargs.pop('extension', None) @@ -63,7 +63,7 @@ class ColumnProperty(StrategizedProperty): self.__class__.__name__, ', '.join(sorted(kwargs.keys())))) util.set_creation_order(self) - if self.no_instrument: + if not self.instrument: self.strategy_class = strategies.UninstrumentedColumnLoader elif self.deferred: self.strategy_class = strategies.DeferredColumnLoader @@ -71,7 +71,7 @@ class ColumnProperty(StrategizedProperty): self.strategy_class = strategies.ColumnLoader def instrument_class(self, mapper): - if self.no_instrument: + if not self.instrument: return attributes.register_descriptor( @@ -104,7 +104,7 @@ class ColumnProperty(StrategizedProperty): def setattr(self, state, value, column): state.get_impl(self.key).set(state, state.dict, value, None) - def merge(self, session, source, dest, dont_load, _recursive): + def merge(self, session, source, dest, load, _recursive): value = attributes.instance_state(source).value_as_iterable( self.key, passive=True) if value: @@ -302,7 +302,7 @@ class SynonymProperty(MapperProperty): proxy_property=self.descriptor ) - def merge(self, session, source, dest, dont_load, _recursive): + def merge(self, session, source, dest, load, _recursive): pass log.class_logger(SynonymProperty) @@ -335,7 +335,7 @@ class ComparableProperty(MapperProperty): def create_row_processor(self, selectcontext, path, mapper, row, adapter): return (None, None) - def merge(self, session, source, dest, dont_load, _recursive): + def merge(self, session, source, dest, load, _recursive): pass @@ -627,8 +627,8 @@ class RelationProperty(StrategizedProperty): def __str__(self): return str(self.parent.class_.__name__) + "." + self.key - def merge(self, session, source, dest, dont_load, _recursive): - if not dont_load: + def merge(self, session, source, dest, load, _recursive): + if load: # TODO: no test coverage for recursive check for r in self._reverse_property: if (source, r) in _recursive: @@ -650,10 +650,10 @@ class RelationProperty(StrategizedProperty): dest_list = [] for current in instances: _recursive[(current, self)] = True - obj = session._merge(current, dont_load=dont_load, _recursive=_recursive) + obj = session._merge(current, load=load, _recursive=_recursive) if obj is not None: dest_list.append(obj) - if dont_load: + if not load: coll = attributes.init_collection(dest_state, self.key) for c in dest_list: coll.append_without_event(c) @@ -663,9 +663,9 @@ class RelationProperty(StrategizedProperty): current = instances[0] if current is not None: _recursive[(current, self)] = True - obj = session._merge(current, dont_load=dont_load, _recursive=_recursive) + obj = session._merge(current, load=load, _recursive=_recursive) if obj is not None: - if dont_load: + if not load: dest_state.dict[self.key] = obj else: setattr(dest, self.key, obj) diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index 5b83df4ac6..ea5aae645e 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -1139,7 +1139,7 @@ class Session(object): for state, m, o in cascade_states: self._delete_impl(state) - def merge(self, instance, dont_load=False): + def merge(self, instance, load=True, **kw): """Copy the state an instance onto the persistent instance with the same identifier. If there is no persistent instance currently associated with the @@ -1152,6 +1152,10 @@ class Session(object): mapped with ``cascade="merge"``. """ + if 'dont_load' in kw: + load = not kw['dont_load'] + util.warn_deprecated("dont_load=True has been renamed to load=False.") + # TODO: this should be an IdentityDict for instances, but will # need a separate dict for PropertyLoader tuples _recursive = {} @@ -1159,11 +1163,11 @@ class Session(object): autoflush = self.autoflush try: self.autoflush = False - return self._merge(instance, dont_load=dont_load, _recursive=_recursive) + return self._merge(instance, load=load, _recursive=_recursive) finally: self.autoflush = autoflush - def _merge(self, instance, dont_load=False, _recursive=None): + def _merge(self, instance, load=True, _recursive=None): mapper = _object_mapper(instance) if instance in _recursive: return _recursive[instance] @@ -1173,24 +1177,24 @@ class Session(object): key = state.key if key is None: - if dont_load: + if not load: raise sa_exc.InvalidRequestError( - "merge() with dont_load=True option does not support " + "merge() with load=False option does not support " "objects transient (i.e. unpersisted) objects. flush() " "all changes on mapped instances before merging with " - "dont_load=True.") + "load=False.") key = mapper._identity_key_from_state(state) merged = None if key: if key in self.identity_map: merged = self.identity_map[key] - elif dont_load: + elif not load: if state.modified: raise sa_exc.InvalidRequestError( - "merge() with dont_load=True option does not support " + "merge() with load=False option does not support " "objects marked as 'dirty'. flush() all changes on " - "mapped instances before merging with dont_load=True.") + "mapped instances before merging with load=False.") merged = mapper.class_manager.new_instance() merged_state = attributes.instance_state(merged) merged_state.key = key @@ -1208,9 +1212,9 @@ class Session(object): _recursive[instance] = merged for prop in mapper.iterate_properties: - prop.merge(self, instance, merged, dont_load, _recursive) + prop.merge(self, instance, merged, load, _recursive) - if dont_load: + if not load: attributes.instance_state(merged).commit_all(attributes.instance_dict(merged), self.identity_map) # remove any history if new_instance: diff --git a/lib/sqlalchemy/orm/state.py b/lib/sqlalchemy/orm/state.py index 10a0f43eeb..f548d073b7 100644 --- a/lib/sqlalchemy/orm/state.py +++ b/lib/sqlalchemy/orm/state.py @@ -230,7 +230,7 @@ class InstanceState(object): for key in attribute_names: impl = self.manager[key].impl if not filter_deferred or \ - not impl.dont_expire_missing or \ + impl.expire_missing or \ key in dict_: self.expired_attributes.add(key) if impl.accepts_scalar_loader: diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index ebb576a716..d2ab214666 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -230,7 +230,7 @@ class DeferredColumnLoader(LoaderStrategy): copy_function=self.columns[0].type.copy_value, mutable_scalars=self.columns[0].type.is_mutable(), callable_=self._class_level_loader, - dont_expire_missing=True + expire_missing=False ) def setup_query(self, context, entity, path, adapter, only_load_props=None, **kwargs): diff --git a/test/orm/test_merge.py b/test/orm/test_merge.py index 29de21241f..5433515caa 100644 --- a/test/orm/test_merge.py +++ b/test/orm/test_merge.py @@ -302,20 +302,20 @@ class MergeTest(_fixtures.FixtureTest): # test with "dontload" merge sess5 = create_session() - u = sess5.merge(u, dont_load=True) + u = sess5.merge(u, load=False) assert len(u.addresses) for a in u.addresses: assert a.user is u def go(): sess5.flush() # no changes; therefore flush should do nothing - # but also, dont_load wipes out any difference in committed state, + # but also, load=False wipes out any difference in committed state, # so no flush at all self.assert_sql_count(testing.db, go, 0) eq_(on_load.called, 15) sess4 = create_session() - u = sess4.merge(u, dont_load=True) + u = sess4.merge(u, load=False) # post merge change u.addresses[1].email_address='afafds' def go(): @@ -447,17 +447,35 @@ class MergeTest(_fixtures.FixtureTest): assert u3 is u @testing.resolve_artifact_names - def test_transient_dontload(self): + def test_transient_no_load(self): mapper(User, users) sess = create_session() u = User() - assert_raises_message(sa.exc.InvalidRequestError, "dont_load=True option does not support", sess.merge, u, dont_load=True) + assert_raises_message(sa.exc.InvalidRequestError, "load=False option does not support", sess.merge, u, load=False) + @testing.resolve_artifact_names + def test_dont_load_deprecated(self): + mapper(User, users) + + sess = create_session() + u = User(name='ed') + sess.add(u) + sess.flush() + u = sess.query(User).first() + sess.expunge(u) + sess.execute(users.update().values(name='jack')) + @testing.uses_deprecated("dont_load=True has been renamed") + def go(): + u1 = sess.merge(u, dont_load=True) + assert u1 in sess + assert u1.name=='ed' + assert u1 not in sess.dirty + go() @testing.resolve_artifact_names - def test_dontload_with_backrefs(self): - """dontload populates relations in both directions without requiring a load""" + def test_no_load_with_backrefs(self): + """load=False populates relations in both directions without requiring a load""" mapper(User, users, properties={ 'addresses':relation(mapper(Address, addresses), backref='user') }) @@ -472,7 +490,7 @@ class MergeTest(_fixtures.FixtureTest): assert 'user' in u.addresses[1].__dict__ sess = create_session() - u2 = sess.merge(u, dont_load=True) + u2 = sess.merge(u, load=False) assert 'user' in u2.addresses[1].__dict__ eq_(u2.addresses[1].user, User(id=7, name='fred')) @@ -481,7 +499,7 @@ class MergeTest(_fixtures.FixtureTest): sess.close() sess = create_session() - u = sess.merge(u2, dont_load=True) + u = sess.merge(u2, load=False) assert 'user' not in u.addresses[1].__dict__ eq_(u.addresses[1].user, User(id=7, name='fred')) @@ -490,12 +508,12 @@ class MergeTest(_fixtures.FixtureTest): def test_dontload_with_eager(self): """ - This test illustrates that with dont_load=True, we can't just copy the + This test illustrates that with load=False, we can't just copy the committed_state of the merged instance over; since it references collection objects which themselves are to be merged. This committed_state would instead need to be piecemeal 'converted' to represent the correct objects. However, at the moment I'd rather not - support this use case; if you are merging with dont_load=True, you're + support this use case; if you are merging with load=False, you're typically dealing with caching and the merged objects shouldnt be 'dirty'. @@ -518,16 +536,16 @@ class MergeTest(_fixtures.FixtureTest): u2 = sess2.query(User).options(sa.orm.eagerload('addresses')).get(7) sess3 = create_session() - u3 = sess3.merge(u2, dont_load=True) + u3 = sess3.merge(u2, load=False) def go(): sess3.flush() self.assert_sql_count(testing.db, go, 0) @testing.resolve_artifact_names - def test_dont_load_disallows_dirty(self): - """dont_load doesnt support 'dirty' objects right now + def test_no_load_disallows_dirty(self): + """load=False doesnt support 'dirty' objects right now - (see test_dont_load_with_eager()). Therefore lets assert it. + (see test_no_load_with_eager()). Therefore lets assert it. """ mapper(User, users) @@ -541,17 +559,17 @@ class MergeTest(_fixtures.FixtureTest): u.name = 'ed' sess2 = create_session() try: - sess2.merge(u, dont_load=True) + sess2.merge(u, load=False) assert False except sa.exc.InvalidRequestError, e: - assert ("merge() with dont_load=True option does not support " + assert ("merge() with load=False option does not support " "objects marked as 'dirty'. flush() all changes on mapped " - "instances before merging with dont_load=True.") in str(e) + "instances before merging with load=False.") in str(e) u2 = sess2.query(User).get(7) sess3 = create_session() - u3 = sess3.merge(u2, dont_load=True) + u3 = sess3.merge(u2, load=False) assert not sess3.dirty def go(): sess3.flush() @@ -559,7 +577,7 @@ class MergeTest(_fixtures.FixtureTest): @testing.resolve_artifact_names - def test_dont_load_sets_backrefs(self): + def test_no_load_sets_backrefs(self): mapper(User, users, properties={ 'addresses':relation(mapper(Address, addresses),backref='user')}) @@ -577,17 +595,17 @@ class MergeTest(_fixtures.FixtureTest): assert u.addresses[0].user is u sess2 = create_session() - u2 = sess2.merge(u, dont_load=True) + u2 = sess2.merge(u, load=False) assert not sess2.dirty def go(): assert u2.addresses[0].user is u2 self.assert_sql_count(testing.db, go, 0) @testing.resolve_artifact_names - def test_dont_load_preserves_parents(self): - """Merge with dont_load does not trigger a 'delete-orphan' operation. + def test_no_load_preserves_parents(self): + """Merge with load=False does not trigger a 'delete-orphan' operation. - merge with dont_load sets attributes without using events. this means + merge with load=False sets attributes without using events. this means the 'hasparent' flag is not propagated to the newly merged instance. in fact this works out OK, because the '_state.parents' collection on the newly merged instance is empty; since the mapper doesn't see an @@ -612,7 +630,7 @@ class MergeTest(_fixtures.FixtureTest): assert u.addresses[0].user is u sess2 = create_session() - u2 = sess2.merge(u, dont_load=True) + u2 = sess2.merge(u, load=False) assert not sess2.dirty a2 = u2.addresses[0] a2.email_address='somenewaddress' @@ -626,19 +644,19 @@ class MergeTest(_fixtures.FixtureTest): # this use case is not supported; this is with a pending Address on # the pre-merged object, and we currently dont support 'dirty' objects - # being merged with dont_load=True. in this case, the empty + # being merged with load=False. in this case, the empty # '_state.parents' collection would be an issue, since the optimistic # flag is False in _is_orphan() for pending instances. so if we start - # supporting 'dirty' with dont_load=True, this test will need to pass + # supporting 'dirty' with load=False, this test will need to pass sess = create_session() u = sess.query(User).get(7) u.addresses.append(Address()) sess2 = create_session() try: - u2 = sess2.merge(u, dont_load=True) + u2 = sess2.merge(u, load=False) assert False - # if dont_load is changed to support dirty objects, this code + # if load=False is changed to support dirty objects, this code # needs to pass a2 = u2.addresses[0] a2.email_address='somenewaddress' @@ -649,7 +667,7 @@ class MergeTest(_fixtures.FixtureTest): eq_(sess2.query(User).get(u2.id).addresses[0].email_address, 'somenewaddress') except sa.exc.InvalidRequestError, e: - assert "dont_load=True option does not support" in str(e) + assert "load=False option does not support" in str(e) @testing.resolve_artifact_names def test_synonym_comparable(self): diff --git a/test/orm/test_pickled.py b/test/orm/test_pickled.py index 93e188055f..6ac9f24701 100644 --- a/test/orm/test_pickled.py +++ b/test/orm/test_pickled.py @@ -59,7 +59,7 @@ class PickleTest(_fixtures.FixtureTest): u2 = pickle.loads(pickle.dumps(u1)) sess2 = create_session() - u2 = sess2.merge(u2, dont_load=True) + u2 = sess2.merge(u2, load=False) eq_(u2.name, 'ed') eq_(u2, User(name='ed', addresses=[Address(email_address='ed@bar.com')])) @@ -93,7 +93,7 @@ class PickleTest(_fixtures.FixtureTest): u2 = pickle.loads(pickle.dumps(u1)) sess2 = create_session() - u2 = sess2.merge(u2, dont_load=True) + u2 = sess2.merge(u2, load=False) eq_(u2.name, 'ed') assert 'addresses' not in u2.__dict__ ad = u2.addresses[0] -- 2.47.3