From: Mike Bayer Date: Thu, 9 Sep 2010 23:21:23 +0000 (-0400) Subject: - collection docs X-Git-Tag: rel_0_6_5~77 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9674b4bf176539400ba10de41c32186873d4dc13;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git - collection docs - Added an assertion during flush which ensures that no NULL-holding identity keys were generated on "newly persistent" objects. This can occur when user defined code inadvertently triggers flushes on not-fully-loaded objects. --- diff --git a/CHANGES b/CHANGES index ea30651c51..6d783757de 100644 --- a/CHANGES +++ b/CHANGES @@ -3,6 +3,15 @@ ======= CHANGES ======= +0.6.5 +===== +- orm + - Added an assertion during flush which ensures + that no NULL-holding identity keys were generated + on "newly persistent" objects. + This can occur when user defined code inadvertently + triggers flushes on not-fully-loaded objects. + 0.6.4 ===== - orm diff --git a/doc/build/orm/collections.rst b/doc/build/orm/collections.rst index 73ba1277b4..131adb70be 100644 --- a/doc/build/orm/collections.rst +++ b/doc/build/orm/collections.rst @@ -327,6 +327,34 @@ collection support to other classes. It uses a keying function to delegate to MappedCollection.__init__(self, keyfunc=lambda node: node.name) OrderedDict.__init__(self, *args, **kw) +When subclassing :class:`.MappedCollection`, user-defined versions +of ``__setitem__()`` or ``__delitem__()`` should be decorated +with :meth:`.collection.internally_instrumented`, **if** they call down +to those same methods on :class:`.MappedCollection`. This because the methods +on :class:`.MappedCollection` are already instrumented - calling them +from within an already instrumented call can cause events to be fired off +repeatedly, or inappropriately, leading to internal state corruption in +rare cases:: + + from sqlalchemy.orm.collections import MappedCollection,\ + collection + + class MyMappedCollection(MappedCollection): + """Use @internally_instrumented when your methods + call down to already-instrumented methods. + + """ + + @collection.internally_instrumented + def __setitem__(self, key, value, _sa_initiator=None): + # do something with key, value + super(MappedCollection, self).__setitem__(key, value, _sa_initiator) + + @collection.internally_instrumented + def __delitem__(self, key, _sa_initiator=None): + # do something with key, value + super(MappedCollection, self).__delitem__(key, value, _sa_initiator) + The ORM understands the ``dict`` interface just like lists and sets, and will automatically instrument all dict-like methods if you choose to subclass ``dict`` or provide dict-like collection behavior in a duck-typed class. You @@ -368,6 +396,7 @@ Collections API .. autofunction:: attribute_mapped_collection .. autoclass:: collection + :members: .. autofunction:: collection_adapter diff --git a/lib/sqlalchemy/__init__.py b/lib/sqlalchemy/__init__.py index 2d809e3396..cb4e8e10b1 100644 --- a/lib/sqlalchemy/__init__.py +++ b/lib/sqlalchemy/__init__.py @@ -114,6 +114,6 @@ from sqlalchemy.engine import create_engine, engine_from_config __all__ = sorted(name for name, obj in locals().items() if not (name.startswith('_') or inspect.ismodule(obj))) -__version__ = '0.6.4' +__version__ = '0.6.5' del inspect, sys diff --git a/lib/sqlalchemy/orm/collections.py b/lib/sqlalchemy/orm/collections.py index 884ec11222..b80be970af 100644 --- a/lib/sqlalchemy/orm/collections.py +++ b/lib/sqlalchemy/orm/collections.py @@ -253,7 +253,7 @@ class collection(object): The remover method is called with one positional argument: the value to remove. The method will be automatically decorated with - 'removes_return()' if not already decorated:: + :meth:`removes_return` if not already decorated:: @collection.remover def zap(self, entity): ... @@ -293,7 +293,7 @@ class collection(object): """Tag the method as instrumented. This tag will prevent any decoration from being applied to the method. - Use this if you are orchestrating your own calls to collection_adapter + Use this if you are orchestrating your own calls to :func:`.collection_adapter` in one of the basic SQLAlchemy interface methods, or to prevent an automatic ABC method decoration from wrapping your implementation:: @@ -339,7 +339,7 @@ class collection(object): The default converter implementation will use duck-typing to do the conversion. A dict-like collection will be convert into an iterable - of dictionary values, and other types will simply be iterated. + of dictionary values, and other types will simply be iterated:: @collection.converter def convert(self, other): ... @@ -442,7 +442,8 @@ class collection(object): # public instrumentation interface for 'internally instrumented' # implementations def collection_adapter(collection): - """Fetch the CollectionAdapter for a collection.""" + """Fetch the :class:`.CollectionAdapter` for a collection.""" + return getattr(collection, '_sa_adapter', None) def collection_iter(collection): diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index d906df2a37..54b41fcc61 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -1018,6 +1018,14 @@ class Session(object): if obj is not None: instance_key = mapper._identity_key_from_state(state) + + if _none_set.issubset(instance_key[1]) and \ + not mapper.allow_partial_pks or \ + _none_set.issuperset(instance_key[1]): + raise exc.FlushError('Instance %s has a NULL identity ' + 'key. Check if this flush is occuring at an ' + 'inappropriate time, such as during a load ' + 'operation.' % mapperutil.state_str(state)) if state.key is None: state.key = instance_key diff --git a/test/orm/test_unitofwork.py b/test/orm/test_unitofwork.py index ea63975178..da0f2a9e4b 100644 --- a/test/orm/test_unitofwork.py +++ b/test/orm/test_unitofwork.py @@ -11,7 +11,8 @@ from sqlalchemy.test import engines, testing, pickleable from sqlalchemy import Integer, String, ForeignKey, literal_column from sqlalchemy.test.schema import Table from sqlalchemy.test.schema import Column -from sqlalchemy.orm import mapper, relationship, create_session, column_property, attributes +from sqlalchemy.orm import mapper, relationship, create_session, \ + column_property, attributes, Session, reconstructor, object_session from sqlalchemy.test.testing import eq_, ne_ from test.orm import _base, _fixtures from test.engine import _base as engine_base @@ -2111,7 +2112,68 @@ class BooleanColTest(_base.MappedTest): sess.flush() eq_(sess.query(T).filter(T.value==True).all(), [T(value=True, name="t1"),T(value=True, name="t3")]) - +class DontAllowFlushOnLoadingObjectTest(_base.MappedTest): + """Test that objects with NULL identity keys aren't permitted to complete a flush. + + User-defined callables that execute during a load may modify state + on instances which results in their being autoflushed, before attributes + are populated. If the primary key identifiers are missing, an explicit assertion + is needed to check that the object doesn't go through the flush process with + no net changes and gets placed in the identity map with an incorrect + identity key. + + """ + @classmethod + def define_tables(cls, metadata): + t1 = Table('t1', metadata, + Column('id', Integer, primary_key=True), + Column('data', String(30)), + ) + + @testing.resolve_artifact_names + def test_flush_raises(self): + class T1(_base.ComparableEntity): + @reconstructor + def go(self): + # blow away 'id', no change event. + # this simulates a callable occuring + # before 'id' was even populated, i.e. a callable + # within an attribute_mapped_collection + self.__dict__.pop('id', None) + + # generate a change event, perhaps this occurs because + # someone wrote a broken attribute_mapped_collection that + # inappropriately fires off change events when it should not, + # now we're dirty + self.data = 'foo bar' + + # blow away that change, so an UPDATE does not occur + # (since it would break) + self.__dict__.pop('data', None) + + # flush ! any lazyloader here would trigger + # autoflush, for example. + sess.flush() + + mapper(T1, t1) + + sess = Session() + sess.add(T1(data='test', id=5)) + sess.commit() + sess.close() + + # make sure that invalid state doesn't get into the session + # with the wrong key. If the identity key is not NULL, at least + # the population process would continue after the erroneous flush + # and thing would right themselves. + assert_raises_message(sa.orm.exc.FlushError, + 'has a NULL identity key. Check if this ' + 'flush is occuring at an inappropriate ' + 'time, such as during a load operation.', + sess.query(T1).first) + + + class RowSwitchTest(_base.MappedTest): @classmethod def define_tables(cls, metadata):