From: Mike Bayer Date: Wed, 26 Oct 2011 16:41:18 +0000 (-0400) Subject: - [bug] the value of a composite attribute is now X-Git-Tag: rel_0_7_4~70 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=d9adb2a4fd3e865d3c8d4f6f2e0a12d5c4036c97;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git - [bug] the value of a composite attribute is now expired after an insert or update operation, instead of regenerated in place. This ensures that a column value which is expired within a flush will be loaded first, before the composite is regenerated using that value. [ticket:2309] - [bug] The fix in [ticket:2309] also emits the "refresh" event when the composite value is loaded on access, even if all column values were already present, as is appropriate. This fixes the "mutable" extension which relies upon the "load" event to ensure the _parents dictionary is up to date, fixes [ticket:2308]. Thanks to Scott Torborg for the test case here. --- diff --git a/CHANGES b/CHANGES index f2411b0af4..74f1bdc024 100644 --- a/CHANGES +++ b/CHANGES @@ -18,6 +18,22 @@ CHANGES - [bug] Cls.column.collate("some collation") now works. [ticket:1776] Also in 0.6.9 + - [bug] the value of a composite attribute is now + expired after an insert or update operation, instead + of regenerated in place. This ensures that a + column value which is expired within a flush + will be loaded first, before the composite + is regenerated using that value. [ticket:2309] + + - [bug] The fix in [ticket:2309] also emits the + "refresh" event when the composite value is + loaded on access, even if all column + values were already present, as is appropriate. + This fixes the "mutable" extension which relies + upon the "load" event to ensure the _parents + dictionary is up to date, fixes [ticket:2308]. + Thanks to Scott Torborg for the test case here. + - sql - [feature] Added accessor to types called "python_type", returns the rudimentary Python type object diff --git a/lib/sqlalchemy/orm/descriptor_props.py b/lib/sqlalchemy/orm/descriptor_props.py index 594705a8af..c4d59defad 100644 --- a/lib/sqlalchemy/orm/descriptor_props.py +++ b/lib/sqlalchemy/orm/descriptor_props.py @@ -104,6 +104,7 @@ class CompositeProperty(DescriptorProperty): def fget(instance): dict_ = attributes.instance_dict(instance) + state = attributes.instance_state(instance) if self.key not in dict_: # key not present. Iterate through related @@ -111,11 +112,17 @@ class CompositeProperty(DescriptorProperty): # ensures they all load. values = [getattr(instance, key) for key in self._attribute_keys] - # usually, the load() event will have loaded our key - # at this point, unless we only loaded relationship() - # attributes above. Populate here if that's the case. - if self.key not in dict_ and not _none_set.issuperset(values): + # current expected behavior here is that the composite is + # created on access if the object is persistent or if + # col attributes have non-None. This would be better + # if the composite were created unconditionally, + # but that would be a behavioral change. + if self.key not in dict_ and ( + state.key is not None or + not _none_set.issuperset(values) + ): dict_[self.key] = self.composite_class(*values) + state.manager.dispatch.refresh(state, None, [self.key]) return dict_.get(self.key, None) @@ -197,6 +204,7 @@ class CompositeProperty(DescriptorProperty): if k not in dict_: return + #assert self.key not in dict_ dict_[self.key] = self.composite_class( *[state.dict[key] for key in self._attribute_keys] @@ -207,10 +215,14 @@ class CompositeProperty(DescriptorProperty): state.dict.pop(self.key, None) def insert_update_handler(mapper, connection, state): - state.dict[self.key] = self.composite_class( - *[state.dict.get(key, None) for key in - self._attribute_keys] - ) + """After an insert or update, some columns may be expired due + to server side defaults, or re-populated due to client side + defaults. Pop out the composite value here so that it + recreates. + + """ + + state.dict.pop(self.key, None) event.listen(self.parent, 'after_insert', insert_update_handler, raw=True) diff --git a/test/ext/test_mutable.py b/test/ext/test_mutable.py index df4006c01b..129a460b0c 100644 --- a/test/ext/test_mutable.py +++ b/test/ext/test_mutable.py @@ -1,4 +1,4 @@ -from sqlalchemy import Integer, ForeignKey +from sqlalchemy import Integer, ForeignKey, String from sqlalchemy.types import PickleType, TypeDecorator, VARCHAR from sqlalchemy.orm import mapper, Session, composite from sqlalchemy.orm.mapper import Mapper @@ -115,6 +115,17 @@ class _MutableDictTestBase(object): f2.data['a'] = 'c' assert f2 in sess.dirty + def test_unrelated_flush(self): + sess = Session() + f1 = Foo(data={"a":"b"}, unrelated_data="unrelated") + sess.add(f1) + sess.flush() + f1.unrelated_data = "unrelated 2" + sess.flush() + f1.data["a"] = "c" + sess.commit() + eq_(f1.data["a"], "c") + def _test_non_mutable(self): sess = Session() @@ -137,7 +148,8 @@ class MutableWithScalarPickleTest(_MutableDictTestBase, fixtures.MappedTest): Column('id', Integer, primary_key=True, test_needs_autoincrement=True), Column('skip', mutable_pickle), Column('data', mutable_pickle), - Column('non_mutable_data', PickleType) + Column('non_mutable_data', PickleType), + Column('unrelated_data', String(50)) ) def test_non_mutable(self): @@ -170,7 +182,8 @@ class MutableWithScalarJSONTest(_MutableDictTestBase, fixtures.MappedTest): Table('foo', metadata, Column('id', Integer, primary_key=True, test_needs_autoincrement=True), Column('data', MutationDict.as_mutable(JSONEncodedDict)), - Column('non_mutable_data', JSONEncodedDict) + Column('non_mutable_data', JSONEncodedDict), + Column('unrelated_data', String(50)) ) def test_non_mutable(self): @@ -184,7 +197,8 @@ class MutableAssocWithAttrInheritTest(_MutableDictTestBase, fixtures.MappedTest) Table('foo', metadata, Column('id', Integer, primary_key=True, test_needs_autoincrement=True), Column('data', PickleType), - Column('non_mutable_data', PickleType) + Column('non_mutable_data', PickleType), + Column('unrelated_data', String(50)) ) Table('subfoo', metadata, @@ -230,7 +244,8 @@ class MutableAssociationScalarPickleTest(_MutableDictTestBase, fixtures.MappedTe Table('foo', metadata, Column('id', Integer, primary_key=True, test_needs_autoincrement=True), Column('skip', PickleType), - Column('data', PickleType) + Column('data', PickleType), + Column('unrelated_data', String(50)) ) class MutableAssociationScalarJSONTest(_MutableDictTestBase, fixtures.MappedTest): @@ -260,7 +275,8 @@ class MutableAssociationScalarJSONTest(_MutableDictTestBase, fixtures.MappedTest Table('foo', metadata, Column('id', Integer, primary_key=True, test_needs_autoincrement=True), - Column('data', JSONEncodedDict) + Column('data', JSONEncodedDict), + Column('unrelated_data', String(50)) ) @@ -270,7 +286,8 @@ class _CompositeTestBase(object): Table('foo', metadata, Column('id', Integer, primary_key=True, test_needs_autoincrement=True), Column('x', Integer), - Column('y', Integer) + Column('y', Integer), + Column('unrelated_data', String(50)) ) def teardown(self): @@ -374,6 +391,18 @@ class MutableCompositesTest(_CompositeTestBase, fixtures.MappedTest): setattr, f1, 'data', 'foo' ) + def test_unrelated_flush(self): + sess = Session() + f1 = Foo(data=Point(3, 4), unrelated_data="unrelated") + sess.add(f1) + sess.flush() + f1.unrelated_data = "unrelated 2" + sess.flush() + f1.data.x = 5 + sess.commit() + + eq_(f1.data.x, 5) + class MutableInheritedCompositesTest(_CompositeTestBase, fixtures.MappedTest): @classmethod def define_tables(cls, metadata): diff --git a/test/orm/test_composites.py b/test/orm/test_composites.py index 0c16e57a18..60eaf2df1d 100644 --- a/test/orm/test_composites.py +++ b/test/orm/test_composites.py @@ -27,8 +27,7 @@ class PointTest(fixtures.MappedTest): Column('id', Integer, primary_key=True, test_needs_autoincrement=True), Column('graph_id', Integer, - ForeignKey('graphs.id'), - nullable=False), + ForeignKey('graphs.id')), Column('x1', Integer), Column('y1', Integer), Column('x2', Integer), @@ -115,6 +114,32 @@ class PointTest(fixtures.MappedTest): e = sess.query(Edge).get(g.edges[1].id) eq_(e.end, Point(18, 4)) + def test_not_none(self): + Graph, Edge, Point = (self.classes.Graph, + self.classes.Edge, + self.classes.Point) + + # current contract. the composite is None + # when hasn't been populated etc. on a + # pending/transient object. + e1 = Edge() + assert e1.end is None + sess = Session() + sess.add(e1) + + # however, once it's persistent, the code as of 0.7.3 + # would unconditionally populate it, even though it's + # all None. I think this usage contract is inconsistent, + # and it would be better that the composite is just + # created unconditionally in all cases. + # but as we are just trying to fix [ticket:2308] and + # [ticket:2309] without changing behavior we maintain + # that only "persistent" gets the composite with the + # Nones + + sess.flush() + assert e1.end is not None + def test_eager_load(self): Graph, Point = self.classes.Graph, self.classes.Point @@ -322,7 +347,7 @@ class DefaultsTest(fixtures.MappedTest): test_needs_autoincrement=True), Column('x1', Integer, default=2), Column('x2', Integer), - Column('x3', Integer, default=15), + Column('x3', Integer, server_default="15"), Column('x4', Integer) ) @@ -335,21 +360,24 @@ class DefaultsTest(fixtures.MappedTest): class FBComposite(cls.Comparable): def __init__(self, x1, x2, x3, x4): - self.x1 = x1 + self.goofy_x1 = x1 self.x2 = x2 self.x3 = x3 self.x4 = x4 def __composite_values__(self): - return self.x1, self.x2, self.x3, self.x4 + return self.goofy_x1, self.x2, self.x3, self.x4 __hash__ = None def __eq__(self, other): - return other.x1 == self.x1 and \ + return other.goofy_x1 == self.goofy_x1 and \ other.x2 == self.x2 and \ other.x3 == self.x3 and \ other.x4 == self.x4 def __ne__(self, other): return not self.__eq__(other) - + def __repr__(self): + return "FBComposite(%r, %r, %r, %r)" % ( + self.goofy_x1, self.x2, self.x3, self.x4 + ) mapper(Foobar, foobars, properties=dict( foob=sa.orm.composite(FBComposite, foobars.c.x1, @@ -368,12 +396,12 @@ class DefaultsTest(fixtures.MappedTest): sess.add(f1) sess.flush() - assert f1.foob == FBComposite(2, 5, 15, None) + eq_(f1.foob, FBComposite(2, 5, 15, None)) f2 = Foobar() sess.add(f2) sess.flush() - assert f2.foob == FBComposite(2, None, 15, None) + eq_(f2.foob, FBComposite(2, None, 15, None)) def test_set_composite_values(self): Foobar, FBComposite = self.classes.Foobar, self.classes.FBComposite @@ -384,7 +412,8 @@ class DefaultsTest(fixtures.MappedTest): sess.add(f1) sess.flush() - assert f1.foob == FBComposite(2, 5, 15, None) + eq_(f1.foob, FBComposite(2, 5, 15, None)) + class MappedSelectTest(fixtures.MappedTest): @classmethod