From 092fbb40eb72d08a9eb433a6ac182723af774201 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Mon, 30 Aug 2010 17:41:47 -0400 Subject: [PATCH] - The include_properties and exclude_properties arguments to mapper() now accept Column objects as members in addition to strings. This so that same-named Column objects, such as those within a join(), can be disambiguated. - A warning is now emitted if a mapper is created against a join or other single selectable that includes multiple columns with the same name in its .c. collection, and those columns aren't explictly named as part of the same or separate attributes (or excluded). In 0.7 this warning will be an exception. Note that this warning is not emitted when the combination occurs as a result of inheritance, so that attributes still allow being overridden naturally. [ticket:1896]. In 0.7 this will be improved further. - The primary_key argument to mapper() can now specify a series of columns that are only a subset of the calculated "primary key" columns of the mapped selectable, without an error being raised. This helps for situations where a selectable's effective primary key is simpler than the number of columns in the selectable that are actually marked as "primary_key", such as a join against two tables on their primary key columns [ticket:1896]. --- CHANGES | 27 ++++++++++++ lib/sqlalchemy/orm/__init__.py | 25 ++++++----- lib/sqlalchemy/orm/mapper.py | 58 ++++++++++++++++++-------- lib/sqlalchemy/orm/properties.py | 12 +++++- lib/sqlalchemy/orm/strategies.py | 19 +++++---- test/ext/test_declarative.py | 35 ++++++++++++++++ test/orm/inheritance/test_basic.py | 7 +++- test/orm/test_mapper.py | 67 +++++++++++++++++++++++------- 8 files changed, 197 insertions(+), 53 deletions(-) diff --git a/CHANGES b/CHANGES index a4e5c22834..d78cd6655b 100644 --- a/CHANGES +++ b/CHANGES @@ -21,6 +21,33 @@ CHANGES where a one-step Session constructor is desired. Most users should stick with sessionmaker() for general use, however. + + - The include_properties and exclude_properties arguments + to mapper() now accept Column objects as members in + addition to strings. This so that same-named Column + objects, such as those within a join(), can be + disambiguated. + + - A warning is now emitted if a mapper is created against a + join or other single selectable that includes multiple + columns with the same name in its .c. collection, + and those columns aren't explictly named as part of + the same or separate attributes (or excluded). + In 0.7 this warning will be an exception. Note that + this warning is not emitted when the combination occurs + as a result of inheritance, so that attributes + still allow being overridden naturally. + [ticket:1896]. In 0.7 this will be improved further. + + - The primary_key argument to mapper() can now specify + a series of columns that are only a subset of + the calculated "primary key" columns of the mapped + selectable, without an error being raised. This + helps for situations where a selectable's effective + primary key is simpler than the number of columns + in the selectable that are actually marked as + "primary_key", such as a join against two + tables on their primary key columns [ticket:1896]. - An object that's been deleted now gets a flag 'deleted', which prohibits the object from diff --git a/lib/sqlalchemy/orm/__init__.py b/lib/sqlalchemy/orm/__init__.py index 55daeb3dbd..17d967db4c 100644 --- a/lib/sqlalchemy/orm/__init__.py +++ b/lib/sqlalchemy/orm/__init__.py @@ -671,22 +671,27 @@ def mapper(class_, local_table=None, *args, **params): :param concrete: If True, indicates this mapper should use concrete table inheritance with its parent mapper. - :param exclude_properties: A list of properties not to map. Columns - present in the mapped table and present in this list will not be - automatically converted into properties. Note that neither this - option nor include_properties will allow an end-run around Python - inheritance. If mapped class ``B`` inherits from mapped class - ``A``, no combination of includes or excludes will allow ``B`` to - have fewer properties than its superclass, ``A``. + :param exclude_properties: A list or set of string column names to + be excluded from mapping. As of SQLAlchemy 0.6.4, this collection + may also include :class:`.Column` objects. Columns named or present + in this list will not be automatically mapped. Note that neither + this option nor include_properties will allow one to circumvent plan + Python inheritance - if mapped class ``B`` inherits from mapped + class ``A``, no combination of includes or excludes will allow ``B`` + to have fewer properties than its superclass, ``A``. :param extension: A :class:`.MapperExtension` instance or list of :class:`~sqlalchemy.orm.interfaces.MapperExtension` instances which will be applied to all operations by this :class:`~sqlalchemy.orm.mapper.Mapper`. - :param include_properties: An inclusive list of properties to map. - Columns present in the mapped table but not present in this list - will not be automatically converted into properties. + :param include_properties: An inclusive list or set of string column + names to map. As of SQLAlchemy 0.6.4, this collection may also + include :class:`.Column` objects in order to disambiguate between + same-named columns in a selectable (such as a + :func:`~.expression.join()`). If this list is not ``None``, columns + present in the mapped table but not named or present in this list + will not be automatically mapped. See also "exclude_properties". :param inherits: Another :class:`~sqlalchemy.orm.Mapper` for which this :class:`~sqlalchemy.orm.Mapper` will have an inheritance diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index c3e4b042ed..1f2216cecc 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -192,8 +192,14 @@ class Mapper(object): else: self.polymorphic_map = _polymorphic_map - self.include_properties = include_properties - self.exclude_properties = exclude_properties + if include_properties: + self.include_properties = util.to_set(include_properties) + else: + self.include_properties = None + if exclude_properties: + self.exclude_properties = util.to_set(exclude_properties) + else: + self.exclude_properties = None self.compiled = False @@ -471,7 +477,7 @@ class Mapper(object): for col in self._columntoproperty if not hasattr(col, 'table') or col.table not in self._cols_by_table) - + # if explicit PK argument sent, add those columns to the # primary key mappings if self.primary_key_argument: @@ -479,13 +485,14 @@ class Mapper(object): if k.table not in self._pks_by_table: self._pks_by_table[k.table] = util.OrderedSet() self._pks_by_table[k.table].add(k) - - if self.mapped_table not in self._pks_by_table or \ - len(self._pks_by_table[self.mapped_table]) == 0: - raise sa_exc.ArgumentError( - "Mapper %s could not assemble any primary " - "key columns for mapped table '%s'" % - (self, self.mapped_table.description)) + + # otherwise, see that we got a full PK for the mapped table + elif self.mapped_table not in self._pks_by_table or \ + len(self._pks_by_table[self.mapped_table]) == 0: + raise sa_exc.ArgumentError( + "Mapper %s could not assemble any primary " + "key columns for mapped table '%s'" % + (self, self.mapped_table.description)) if self.inherits and \ not self.concrete and \ @@ -537,7 +544,7 @@ class Mapper(object): if self.inherits: for key, prop in self.inherits._props.iteritems(): if key not in self._props and \ - not self._should_exclude(key, key, local=False): + not self._should_exclude(key, key, local=False, column=None): self._adapt_inherited_property(key, prop, False) # create properties for each column in the mapped table, @@ -550,7 +557,8 @@ class Mapper(object): if self._should_exclude( column.key, column_key, - local=self.local_table.c.contains_column(column) + local=self.local_table.c.contains_column(column), + column=column ): continue @@ -583,7 +591,7 @@ class Mapper(object): % col.description) else: instrument = True - if self._should_exclude(col.key, col.key, local=False): + if self._should_exclude(col.key, col.key, local=False, column=col): raise sa_exc.InvalidRequestError( "Cannot exclude or override the discriminator column %r" % col.key) @@ -625,8 +633,18 @@ class Mapper(object): # existing ColumnProperty from an inheriting mapper. # make a copy and append our column to it prop = prop.copy() + else: + util.warn( + "Implicitly combining column %s with column " + "%s under attribute '%s'. This usage will be " + "prohibited in 0.7. Please configure one " + "or more attributes for these same-named columns " + "explicitly." + % (prop.columns[-1], column, key)) + prop.columns.append(column) self._log("appending to existing ColumnProperty %s" % (key)) + elif prop is None or isinstance(prop, ConcreteInheritedProperty): mapped_column = [] for c in columns: @@ -1099,7 +1117,7 @@ class Mapper(object): (MapperProperty, attributes.InstrumentedAttribute)) and \ hasattr(obj, '__get__') - def _should_exclude(self, name, assigned_name, local): + def _should_exclude(self, name, assigned_name, local, column): """determine whether a particular property should be implicitly present on the class. @@ -1121,13 +1139,17 @@ class Mapper(object): getattr(self.class_, assigned_name)): return True - if (self.include_properties is not None and - name not in self.include_properties): + if self.include_properties is not None and \ + name not in self.include_properties and \ + (column is None or column not in self.include_properties): self._log("not including property %s" % (name)) return True - if (self.exclude_properties is not None and - name in self.exclude_properties): + if self.exclude_properties is not None and \ + ( + name in self.exclude_properties or \ + (column is not None and column in self.exclude_properties) + ): self._log("excluding property %s" % (name)) return True diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py index 7e19d7b16d..263b611a57 100644 --- a/lib/sqlalchemy/orm/properties.py +++ b/lib/sqlalchemy/orm/properties.py @@ -60,7 +60,17 @@ class ColumnProperty(StrategizedProperty): self.__class__.Comparator) self.descriptor = kwargs.pop('descriptor', None) self.extension = kwargs.pop('extension', None) - self.doc = kwargs.pop('doc', getattr(columns[0], 'doc', None)) + + if 'doc' in kwargs: + self.doc = kwargs.pop('doc') + else: + for col in reversed(self.columns): + doc = getattr(col, 'doc', None) + if doc is not None: + self.doc = doc + break + else: + self.doc = None if kwargs: raise TypeError( diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index 1c4571aed8..4d98e8e629 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -118,17 +118,20 @@ class ColumnLoader(LoaderStrategy): ) def create_row_processor(self, selectcontext, path, mapper, row, adapter): - key, col = self.key, self.columns[0] - if adapter: - col = adapter.columns[col] - - if col is not None and col in row: - def new_execute(state, dict_, row): - dict_[key] = row[col] + key = self.key + # look through list of columns represented here + # to see which, if any, is present in the row. + for col in self.columns: + if adapter: + col = adapter.columns[col] + if col is not None and col in row: + def new_execute(state, dict_, row): + dict_[key] = row[col] + return new_execute, None else: def new_execute(state, dict_, row): state.expire_attribute_pre_commit(dict_, key) - return new_execute, None + return new_execute, None log.class_logger(ColumnLoader) diff --git a/test/ext/test_declarative.py b/test/ext/test_declarative.py index 71e31233b8..26e1563fe2 100644 --- a/test/ext/test_declarative.py +++ b/test/ext/test_declarative.py @@ -1316,7 +1316,42 @@ class DeclarativeInheritanceTest(DeclarativeTestBase): primary_language = Column('primary_language', String(50)) assert class_mapper(Engineer).inherits is class_mapper(Person) + + @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 + 'id' attribute behave intuitively. + + See ticket 1892. + + """ + class Booking(Base): + __tablename__ = 'booking' + id = Column(Integer, primary_key=True) + + class PlanBooking(Booking): + __tablename__ = 'plan_booking' + id = Column(Integer, ForeignKey(Booking.id), + primary_key=True) + + # referencing PlanBooking.id gives us the column + # on plan_booking, not booking + class FeatureBooking(Booking): + __tablename__ = 'feature_booking' + id = Column(Integer, ForeignKey(Booking.id), + primary_key=True) + plan_booking_id = Column(Integer, + ForeignKey(PlanBooking.id)) + + plan_booking = relationship(PlanBooking, + backref='feature_bookings') + + assert FeatureBooking.__table__.c.plan_booking_id.\ + references(PlanBooking.__table__.c.id) + assert FeatureBooking.__table__.c.id.\ + references(Booking.__table__.c.id) + def test_with_undefined_foreignkey(self): class Parent(Base): diff --git a/test/orm/inheritance/test_basic.py b/test/orm/inheritance/test_basic.py index b4aaf13bae..c6dec16b7f 100644 --- a/test/orm/inheritance/test_basic.py +++ b/test/orm/inheritance/test_basic.py @@ -981,7 +981,8 @@ class OverrideColKeyTest(_base.MappedTest): # s2 gets a new id, base_id is overwritten by the ultimate # PK col assert s2.id == s2.base_id != 15 - + + @testing.emits_warning(r'Implicit') def test_override_implicit(self): # this is how the pattern looks intuitively when # using declarative. @@ -1143,7 +1144,9 @@ class OptimizedLoadTest(_base.MappedTest): # redefine Sub's "id" to favor the "id" col in the subtable. # "id" is also part of the primary join condition - mapper(Sub, sub, inherits=Base, polymorphic_identity='sub', properties={'id':sub.c.id}) + mapper(Sub, sub, inherits=Base, + polymorphic_identity='sub', + properties={'id':[sub.c.id, base.c.id]}) sess = sessionmaker()() s1 = Sub(data='s1data', sub='s1sub') sess.add(s1) diff --git a/test/orm/test_mapper.py b/test/orm/test_mapper.py index e24906a1f5..3012f4b43f 100644 --- a/test/orm/test_mapper.py +++ b/test/orm/test_mapper.py @@ -482,18 +482,19 @@ class MapperTest(_fixtures.FixtureTest): class Manager(Employee): pass class Hoho(object): pass class Lala(object): pass - + class Fub(object):pass + class Frob(object):pass class HasDef(object): def name(self): pass p_m = mapper(Person, t, polymorphic_on=t.c.type, include_properties=('id', 'type', 'name')) - e_m = mapper(Employee, inherits=p_m, polymorphic_identity='employee', - properties={ - 'boss': relationship(Manager, backref=backref('peon', ), remote_side=t.c.id) - }, - exclude_properties=('vendor_id',)) + e_m = mapper(Employee, inherits=p_m, + polymorphic_identity='employee', properties={'boss' + : relationship(Manager, backref=backref('peon'), + remote_side=t.c.id)}, + exclude_properties=('vendor_id', )) m_m = mapper(Manager, inherits=e_m, polymorphic_identity='manager', include_properties=('id', 'type')) @@ -506,8 +507,12 @@ class MapperTest(_fixtures.FixtureTest): hd_m = mapper(HasDef, t, column_prefix="h_") + fb_m = mapper(Fub, t, include_properties=(t.c.id, t.c.type)) + frb_m = mapper(Frob, t, column_prefix='f_', + exclude_properties=(t.c.boss_id, + 'employee_number', t.c.vendor_id)) + p_m.compile() - #sa.orm.compile_mappers() def assert_props(cls, want): have = set([n for n in dir(cls) if not n.startswith('_')]) @@ -519,7 +524,8 @@ class MapperTest(_fixtures.FixtureTest): want = set(want) eq_(have, want) - assert_props(HasDef, ['h_boss_id', 'h_employee_number', 'h_id', 'name', 'h_name', 'h_vendor_id', 'h_type']) + assert_props(HasDef, ['h_boss_id', 'h_employee_number', 'h_id', + 'name', 'h_name', 'h_vendor_id', 'h_type']) assert_props(Person, ['id', 'name', 'type']) assert_instrumented(Person, ['id', 'name', 'type']) assert_props(Employee, ['boss', 'boss_id', 'employee_number', @@ -535,24 +541,57 @@ class MapperTest(_fixtures.FixtureTest): assert_props(Vendor, ['vendor_id', 'id', 'name', 'type']) assert_props(Hoho, ['id', 'name', 'type']) assert_props(Lala, ['p_employee_number', 'p_id', 'p_name', 'p_type']) - + assert_props(Fub, ['id', 'type']) + assert_props(Frob, ['f_id', 'f_type', 'f_name', ]) # excluding the discriminator column is currently not allowed class Foo(Person): pass - assert_raises(sa.exc.InvalidRequestError, mapper, Foo, inherits=Person, polymorphic_identity='foo', exclude_properties=('type',) ) + + assert_raises( + sa.exc.InvalidRequestError, + mapper, + Foo, inherits=Person, polymorphic_identity='foo', + exclude_properties=('type', ), + ) @testing.resolve_artifact_names - def test_mapping_to_join(self): - """Mapping to a join""" + def test_mapping_to_join_raises(self): + """Test implicit merging of two cols warns.""" + usersaddresses = sa.join(users, addresses, users.c.id == addresses.c.user_id) - mapper(User, usersaddresses, primary_key=[users.c.id]) + assert_raises_message( + sa.exc.SAWarning, + "Implicitly", + mapper, User, usersaddresses, primary_key=[users.c.id] + ) + sa.orm.clear_mappers() + + @testing.emits_warning(r'Implicitly') + def go(): + # but it works despite the warning + mapper(User, usersaddresses, primary_key=[users.c.id]) + l = create_session().query(User).order_by(users.c.id).all() + eq_(l, self.static.user_result[:3]) + go() + + @testing.resolve_artifact_names + def test_mapping_to_join(self): + """Mapping to a join""" + + usersaddresses = sa.join(users, addresses, users.c.id + == addresses.c.user_id) + mapper(User, usersaddresses, primary_key=[users.c.id], + exclude_properties=[addresses.c.id]) l = create_session().query(User).order_by(users.c.id).all() eq_(l, self.static.user_result[:3]) @testing.resolve_artifact_names def test_mapping_to_join_no_pk(self): - m = mapper(Address, addresses.join(email_bounces)) + m = mapper(Address, + addresses.join(email_bounces), + properties={'id':[addresses.c.id, email_bounces.c.id]} + ) m.compile() assert addresses in m._pks_by_table assert email_bounces not in m._pks_by_table -- 2.47.2