From: Mike Bayer Date: Sun, 15 Jul 2012 00:33:16 +0000 (-0400) Subject: - [bug] Fixed bug mostly local to new X-Git-Tag: rel_0_8_0b1~328 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=fce89ccf0c5561ff4747371619646aadc88c37cf;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git - [bug] Fixed bug mostly local to new AbstractConcreteBase helper where the "type" attribute from the superclass would not be overridden on the subclass to produce the "reserved for base" error message, instead placing a do-nothing attribute there. This was inconsistent vs. using ConcreteBase as well as all the behavior of classical concrete mappings, where the "type" column from the polymorphic base would be explicitly disabled on subclasses, unless overridden explicitly. --- diff --git a/CHANGES b/CHANGES index c5402cd11d..cab952a857 100644 --- a/CHANGES +++ b/CHANGES @@ -355,6 +355,19 @@ are also present in 0.8. 0.7.9 ===== +- orm + - [bug] Fixed bug mostly local to new + AbstractConcreteBase helper where the "type" + attribute from the superclass would not + be overridden on the subclass to produce the + "reserved for base" error message, instead placing + a do-nothing attribute there. This was inconsistent + vs. using ConcreteBase as well as all the behavior + of classical concrete mappings, where the "type" + column from the polymorphic base would be explicitly + disabled on subclasses, unless overridden + explicitly. + - sql - [bug] Fixed CTE bug whereby positional bound parameters present in the CTEs themselves diff --git a/lib/sqlalchemy/orm/descriptor_props.py b/lib/sqlalchemy/orm/descriptor_props.py index 83ccf75d85..efbceb0fc2 100644 --- a/lib/sqlalchemy/orm/descriptor_props.py +++ b/lib/sqlalchemy/orm/descriptor_props.py @@ -18,7 +18,7 @@ from ..sql import expression properties = util.importlater('sqlalchemy.orm', 'properties') class DescriptorProperty(MapperProperty): - """:class:`.MapperProperty` which proxies access to a + """:class:`.MapperProperty` which proxies access to a user-defined descriptor.""" doc = None @@ -34,7 +34,7 @@ class DescriptorProperty(MapperProperty): self.key = key if hasattr(prop, 'get_history'): - def get_history(self, state, dict_, + def get_history(self, state, dict_, passive=attributes.PASSIVE_OFF): return prop.get_history(state, dict_, passive) @@ -61,7 +61,7 @@ class DescriptorProperty(MapperProperty): create_proxied_attribute(self.descriptor)\ ( self.parent.class_, - self.key, + self.key, self.descriptor, lambda: self._comparator_factory(mapper), doc=self.doc, @@ -89,7 +89,7 @@ class CompositeProperty(DescriptorProperty): self._setup_event_handlers() def do_init(self): - """Initialization which occurs after the :class:`.CompositeProperty` + """Initialization which occurs after the :class:`.CompositeProperty` has been associated with its parent mapper. """ @@ -97,7 +97,7 @@ class CompositeProperty(DescriptorProperty): self._setup_arguments_on_columns() def _create_descriptor(self): - """Create the Python descriptor that will serve as + """Create the Python descriptor that will serve as the access point on instances of the mapped class. """ @@ -113,12 +113,12 @@ class CompositeProperty(DescriptorProperty): values = [getattr(instance, key) for key in self._attribute_keys] # 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 + # 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 + state.key is not None or not _none_set.issuperset(values) ): dict_[self.key] = self.composite_class(*values) @@ -139,7 +139,7 @@ class CompositeProperty(DescriptorProperty): setattr(instance, key, None) else: for key, value in zip( - self._attribute_keys, + self._attribute_keys, value.__composite_values__()): setattr(instance, key, value) @@ -198,7 +198,7 @@ class CompositeProperty(DescriptorProperty): return # if column elements aren't loaded, skip. - # __get__() will initiate a load for those + # __get__() will initiate a load for those # columns for k in self._attribute_keys: if k not in dict_: @@ -206,7 +206,7 @@ class CompositeProperty(DescriptorProperty): #assert self.key not in dict_ dict_[self.key] = self.composite_class( - *[state.dict[key] for key in + *[state.dict[key] for key in self._attribute_keys] ) @@ -217,16 +217,16 @@ class CompositeProperty(DescriptorProperty): def insert_update_handler(mapper, connection, state): """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 + defaults. Pop out the composite value here so that it recreates. - + """ state.dict.pop(self.key, None) - event.listen(self.parent, 'after_insert', + event.listen(self.parent, 'after_insert', insert_update_handler, raw=True) - event.listen(self.parent, 'after_update', + event.listen(self.parent, 'after_update', insert_update_handler, raw=True) event.listen(self.parent, 'load', load_handler, raw=True, propagate=True) event.listen(self.parent, 'refresh', load_handler, raw=True, propagate=True) @@ -307,19 +307,19 @@ class CompositeProperty(DescriptorProperty): return str(self.parent.class_.__name__) + "." + self.key class ConcreteInheritedProperty(DescriptorProperty): - """A 'do nothing' :class:`.MapperProperty` that disables + """A 'do nothing' :class:`.MapperProperty` that disables an attribute on a concrete subclass that is only present on the inherited mapper, not the concrete classes' mapper. Cases where this occurs include: - * When the superclass mapper is mapped against a - "polymorphic union", which includes all attributes from + * When the superclass mapper is mapped against a + "polymorphic union", which includes all attributes from all subclasses. * When a relationship() is configured on an inherited mapper, but not on the subclass mapper. Concrete mappers require - that relationship() is configured explicitly on each - subclass. + that relationship() is configured explicitly on each + subclass. """ @@ -337,7 +337,7 @@ class ConcreteInheritedProperty(DescriptorProperty): def warn(): raise AttributeError("Concrete %s does not implement " "attribute %r at the instance level. Add this " - "property explicitly to %s." % + "property explicitly to %s." % (self.parent, self.key, self.parent)) class NoninheritedConcreteProp(object): @@ -354,7 +354,7 @@ class ConcreteInheritedProperty(DescriptorProperty): class SynonymProperty(DescriptorProperty): - def __init__(self, name, map_column=None, + def __init__(self, name, map_column=None, descriptor=None, comparator_factory=None, doc=None): self.name = name @@ -387,7 +387,7 @@ class SynonymProperty(DescriptorProperty): if self.key not in parent.mapped_table.c: raise sa_exc.ArgumentError( "Can't compile synonym '%s': no column on table " - "'%s' named '%s'" + "'%s' named '%s'" % (self.name, parent.mapped_table.description, self.key)) elif parent.mapped_table.c[self.key] in \ parent._columntoproperty and \ @@ -397,13 +397,13 @@ class SynonymProperty(DescriptorProperty): raise sa_exc.ArgumentError( "Can't call map_column=True for synonym %r=%r, " "a ColumnProperty already exists keyed to the name " - "%r for column %r" % + "%r for column %r" % (self.key, self.name, self.name, self.key) ) p = properties.ColumnProperty(parent.mapped_table.c[self.key]) parent._configure_property( - self.name, p, - init=init, + self.name, p, + init=init, setparent=True) p._mapped_by_synonym = self.key diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index 57c8de4986..9aff3cb85d 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -575,6 +575,12 @@ class Mapper(object): self.inherits._inheriting_mappers.add(self) self.passive_updates = self.inherits.passive_updates self._all_tables = self.inherits._all_tables + for key, prop in mapper._props.iteritems(): + if key not in self._props and \ + not self._should_exclude(key, key, local=False, + column=None): + self._adapt_inherited_property(key, prop, False) + def _set_polymorphic_on(self, polymorphic_on): self.polymorphic_on = polymorphic_on @@ -947,7 +953,6 @@ class Mapper(object): # polymorphic_union. # we'll make a separate ColumnProperty for it. instrument = True - key = getattr(col, 'key', None) if key: if self._should_exclude(col.key, col.key, False, col): diff --git a/test/ext/test_declarative_inheritance.py b/test/ext/test_declarative_inheritance.py index 2d63e2de29..9384aa03dd 100644 --- a/test/ext/test_declarative_inheritance.py +++ b/test/ext/test_declarative_inheritance.py @@ -142,7 +142,7 @@ class DeclarativeInheritanceTest(DeclarativeTestBase): == 'cobol')).first(), c2) # ensure that the Manager mapper was compiled with the Manager id - # column as higher priority. this ensures that "Manager.id" + # column as higher priority. this ensures that "Manager.id" # is appropriately treated as the "id" column in the "manager" # table (reversed from 0.6's behavior.) @@ -220,7 +220,7 @@ class DeclarativeInheritanceTest(DeclarativeTestBase): sess.add(e1) sess.flush() sess.expunge_all() - eq_(sess.query(Person).first(), + eq_(sess.query(Person).first(), Engineer(primary_language='java', name='dilbert')) def test_add_sub_parentcol_after_the_fact(self): @@ -286,7 +286,7 @@ class DeclarativeInheritanceTest(DeclarativeTestBase): @testing.fails_if(lambda: True, "Not implemented until 0.7") def test_foreign_keys_with_col(self): - """Test that foreign keys that reference a literal 'id' subclass + """Test that foreign keys that reference a literal 'id' subclass 'id' attribute behave intuitively. See [ticket:1892]. @@ -352,7 +352,7 @@ class DeclarativeInheritanceTest(DeclarativeTestBase): sa.orm.configure_mappers() # no exceptions here def test_foreign_keys_with_col(self): - """Test that foreign keys that reference a literal 'id' subclass + """Test that foreign keys that reference a literal 'id' subclass 'id' attribute behave intuitively. See [ticket:1892]. @@ -862,7 +862,8 @@ class OverlapColPrecedenceTest(DeclarativeTestBase): from test.orm.test_events import _RemoveListeners class ConcreteInhTest(_RemoveListeners, DeclarativeTestBase): - def _roundtrip(self, Employee, Manager, Engineer, Boss, polymorphic=True): + def _roundtrip(self, Employee, Manager, Engineer, Boss, + polymorphic=True, explicit_type=False): Base.metadata.create_all() sess = create_session() e1 = Engineer(name='dilbert', primary_language='java') @@ -870,6 +871,23 @@ class ConcreteInhTest(_RemoveListeners, DeclarativeTestBase): m1 = Manager(name='dogbert', golf_swing='fore!') e3 = Engineer(name='vlad', primary_language='cobol') b1 = Boss(name="pointy haired") + + if polymorphic: + for obj in [e1, e2, m1, e3, b1]: + if explicit_type: + eq_(obj.type, obj.__mapper__.polymorphic_identity) + else: + assert_raises_message( + AttributeError, + "does not implement attribute .?'type' " + "at the instance level.", + getattr, obj, "type" + ) + else: + assert "type" not in Engineer.__dict__ + assert "type" not in Manager.__dict__ + assert "type" not in Boss.__dict__ + sess.add_all([e1, e2, m1, e3, b1]) sess.flush() sess.expunge_all() @@ -891,18 +909,18 @@ class ConcreteInhTest(_RemoveListeners, DeclarativeTestBase): test_needs_autoincrement=True), Column('name' , String(50)), Column('primary_language', String(50))) - managers = Table('managers', Base.metadata, - Column('id',Integer, primary_key=True, test_needs_autoincrement=True), - Column('name', String(50)), + managers = Table('managers', Base.metadata, + Column('id',Integer, primary_key=True, test_needs_autoincrement=True), + Column('name', String(50)), Column('golf_swing', String(50)) ) - boss = Table('boss', Base.metadata, - Column('id',Integer, primary_key=True, test_needs_autoincrement=True), - Column('name', String(50)), + boss = Table('boss', Base.metadata, + Column('id',Integer, primary_key=True, test_needs_autoincrement=True), + Column('name', String(50)), Column('golf_swing', String(50)) ) punion = polymorphic_union({ - 'engineer': engineers, + 'engineer': engineers, 'manager' : managers, 'boss': boss}, 'type', 'punion') @@ -974,31 +992,31 @@ class ConcreteInhTest(_RemoveListeners, DeclarativeTestBase): class Manager(Employee): __tablename__ = 'manager' - employee_id = Column(Integer, primary_key=True, + employee_id = Column(Integer, primary_key=True, test_needs_autoincrement=True) name = Column(String(50)) golf_swing = Column(String(40)) __mapper_args__ = { - 'polymorphic_identity':'manager', + 'polymorphic_identity':'manager', 'concrete':True} class Boss(Manager): __tablename__ = 'boss' - employee_id = Column(Integer, primary_key=True, + employee_id = Column(Integer, primary_key=True, test_needs_autoincrement=True) name = Column(String(50)) golf_swing = Column(String(40)) __mapper_args__ = { - 'polymorphic_identity':'boss', + 'polymorphic_identity':'boss', 'concrete':True} class Engineer(Employee): __tablename__ = 'engineer' - employee_id = Column(Integer, primary_key=True, + employee_id = Column(Integer, primary_key=True, test_needs_autoincrement=True) name = Column(String(50)) primary_language = Column(String(40)) - __mapper_args__ = {'polymorphic_identity':'engineer', + __mapper_args__ = {'polymorphic_identity':'engineer', 'concrete':True} self._roundtrip(Employee, Manager, Engineer, Boss) @@ -1006,38 +1024,86 @@ class ConcreteInhTest(_RemoveListeners, DeclarativeTestBase): def test_concrete_extension(self): class Employee(ConcreteBase, Base, fixtures.ComparableEntity): __tablename__ = 'employee' - employee_id = Column(Integer, primary_key=True, + employee_id = Column(Integer, primary_key=True, test_needs_autoincrement=True) name = Column(String(50)) __mapper_args__ = { - 'polymorphic_identity':'employee', + 'polymorphic_identity':'employee', 'concrete':True} class Manager(Employee): __tablename__ = 'manager' - employee_id = Column(Integer, primary_key=True, + employee_id = Column(Integer, primary_key=True, test_needs_autoincrement=True) name = Column(String(50)) golf_swing = Column(String(40)) __mapper_args__ = { - 'polymorphic_identity':'manager', + 'polymorphic_identity':'manager', 'concrete':True} class Boss(Manager): __tablename__ = 'boss' - employee_id = Column(Integer, primary_key=True, + employee_id = Column(Integer, primary_key=True, test_needs_autoincrement=True) name = Column(String(50)) golf_swing = Column(String(40)) __mapper_args__ = { - 'polymorphic_identity':'boss', + 'polymorphic_identity':'boss', 'concrete':True} class Engineer(Employee): __tablename__ = 'engineer' - employee_id = Column(Integer, primary_key=True, + employee_id = Column(Integer, primary_key=True, test_needs_autoincrement=True) name = Column(String(50)) primary_language = Column(String(40)) - __mapper_args__ = {'polymorphic_identity':'engineer', + __mapper_args__ = {'polymorphic_identity':'engineer', 'concrete':True} self._roundtrip(Employee, Manager, Engineer, Boss) + + def test_ok_to_override_type_from_abstract(self): + class Employee(AbstractConcreteBase, Base, fixtures.ComparableEntity): + pass + + class Manager(Employee): + __tablename__ = 'manager' + employee_id = Column(Integer, primary_key=True, + test_needs_autoincrement=True) + name = Column(String(50)) + golf_swing = Column(String(40)) + + @property + def type(self): + return "manager" + + __mapper_args__ = { + 'polymorphic_identity': "manager", + 'concrete':True} + + class Boss(Manager): + __tablename__ = 'boss' + employee_id = Column(Integer, primary_key=True, + test_needs_autoincrement=True) + name = Column(String(50)) + golf_swing = Column(String(40)) + + @property + def type(self): + return "boss" + + __mapper_args__ = { + 'polymorphic_identity': "boss", + 'concrete':True} + + class Engineer(Employee): + __tablename__ = 'engineer' + employee_id = Column(Integer, primary_key=True, + test_needs_autoincrement=True) + name = Column(String(50)) + primary_language = Column(String(40)) + + @property + def type(self): + return "engineer" + __mapper_args__ = {'polymorphic_identity': "engineer", + 'concrete':True} + self._roundtrip(Employee, Manager, Engineer, Boss, explicit_type=True) diff --git a/test/orm/inheritance/test_concrete.py b/test/orm/inheritance/test_concrete.py index 423887e7d9..d51efa62bc 100644 --- a/test/orm/inheritance/test_concrete.py +++ b/test/orm/inheritance/test_concrete.py @@ -166,8 +166,18 @@ class ConcreteTest(fixtures.MappedTest): polymorphic_identity='hacker') session = create_session() tom = Manager('Tom', 'knows how to manage things') + + assert_raises_message(AttributeError, + "does not implement attribute .?'type' at the instance level.", + setattr, tom, "type", "sometype") + jerry = Engineer('Jerry', 'knows how to program') hacker = Hacker('Kurt', 'Badass', 'knows how to hack') + + assert_raises_message(AttributeError, + "does not implement attribute .?'type' at the instance level.", + setattr, hacker, "type", "sometype") + session.add_all((tom, jerry, hacker)) session.flush() @@ -453,7 +463,7 @@ class PropertyInheritanceTest(fixtures.MappedTest): }) mapper(Dest, dest_table, properties={ - 'many_a': relationship(A,back_populates='some_dest'), + 'many_a': relationship(A,back_populates='some_dest'), 'many_b': relationship(B,back_populates='some_dest') }) sess = sessionmaker()() @@ -490,7 +500,7 @@ class PropertyInheritanceTest(fixtures.MappedTest): self.tables.dest_table) - ajoin = polymorphic_union({'a': a_table, 'b': b_table, 'c':c_table}, + ajoin = polymorphic_union({'a': a_table, 'b': b_table, 'c':c_table}, 'type','ajoin') mapper( A, @@ -524,7 +534,7 @@ class PropertyInheritanceTest(fixtures.MappedTest): mapper(Dest, dest_table, properties={ 'many_a': relationship(A, - back_populates='some_dest', + back_populates='some_dest', order_by=ajoin.c.id) } ) @@ -555,10 +565,10 @@ class PropertyInheritanceTest(fixtures.MappedTest): def go(): eq_( [ - Dest(many_a=[A(aname='a1'), - B(bname='b1'), + Dest(many_a=[A(aname='a1'), + B(bname='b1'), B(bname='b2'), - C(cname='c1')]), + C(cname='c1')]), Dest(many_a=[A(aname='a2'), C(cname='c2')])], sess.query(Dest).options(joinedload(Dest.many_a)).order_by(Dest.id).all()) @@ -574,7 +584,7 @@ class PropertyInheritanceTest(fixtures.MappedTest): self.classes.Dest, self.tables.dest_table) - ajoin = polymorphic_union({'a': a_table, 'b': b_table, 'c':c_table}, + ajoin = polymorphic_union({'a': a_table, 'b': b_table, 'c':c_table}, 'type','ajoin') mapper( A, @@ -608,7 +618,7 @@ class PropertyInheritanceTest(fixtures.MappedTest): mapper(Dest, dest_table, properties={ 'many_a': relationship(A, - back_populates='some_dest', + back_populates='some_dest', order_by=ajoin.c.id) } )