deprecate direct Text() conversion in _literal_as_text?
The filter()
method accepts string arguments and uses them as raw SQL. This is unexpected, because everywhere else, only strings wrapped in text()
are used as-is.
I consider this a bug, because it is a very questionable feature to allow raw SQL implicitly.
Comments (13)
-
repo owner -
repo owner - changed status to wontfix
whether or not this feature is desirable there's really no option to change this behavior at this point. specific examples of where a bound value would be preferable over text() should be separate issues.
-
repo owner - changed status to open
let's examine ways to deprecate the implicit text() for 1.0
-
repo owner - changed milestone to 1.0
- changed component to sql
- changed title to deprecate direct Text() conversion in _literal_as_text?
- marked as proposal
asking around to see what the appetite would be for the implicit string->text() behavior to be deprecated.
This change would really mess people up, not as much on filter()/where(), but on order_by(). it applies here too:
s = stmt.order_by("value")
it should be easy to control at least I'm pretty sure _literal_as_text() is the only point where this translation occurs.
-
reporter I think perhaps a valid identifier might be okay, e.g. a column name. That is, something that can be resolved to a column.
-
repo owner we don't have the feature that order_by() etc. actually attempt to resolve the given string to a column label known to be in the statement. If you are selecting from table.c.foo, if you want to order by that, you say order_by(table.c.foo). The practice of doing order_by("foo") is problematic also when the ORM does query rewriting in any case and I try to discourage it for that reason.
-
repo owner - attached 2992.patch
let's see if we can actually do a name resolution on order by and group by. this would allow column translations to work on those too. the approach here moves the text() warning to compile time, as that's the most efficient / guaranteed to work time when we can resolve order_by/group_by names.
lots of tests needed including for the warnings, for the order_by/group_by lookups, to see if translations really work, etc. probably can go in test_compiler but should be its own suite.
-
repo owner first branch rev is at 5b0ac2748004ad091f9067.
here's scenarios we need turned into tests:
from sqlalchemy.sql import table, column, select, text, func, desc, asc t1 = table('t1', column('a'), column('b')) s1 = select([t1.c.b]).order_by(asc('a'), 'b') print s1 s2 = select([t1]).alias() print select([s2]).apply_labels().group_by('a') s3 = select([func.foo('bar').label('fb'), t1]).order_by(desc('fb')) print s3 s4 = select([t1]).distinct('b') from sqlalchemy.dialects import postgresql print s4.compile(dialect=postgresql.dialect())
from sqlalchemy import * from sqlalchemy.orm import * from sqlalchemy.ext.declarative import declarative_base Base = declarative_base() class A(Base): __tablename__ = 'a' id = Column(Integer, primary_key=True) bs = relationship("B") class B(Base): __tablename__ = 'b' id = Column(Integer, primary_key=True) a_id = Column(Integer, ForeignKey('a.id')) sess = Session(e) print sess.query(A).join(B).order_by("a_id") print sess.query(A, B, func.x().label('g')).join(B).order_by("a_id", "b_id", "g")
-
reporter Nice!
Ideally, the warning would be "pluggable" (to allow a custom behavior that simply raises an exception), but it's definitely much better than the previous behavior.
-
repo owner that's implicit in warnings as the warnings filter can promote them to exceptions.
the issue with this warning is that it would be really helpful to put the text expression detected in the warning. However, that means the Python warnings filter will cache that unique string permanently, so an app that puts arbitrary text will eventually run out of memory.
-
repo owner going to fix that with
#3178 -
repo owner - changed status to resolved
- The :func:
~.expression.column
and :func:~.expression.table
constructs are now importable from the "from sqlalchemy" namespace, just like every other Core construct. - The implicit conversion of strings to :func:
.text
constructs when passed to most builder methods of :func:.select
as well as :class:.Query
now emits a warning with just the plain string sent. The textual conversion still proceeds normally, however. The only method that accepts a string without a warning are the "label reference" methods like order_by(), group_by(); these functions will now at compile time attempt to resolve a single string argument to a column or label expression present in the selectable; if none is located, the expression still renders, but you get the warning again. The rationale here is that the implicit conversion from string to text is more unexpected than not these days, and it is better that the user send more direction to the Core / ORM when passing a raw string as to what direction should be taken. Core/ORM tutorials have been updated to go more in depth as to how text is handled. fixes#2992
→ <<cset 7c6a45c480a8>>
-
repo owner that was a big damn deal
- Log in to comment
strings are accepted by most query/select methods, and in those cases are coerced to text() implicitly. The case where strings are converted to bound parameters is when they are used in expressions. This behavior should be pretty consistent across the board, and it is widely documented:
http://docs.sqlalchemy.org/en/rel_0_9/core/tutorial.html#using-text
http://docs.sqlalchemy.org/en/rel_0_9/orm/tutorial.html#using-literal-sql
this behavior has been part of SQLAlchemy for its entire lifespan.
raw SQL is something that should be handled carefully but libraries certainly accept it, our users would be pretty upset if connection.execute() for example no longer accepted a string.