diff options
| -rw-r--r-- | doc/build/changelog/changelog_08.rst | 21 | ||||
| -rw-r--r-- | lib/sqlalchemy/engine/result.py | 2 | ||||
| -rw-r--r-- | lib/sqlalchemy/sql/compiler.py | 34 | ||||
| -rw-r--r-- | lib/sqlalchemy/sql/expression.py | 46 | ||||
| -rw-r--r-- | lib/sqlalchemy/util/_collections.py | 1 | ||||
| -rw-r--r-- | test/aaa_profiling/test_compiler.py | 12 | ||||
| -rw-r--r-- | test/orm/test_froms.py | 61 | ||||
| -rw-r--r-- | test/profiles.txt | 4 | ||||
| -rw-r--r-- | test/sql/test_query.py | 16 | ||||
| -rw-r--r-- | test/sql/test_selectable.py | 153 |
10 files changed, 326 insertions, 24 deletions
diff --git a/doc/build/changelog/changelog_08.rst b/doc/build/changelog/changelog_08.rst index cbc849095..4ac370cf9 100644 --- a/doc/build/changelog/changelog_08.rst +++ b/doc/build/changelog/changelog_08.rst @@ -7,6 +7,27 @@ :version: 0.8.1 .. change:: + :tags: bug, core + :tickets: 2702 + + A major fix to the way in which a select() object produces + labeled columns when apply_labels() is used; this mode + produces a SELECT where each column is labeled as in + <tablename>_<columnname>, to remove column name collisions + for a multiple table select. The fix is that if two labels + collide when combined with the table name, i.e. + "foo.bar_id" and "foo_bar.id", anonymous aliasing will be + applied to one of the dupes. This allows the ORM to handle + both columns independently; previously, 0.7 + would in some cases silently emit a second SELECT for the + column that was "duped", and in 0.8 an ambiguous column error + would be emitted. The "keys" applied to the .c. collection + of the select() will also be deduped, so that the "column + being replaced" warning will no longer emit for any select() + that specifies use_labels, though the dupe key will be given + an anonymous label which isn't generally user-friendly. + + .. change:: :tags: bug, orm, declarative :tickets: 2656 diff --git a/lib/sqlalchemy/engine/result.py b/lib/sqlalchemy/engine/result.py index 7572564bb..1c148e1f0 100644 --- a/lib/sqlalchemy/engine/result.py +++ b/lib/sqlalchemy/engine/result.py @@ -301,7 +301,7 @@ class ResultMetaData(object): # this check isn't currently available if the row # was unpickled. if result is not None and \ - result[1] is not None: + result[1] is not None: for obj in result[1]: if key._compare_name_for_result(obj): break diff --git a/lib/sqlalchemy/sql/compiler.py b/lib/sqlalchemy/sql/compiler.py index 5a3a92a3e..b902f9ffc 100644 --- a/lib/sqlalchemy/sql/compiler.py +++ b/lib/sqlalchemy/sql/compiler.py @@ -992,13 +992,15 @@ class SQLCompiler(engine.Compiled): else: self.result_map[keyname] = name, objects, type_ - def _label_select_column(self, select, column, populate_result_map, + def _label_select_column(self, select, column, + populate_result_map, asfrom, column_clause_args, + name=None, within_columns_clause=True): """produce labeled columns present in a select().""" if column.type._has_column_expression and \ - populate_result_map: + populate_result_map: col_expr = column.type.column_expression(column) add_to_result_map = lambda keyname, name, objects, type_: \ self._add_to_result_map( @@ -1023,13 +1025,11 @@ class SQLCompiler(engine.Compiled): else: result_expr = col_expr - elif select is not None and \ - select.use_labels and \ - column._label: + elif select is not None and name: result_expr = _CompileLabel( col_expr, - column._label, - alt_names=(column._key_label, ) + name, + alt_names=(column._key_label,) ) elif \ @@ -1037,7 +1037,7 @@ class SQLCompiler(engine.Compiled): isinstance(column, sql.ColumnClause) and \ not column.is_literal and \ column.table is not None and \ - not isinstance(column.table, sql.Select): + not isinstance(column.table, sql.Select): result_expr = _CompileLabel(col_expr, sql._as_truncated(column.name), alt_names=(column.key,)) @@ -1098,11 +1098,11 @@ class SQLCompiler(engine.Compiled): # correlate_froms.union(existingfroms) populate_result_map = force_result_map or ( - compound_index == 0 and ( - not entry or \ - entry.get('iswrapper', False) - ) - ) + compound_index == 0 and ( + not entry or \ + entry.get('iswrapper', False) + ) + ) self.stack.append({'from': correlate_froms, 'iswrapper': iswrapper}) @@ -1117,10 +1117,12 @@ class SQLCompiler(engine.Compiled): # the actual list of columns to print in the SELECT column list. inner_columns = [ c for c in [ - self._label_select_column(select, column, + self._label_select_column(select, + column, populate_result_map, asfrom, - column_clause_args) - for column in util.unique_list(select.inner_columns) + column_clause_args, + name=name) + for name, column in select._columns_plus_names ] if c is not None ] diff --git a/lib/sqlalchemy/sql/expression.py b/lib/sqlalchemy/sql/expression.py index 5cef778bb..28b1c6ddd 100644 --- a/lib/sqlalchemy/sql/expression.py +++ b/lib/sqlalchemy/sql/expression.py @@ -5781,13 +5781,47 @@ class Select(HasPrefixes, SelectBase): fromclause = _interpret_as_from(fromclause) self._from_obj = self._from_obj.union([fromclause]) + + @_memoized_property + def _columns_plus_names(self): + if self.use_labels: + names = set() + def name_for_col(c): + if c._label is None: + return (None, c) + name = c._label + if name in names: + name = c.anon_label + else: + names.add(name) + return name, c + + return [ + name_for_col(c) + for c in util.unique_list(_select_iterables(self._raw_columns)) + ] + else: + return [ + (None, c) + for c in util.unique_list(_select_iterables(self._raw_columns)) + ] + def _populate_column_collection(self): - for c in self.inner_columns: - if hasattr(c, '_make_proxy'): - c._make_proxy(self, - name=c._label if self.use_labels else None, - key=c._key_label if self.use_labels else None, - name_is_truncatable=True) + for name, c in self._columns_plus_names: + if not hasattr(c, '_make_proxy'): + continue + if name is None: + key = None + elif self.use_labels: + key = c._key_label + if key is not None and key in self.c: + key = c.anon_label + else: + key = None + + c._make_proxy(self, key=key, + name=name, + name_is_truncatable=True) def _refresh_for_new_column(self, column): for fromclause in self._froms: diff --git a/lib/sqlalchemy/util/_collections.py b/lib/sqlalchemy/util/_collections.py index 2c9c982fb..8e61275e7 100644 --- a/lib/sqlalchemy/util/_collections.py +++ b/lib/sqlalchemy/util/_collections.py @@ -672,7 +672,6 @@ column_dict = dict ordered_column_set = OrderedSet populate_column_dict = PopulateDict - def unique_list(seq, hashfunc=None): seen = {} if not hashfunc: diff --git a/test/aaa_profiling/test_compiler.py b/test/aaa_profiling/test_compiler.py index 2776f05ab..1b7798d06 100644 --- a/test/aaa_profiling/test_compiler.py +++ b/test/aaa_profiling/test_compiler.py @@ -60,4 +60,16 @@ class CompileTest(fixtures.TestBase, AssertsExecutionResults): def go(): s = select([t1], t1.c.c2 == t2.c.c1) s.compile(dialect=self.dialect) + go() + + def test_select_labels(self): + # give some of the cached type values + # a chance to warm up + s = select([t1], t1.c.c2 == t2.c.c1).apply_labels() + s.compile(dialect=self.dialect) + + @profiling.function_call_count() + def go(): + s = select([t1], t1.c.c2 == t2.c.c1).apply_labels() + s.compile(dialect=self.dialect) go()
\ No newline at end of file diff --git a/test/orm/test_froms.py b/test/orm/test_froms.py index b98333e3d..c701a7076 100644 --- a/test/orm/test_froms.py +++ b/test/orm/test_froms.py @@ -2320,3 +2320,64 @@ class TestOverlyEagerEquivalentCols(fixtures.MappedTest): filter(Sub1.id==1).one(), b1 ) + +class LabelCollideTest(fixtures.MappedTest): + """Test handling for a label collision. This collision + is handled by core, see ticket:2702 as well as + test/sql/test_selectable->WithLabelsTest. here we want + to make sure the end result is as we expect. + + """ + + @classmethod + def define_tables(cls, metadata): + Table('foo', metadata, + Column('id', Integer, primary_key=True), + Column('bar_id', Integer) + ) + Table('foo_bar', metadata, + Column('id', Integer, primary_key=True), + ) + + @classmethod + def setup_classes(cls): + class Foo(cls.Basic): + pass + class Bar(cls.Basic): + pass + + @classmethod + def setup_mappers(cls): + mapper(cls.classes.Foo, cls.tables.foo) + mapper(cls.classes.Bar, cls.tables.foo_bar) + + @classmethod + def insert_data(cls): + s = Session() + s.add_all([ + cls.classes.Foo(id=1, bar_id=2), + cls.classes.Bar(id=3) + ]) + s.commit() + + def test_overlap_plain(self): + s = Session() + row = s.query(self.classes.Foo, self.classes.Bar).all()[0] + def go(): + eq_(row.Foo.id, 1) + eq_(row.Foo.bar_id, 2) + eq_(row.Bar.id, 3) + # all three columns are loaded independently without + # overlap, no additional SQL to load all attributes + self.assert_sql_count(testing.db, go, 0) + + def test_overlap_subquery(self): + s = Session() + row = s.query(self.classes.Foo, self.classes.Bar).from_self().all()[0] + def go(): + eq_(row.Foo.id, 1) + eq_(row.Foo.bar_id, 2) + eq_(row.Bar.id, 3) + # all three columns are loaded independently without + # overlap, no additional SQL to load all attributes + self.assert_sql_count(testing.db, go, 0)
\ No newline at end of file diff --git a/test/profiles.txt b/test/profiles.txt index b119886bb..d465fa3be 100644 --- a/test/profiles.txt +++ b/test/profiles.txt @@ -33,6 +33,10 @@ test.aaa_profiling.test_compiler.CompileTest.test_select 2.7_postgresql_psycopg2 test.aaa_profiling.test_compiler.CompileTest.test_select 2.7_sqlite_pysqlite_cextensions 135 test.aaa_profiling.test_compiler.CompileTest.test_select 2.7_sqlite_pysqlite_nocextensions 135 +# TEST: test.aaa_profiling.test_compiler.CompileTest.test_select_labels + +test.aaa_profiling.test_compiler.CompileTest.test_select_labels 2.7_sqlite_pysqlite_nocextensions 177 + # TEST: test.aaa_profiling.test_compiler.CompileTest.test_update test.aaa_profiling.test_compiler.CompileTest.test_update 2.5_sqlite_pysqlite_nocextensions 65 diff --git a/test/sql/test_query.py b/test/sql/test_query.py index a61363378..293e629c8 100644 --- a/test/sql/test_query.py +++ b/test/sql/test_query.py @@ -1028,6 +1028,22 @@ class QueryTest(fixtures.TestBase): lambda: row[u2.c.user_id] ) + def test_ambiguous_column_contains(self): + # ticket 2702. in 0.7 we'd get True, False. + # in 0.8, both columns are present so it's True; + # but when they're fetched you'll get the ambiguous error. + users.insert().execute(user_id=1, user_name='john') + result = select([ + users.c.user_id, + addresses.c.user_id]).\ + select_from(users.outerjoin(addresses)).execute() + row = result.first() + + eq_( + set([users.c.user_id in row, addresses.c.user_id in row]), + set([True]) + ) + def test_ambiguous_column_by_col_plus_label(self): users.insert().execute(user_id=1, user_name='john') result = select([users.c.user_id, diff --git a/test/sql/test_selectable.py b/test/sql/test_selectable.py index 30052a806..e881298a7 100644 --- a/test/sql/test_selectable.py +++ b/test/sql/test_selectable.py @@ -1587,3 +1587,156 @@ class AnnotationsTest(fixtures.TestBase): comp2 = c2.comparator assert (c2 == 5).left._annotations == {"foo": "bar", "bat": "hoho"} + +class WithLabelsTest(fixtures.TestBase): + def _assert_labels_warning(self, s): + assert_raises_message( + exc.SAWarning, + "replaced by another column with the same key", + lambda: s.c + ) + + def _assert_result_keys(self, s, keys): + compiled = s.compile() + eq_(set(compiled.result_map), set(keys)) + + def _assert_subq_result_keys(self, s, keys): + compiled = s.select().compile() + eq_(set(compiled.result_map), set(keys)) + + def _names_overlap(self): + m = MetaData() + t1 = Table('t1', m, Column('x', Integer)) + t2 = Table('t2', m, Column('x', Integer)) + return select([t1, t2]) + + def test_names_overlap_nolabel(self): + sel = self._names_overlap() + self._assert_labels_warning(sel) + self._assert_result_keys(sel, ['x']) + + def test_names_overlap_label(self): + sel = self._names_overlap().apply_labels() + eq_( + sel.c.keys(), + ['t1_x', 't2_x'] + ) + self._assert_result_keys(sel, ['t1_x', 't2_x']) + + def _names_overlap_keys_dont(self): + m = MetaData() + t1 = Table('t1', m, Column('x', Integer, key='a')) + t2 = Table('t2', m, Column('x', Integer, key='b')) + return select([t1, t2]) + + def test_names_overlap_keys_dont_nolabel(self): + sel = self._names_overlap_keys_dont() + eq_( + sel.c.keys(), + ['a', 'b'] + ) + self._assert_result_keys(sel, ['x']) + + def test_names_overlap_keys_dont_label(self): + sel = self._names_overlap_keys_dont().apply_labels() + eq_( + sel.c.keys(), + ['t1_a', 't2_b'] + ) + self._assert_result_keys(sel, ['t1_x', 't2_x']) + + def _labels_overlap(self): + m = MetaData() + t1 = Table('t', m, Column('x_id', Integer)) + t2 = Table('t_x', m, Column('id', Integer)) + return select([t1, t2]) + + def test_labels_overlap_nolabel(self): + sel = self._labels_overlap() + eq_( + sel.c.keys(), + ['x_id', 'id'] + ) + self._assert_result_keys(sel, ['x_id', 'id']) + + def test_labels_overlap_label(self): + sel = self._labels_overlap().apply_labels() + t2 = sel.froms[1] + eq_( + sel.c.keys(), + ['t_x_id', t2.c.id.anon_label] + ) + self._assert_result_keys(sel, ['t_x_id', 'id_1']) + self._assert_subq_result_keys(sel, ['t_x_id', 'id_1']) + + def _labels_overlap_keylabels_dont(self): + m = MetaData() + t1 = Table('t', m, Column('x_id', Integer, key='a')) + t2 = Table('t_x', m, Column('id', Integer, key='b')) + return select([t1, t2]) + + def test_labels_overlap_keylabels_dont_nolabel(self): + sel = self._labels_overlap_keylabels_dont() + eq_(sel.c.keys(), ['a', 'b']) + self._assert_result_keys(sel, ['x_id', 'id']) + + def test_labels_overlap_keylabels_dont_label(self): + sel = self._labels_overlap_keylabels_dont().apply_labels() + eq_(sel.c.keys(), ['t_a', 't_x_b']) + self._assert_result_keys(sel, ['t_x_id', 'id_1']) + + def _keylabels_overlap_labels_dont(self): + m = MetaData() + t1 = Table('t', m, Column('a', Integer, key='x_id')) + t2 = Table('t_x', m, Column('b', Integer, key='id')) + return select([t1, t2]) + + def test_keylabels_overlap_labels_dont_nolabel(self): + sel = self._keylabels_overlap_labels_dont() + eq_(sel.c.keys(), ['x_id', 'id']) + self._assert_result_keys(sel, ['a', 'b']) + + def test_keylabels_overlap_labels_dont_label(self): + sel = self._keylabels_overlap_labels_dont().apply_labels() + t2 = sel.froms[1] + eq_(sel.c.keys(), ['t_x_id', t2.c.id.anon_label]) + self._assert_result_keys(sel, ['t_a', 't_x_b']) + self._assert_subq_result_keys(sel, ['t_a', 't_x_b']) + + def _keylabels_overlap_labels_overlap(self): + m = MetaData() + t1 = Table('t', m, Column('x_id', Integer, key='x_a')) + t2 = Table('t_x', m, Column('id', Integer, key='a')) + return select([t1, t2]) + + def test_keylabels_overlap_labels_overlap_nolabel(self): + sel = self._keylabels_overlap_labels_overlap() + eq_(sel.c.keys(), ['x_a', 'a']) + self._assert_result_keys(sel, ['x_id', 'id']) + self._assert_subq_result_keys(sel, ['x_id', 'id']) + + def test_keylabels_overlap_labels_overlap_label(self): + sel = self._keylabels_overlap_labels_overlap().apply_labels() + t2 = sel.froms[1] + eq_(sel.c.keys(), ['t_x_a', t2.c.a.anon_label]) + self._assert_result_keys(sel, ['t_x_id', 'id_1']) + self._assert_subq_result_keys(sel, ['t_x_id', 'id_1']) + + def _keys_overlap_names_dont(self): + m = MetaData() + t1 = Table('t1', m, Column('a', Integer, key='x')) + t2 = Table('t2', m, Column('b', Integer, key='x')) + return select([t1, t2]) + + def test_keys_overlap_names_dont_nolabel(self): + sel = self._keys_overlap_names_dont() + self._assert_labels_warning(sel) + self._assert_result_keys(sel, ['a', 'b']) + + def test_keys_overlap_names_dont_label(self): + sel = self._keys_overlap_names_dont().apply_labels() + eq_( + sel.c.keys(), + ['t1_x', 't2_x'] + ) + self._assert_result_keys(sel, ['t1_a', 't2_b']) |