From: Mike Bayer Date: Sun, 21 Feb 2016 01:22:38 +0000 (-0500) Subject: - reworked the way the "select_wraps_for" expression is X-Git-Tag: rel_1_1_0b1~98^2~16 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8ad968f33100baeb3b13c7e0b724b6b79ab4277f;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git - reworked the way the "select_wraps_for" expression is handled within visit_select(); this attribute was added in the 1.0 series to accommodate the subquery wrapping behavior of SQL Server and Oracle while also working with positional column targeting and no longer relying upon "key fallback" in order to target columns in such a statement. The IBM DB2 third-party dialect also has this use case, but its implementation is using regular expressions to rewrite the textual SELECT only and does not make use of a "wrapped" select at this time. The logic no longer attempts to reconcile proxy set collections as this was not deterministic, and instead assumes that the select() and the wrapper select() match their columns postionally, at least for the column positions they have in common, so it is now very simple and safe. fixes #3657. - as a side effect of #3657 it was also revealed that the strategy of calling upon a ResultProxy._getter was not correctly calling into NoSuchColumnError when an expected column was not present, and instead returned None up to loading.instances() to produce NoneType failures; added a raiseerr argument to _getter() which is called when we aren't expecting None, fixes #3658. --- diff --git a/doc/build/changelog/changelog_10.rst b/doc/build/changelog/changelog_10.rst index 1e10b97d04..fa1be4c16e 100644 --- a/doc/build/changelog/changelog_10.rst +++ b/doc/build/changelog/changelog_10.rst @@ -15,6 +15,32 @@ .. include:: changelog_07.rst :start-line: 5 +.. changelog:: + :version: 1.0.13 + + .. change:: + :tags: bug, orm + :tickets: 3658 + + Fixed regression appearing in the 1.0 series in ORM loading where the + exception raised for an expected column missing would incorrectly + be a ``NoneType`` error, rather than the expected + :class:`.NoSuchColumnError`. + + .. change:: + :tags: bug, mssql, oracle + :tickets: 3657 + + Fixed regression appearing in the 1.0 series which would cause the Oracle + and SQL Server dialects to incorrectly account for result set columns + when these dialects would wrap a SELECT in a subquery in order to + provide LIMIT/OFFSET behavior, and the original SELECT statement + referred to the same column multiple times, such as a column and + a label of that same column. This issue is related + to :ticket:`3658` in that when the error occurred, it would also + cause a ``NoneType`` error, rather than reporting that it couldn't + locate a column. + .. changelog:: :version: 1.0.12 :released: February 15, 2016 diff --git a/lib/sqlalchemy/engine/result.py b/lib/sqlalchemy/engine/result.py index c069fcedf2..f09b0b40b3 100644 --- a/lib/sqlalchemy/engine/result.py +++ b/lib/sqlalchemy/engine/result.py @@ -561,11 +561,11 @@ class ResultMetaData(object): else: return self._key_fallback(key, False) is not None - def _getter(self, key): + def _getter(self, key, raiseerr=True): if key in self._keymap: processor, obj, index = self._keymap[key] else: - ret = self._key_fallback(key, False) + ret = self._key_fallback(key, raiseerr) if ret is None: return None processor, obj, index = ret @@ -640,13 +640,13 @@ class ResultProxy(object): context.engine._should_log_debug() self._init_metadata() - def _getter(self, key): + def _getter(self, key, raiseerr=True): try: getter = self._metadata._getter except AttributeError: return self._non_result(None) else: - return getter(key) + return getter(key, raiseerr) def _has_key(self, key): try: diff --git a/lib/sqlalchemy/orm/loading.py b/lib/sqlalchemy/orm/loading.py index b8bc205d48..d457f3c637 100644 --- a/lib/sqlalchemy/orm/loading.py +++ b/lib/sqlalchemy/orm/loading.py @@ -316,7 +316,7 @@ def _instance_processor( else: if adapter: col = adapter.columns[col] - getter = result._getter(col) + getter = result._getter(col, False) if getter: populators["quick"].append((prop.key, getter)) else: diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index 7d816e6261..370cb974b5 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -174,7 +174,7 @@ class ColumnLoader(LoaderStrategy): for col in self.columns: if adapter: col = adapter.columns[col] - getter = result._getter(col) + getter = result._getter(col, False) if getter: populators["quick"].append((self.key, getter)) break diff --git a/lib/sqlalchemy/sql/compiler.py b/lib/sqlalchemy/sql/compiler.py index a2fc0fe68a..b75dc1c076 100644 --- a/lib/sqlalchemy/sql/compiler.py +++ b/lib/sqlalchemy/sql/compiler.py @@ -1638,15 +1638,11 @@ class SQLCompiler(Compiled): if populate_result_map and select_wraps_for is not None: # if this select is a compiler-generated wrapper, # rewrite the targeted columns in the result map - wrapped_inner_columns = set(select_wraps_for.inner_columns) + translate = dict( - (outer, inner.pop()) for outer, inner in [ - ( - outer, - outer.proxy_set.intersection(wrapped_inner_columns)) - for outer in select.inner_columns - ] if inner + zip(select.inner_columns, select_wraps_for.inner_columns) ) + self._result_columns = [ (key, name, tuple(translate.get(o, o) for o in obj), type_) for key, name, obj, type_ in self._result_columns diff --git a/test/dialect/mssql/test_compiler.py b/test/dialect/mssql/test_compiler.py index d91c79db25..b59ca4fd10 100644 --- a/test/dialect/mssql/test_compiler.py +++ b/test/dialect/mssql/test_compiler.py @@ -1,5 +1,5 @@ # -*- encoding: utf-8 -from sqlalchemy.testing import eq_ +from sqlalchemy.testing import eq_, is_ from sqlalchemy import schema from sqlalchemy.sql import table, column from sqlalchemy.databases import mssql @@ -521,6 +521,30 @@ class CompileTest(fixtures.TestBase, AssertsCompiledSQL): assert t.c.x in set(c._create_result_map()['x'][1]) assert t.c.y in set(c._create_result_map()['y'][1]) + def test_limit_offset_w_ambiguous_cols(self): + t = table('t', column('x', Integer), column('y', Integer)) + + cols = [t.c.x, t.c.x.label('q'), t.c.x.label('p'), t.c.y] + s = select(cols).where(t.c.x == 5).order_by(t.c.y).limit(10).offset(20) + + self.assert_compile( + s, + "SELECT anon_1.x, anon_1.q, anon_1.p, anon_1.y " + "FROM (SELECT t.x AS x, t.x AS q, t.x AS p, t.y AS y, " + "ROW_NUMBER() OVER (ORDER BY t.y) AS mssql_rn " + "FROM t " + "WHERE t.x = :x_1) AS anon_1 " + "WHERE mssql_rn > :param_1 AND mssql_rn <= :param_2 + :param_1", + checkparams={'param_1': 20, 'param_2': 10, 'x_1': 5} + ) + c = s.compile(dialect=mssql.MSDialect()) + eq_(len(c._result_columns), 4) + + result_map = c._create_result_map() + + for col in cols: + is_(result_map[col.key][1][0], col) + def test_limit_offset_with_correlated_order_by(self): t1 = table('t1', column('x', Integer), column('y', Integer)) t2 = table('t2', column('x', Integer), column('y', Integer)) diff --git a/test/orm/test_loading.py b/test/orm/test_loading.py index f86477ec29..6f3f6a016f 100644 --- a/test/orm/test_loading.py +++ b/test/orm/test_loading.py @@ -1,8 +1,11 @@ from . import _fixtures from sqlalchemy.orm import loading, Session, aliased -from sqlalchemy.testing.assertions import eq_, assert_raises +from sqlalchemy.testing.assertions import eq_, \ + assert_raises, assert_raises_message from sqlalchemy.util import KeyedTuple from sqlalchemy.testing import mock +from sqlalchemy import select +from sqlalchemy import exc # class GetFromIdentityTest(_fixtures.FixtureTest): # class LoadOnIdentTest(_fixtures.FixtureTest): # class InstanceProcessorTest(_fixture.FixtureTest): @@ -34,6 +37,19 @@ class InstancesTest(_fixtures.FixtureTest): ) assert cursor.close.called, "Cursor wasn't closed" + def test_row_proc_not_created(self): + User = self.classes.User + s = Session() + + q = s.query(User.id, User.name) + stmt = select([User.id]) + + assert_raises_message( + exc.NoSuchColumnError, + "Could not locate column in row for column 'users.name'", + q.from_statement(stmt).all + ) + class MergeResultTest(_fixtures.FixtureTest): run_setup_mappers = 'once' diff --git a/test/sql/test_compiler.py b/test/sql/test_compiler.py index f11a4e61af..1c5dc23402 100644 --- a/test/sql/test_compiler.py +++ b/test/sql/test_compiler.py @@ -3855,3 +3855,26 @@ class ResultMapTest(fixtures.TestBase): (table1.c.description, 'description', 'description'), table1.c.description.type)} ) + + def test_select_wraps_for_translate_ambiguity(self): + # test for issue #3657 + t = table('a', column('x'), column('y'), column('z')) + + l1, l2, l3 = t.c.z.label('a'), t.c.x.label('b'), t.c.x.label('c') + orig = [t.c.x, t.c.y, l1, l2, l3] + stmt = select(orig) + wrapped = stmt._generate() + wrapped = wrapped.column( + func.ROW_NUMBER().over(order_by=t.c.z)).alias() + + wrapped_again = select([c for c in wrapped.c]) + + compiled = wrapped_again.compile( + compile_kwargs={'select_wraps_for': stmt}) + + proxied = [obj[0] for (k, n, obj, type_) in compiled._result_columns] + for orig_obj, proxied_obj in zip( + orig, + proxied + ): + is_(orig_obj, proxied_obj) diff --git a/test/sql/test_resultset.py b/test/sql/test_resultset.py index bd2b8c0aeb..aaeb82fa4c 100644 --- a/test/sql/test_resultset.py +++ b/test/sql/test_resultset.py @@ -204,7 +204,8 @@ class ResultProxyTest(fixtures.TablesTest): lambda: result[0][addresses.c.address_id]) def test_column_error_printing(self): - row = testing.db.execute(select([1])).first() + result = testing.db.execute(select([1])) + row = result.first() class unprintable(object): @@ -219,6 +220,14 @@ class ResultProxyTest(fixtures.TablesTest): (Column("q", Integer) + 12, r"q \+ :q_1"), (unprintable(), "unprintable element.*"), ]: + assert_raises_message( + exc.NoSuchColumnError, + msg % repl, + result._getter, accessor + ) + + is_(result._getter(accessor, False), None) + assert_raises_message( exc.NoSuchColumnError, msg % repl,