Re: [sqlalchemy] [PATCHES] _compiler_dispatch in visit_delete and FROM ONLY support for postgres
On Sat, Jun 09, 2012 at 11:56:45PM -0400, Michael Bayer wrote:
for _compiler_dispatch, it looks mostly OK - i think isselect isn't needed?
Or if it's meant to accommodate a SELECT that's inside of an UPDATE or
DELETE, I think you'd need to use a stack based approach so that when you
exit visit_select() the isselect flag is reset. The
isdelete/isupdate/isinsert flags are meant to be mutually exclusive. I'd
just peek inside of self.stack, perhaps via the is_subquery() method, or
something more specific, if you truly need to detect SELECT inside of
UPDATE/DELETE for that. Looks OK otherwise.
Yes. is_subquery() is just what I needed.
only() remains more troubling. I can see the difficulty, it's somewhere
in between a hint, and a directive that actually produces a different
result, which is unlike a hint. But using a new selectable construct adds
a large amount of complexity that I still see as unwarranted.
Creating a wrapper around a selectable requires that all the Column objects
be proxied into new ones. Column has a .table attribute that points to
the parent table - so you need to do more than just copy over table.columns
to self._columns - this breaks the Column which you can see if you run a
regular select against the table after the only():
from sqlalchemy import *
m = MetaData()
mytable = Table('mytable', m, Column('id', Integer, primary_key=True),
Column('data', String))
from sqlalchemy.dialects.postgresql import base as postgresql
mytable_only = postgresql.only(mytable)
print select([mytable_only.c.id,
mytable_only.c.data]).compile(dialect=postgresql.dialect())
print select([mytable.c.id,
mytable.c.data]).compile(dialect=postgresql.dialect())
output:
SELECT mytable.id, mytable.data
FROM ONLY mytable
SELECT mytable.id, mytable.data
FROM ONLY mytable
So at the very least, a construct like only() would usually subclass Alias
so that the column proxying is handled.
Also let's not forget that this feature needs to work with the ORM too. In
that case, I guess we'd need to use aliased(), something like:
only_of_class = aliased(MyClass, alias=only(MyClass.__table__))
Through all of this, only() as a separate construct still seems unwarranted
because as far as I can see, only() doesn't represent a new lexical identity
(which is the main job of a FROM element). That is, you wouldn't have
something like this:
SELECT * FROM ONLY mytable, mytable WHERE ...?
Assuming I'm understanding ONLY correctly, if you need the same table twice
in the statement, you still need to use alias(), so not much changes there
from how things work now (including when using with_hint(), which supports
aliases). When you say ONLY mytable, that isn't creating a new lexical
identity within the statement. You're modifying an existing lexical
identity, the identity denoted by the Table or Alias. So even though not
quite a hint, still acts a much more like a hint than a new lexical identity.
The attached patch illustrates some small changes to compiler that lets ONLY
come right through as a table hint, without the need to change much else.
Usage is like:
select([mytable.c.id, mytable.c.data]).with_hint(mytable, ONLY,
dialect_name=postgresql)
mytable_alias = mytable.alias()
select([mytable_alias.c.id, mytable_alias.c.data]).with_hint(mytable_alias,
ONLY, dialect_name=postgresql)
mytable.delete().with_hint(ONLY, dialect_name=postgresql)
This approach, while stretching the definition of hint a bit, makes usage
of existing functionality and uses an API that is already familiar in other
use cases, with no dialect import required and no complex table proxying
going on. We end up making with_hint() a more capable system rather than
adding an entirely new and narrowly-scoped system elsewhere.
If you approve, we can complete and clean up this approach, write some tests,
fix up the isselect thing, and all three patches can find their way in.
Well, since you've already done most of the hard work, the hints
approach seems to work perfectly fine :P
I'm attaching two new patches, one for the _compiler_dispatch in
visit_delete/visit_update, and the other for implementing FROM ONLY
using hints. Pretty similar to your patch but with some fixes to make
it work with UPDATE and DELETE as well as proper tests and
documentation.
-Ryan Kelly
On Jun 9, 2012, at 6:51 PM, Ryan Kelly wrote:
On Sat, May 05, 2012 at 08:00:20PM -0400, Michael Bayer wrote:
On May 5, 2012, at 7:33 PM, Ryan Kelly wrote:
List:
I am currently trying to add support for FROM ONLY to the postgresql
dialect. FROM ONLY is used when you wish to query a table which has other
tables that inherit from it, but you ONLY want results from the parent
table you are referencing. More information can be found here:
Re: [sqlalchemy] [PATCH] MATCH clause implementation for foreign keys
On Jun 9, 2012, at 10:41 PM, Michael Bayer wrote: it looks great. This is in the queue as http://www.sqlalchemy.org/trac/ticket/2502. 1765text += MATCH %s % constraint.match SQL injection? Shouldn't the argument be one of three constants? I suspect there needs to be some specific per-database-driver logic to handle unimplemented cases. PostgreSQL, for example, doesn't support MATCH PARTIAL ( http://www.postgresql.org/docs/9.1/static/sql-createtable.html ) and MySQL, naturally, completely ignores the syntax and triggers other clauses to be ignored: For users familiar with the ANSI/ISO SQL Standard, please note that no storage engine, including InnoDB, recognizes or enforces the MATCH clause used in referential integrity constraint definitions. Use of an explicit MATCH clause will not have the specified effect, and also causes ON DELETE and ON UPDATE clauses to be ignored. For these reasons, specifying MATCH should be avoided. http://dev.mysql.com/doc/refman/5.5/en/create-table.html Cheers, M -- You received this message because you are subscribed to the Google Groups sqlalchemy group. To post to this group, send email to sqlalchemy@googlegroups.com. To unsubscribe from this group, send email to sqlalchemy+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/sqlalchemy?hl=en.
Re: [sqlalchemy] [PATCH] MATCH clause implementation for foreign keys
On Sun, Jun 10, 2012 at 12:43:31PM -0400, A.M. wrote: On Jun 9, 2012, at 10:41 PM, Michael Bayer wrote: it looks great. This is in the queue as http://www.sqlalchemy.org/trac/ticket/2502. 1765 text += MATCH %s % constraint.match SQL injection? Shouldn't the argument be one of three constants? ON UPDATE, ON DELETE, DEFERRABLE, and INITIALLY all work this way. If this is broken, then we should fix those, too. And there are other places, like in the dialect-specific index parameters, that do this as well. I don't agree that it's a problem, however, because if we start saying what can appear there, we're necessarily limiting ourselves to the lowest common denominator. PostgreSQL, for instance, supports SET DEFAULT in ON UPDATE/DELETE, but MySQL does not. How do we handle that case? It seems like a lot of cruft would accumulate if we start specifying which values can go in these places. I suspect there needs to be some specific per-database-driver logic to handle unimplemented cases. PostgreSQL, for example, doesn't support MATCH PARTIAL ( http://www.postgresql.org/docs/9.1/static/sql-createtable.html ) This is correct. I do not believe Oracle does either. But PostgreSQL will courteously die with an error: ERROR: MATCH PARTIAL not yet implemented and MySQL, naturally, completely ignores the syntax and triggers other clauses to be ignored: For users familiar with the ANSI/ISO SQL Standard, please note that no storage engine, including InnoDB, recognizes or enforces the MATCH clause used in referential integrity constraint definitions. Use of an explicit MATCH clause will not have the specified effect, and also causes ON DELETE and ON UPDATE clauses to be ignored. For these reasons, specifying MATCH should be avoided. http://dev.mysql.com/doc/refman/5.5/en/create-table.html I really hope someone has filed a bug report against MySQL about this. The MySQL dialect can be patched to die with a CompileError (maybe) if MATCH is used, which seems like the sanest option, given that it causes unexpected behavior. I don't think the other dialects should be given the same treatment, however, because those will presumably error out if even given MATCH. Can someone test this on other databases to confirm? Cheers, M -- You received this message because you are subscribed to the Google Groups sqlalchemy group. To post to this group, send email to sqlalchemy@googlegroups.com. To unsubscribe from this group, send email to sqlalchemy+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/sqlalchemy?hl=en. -Ryan Kelly -- You received this message because you are subscribed to the Google Groups sqlalchemy group. To post to this group, send email to sqlalchemy@googlegroups.com. To unsubscribe from this group, send email to sqlalchemy+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/sqlalchemy?hl=en.
Re: [sqlalchemy] [PATCH] MATCH clause implementation for foreign keys
On Jun 10, 2012, at 12:43 PM, A.M. wrote: On Jun 9, 2012, at 10:41 PM, Michael Bayer wrote: it looks great. This is in the queue as http://www.sqlalchemy.org/trac/ticket/2502. 1765 text += MATCH %s % constraint.match SQL injection? Shouldn't the argument be one of three constants? This is not a SQL injection vector, for several reasons: 1. its a SQL keyword, not a literal value. Most database client APIs only consider literal values, not SQL keywords or schema identifiers, as candidates for bound values. While some DBAPIs, notably mysql-python, don't make use of real bound parameters and instead use Python string interpolation, technically they're doing the wrong thing. This ties into typical prepared statement mechanics which allow the database to separate the lexical aspect of a SQL statement from the data values. 2. not only is it a SQL keyword, its a keyword that's only used at DDL time. SQL injection IMO is not an issue for DDL - no sane application should ever be producing DDL at application run time, as the schema is part of the application's structure. 3. SQLAlchemy accepts textual SQL statements in many ways.The developer is not absolved from basic practices, just like putting user input into eval() should never be done, nor should user input be placed into the lexical portions of SQL statements. Bound values have special meaning within a statement which is where library-level interaction is warranted. In SQLAlchemy, we use literal strings for these values, such as for ON DELETE, ON UPDATE, WITH LOCKMODE, SQL hints, etc., so that the full array of platform specific values, including new ones that come out in newer releases, can be used without SQLAlchemy requiring modification. All of that said, all these SQL keywords can be tested against a basic sanity check, such as one that tests for alphanumeric characters only, so I wouldn't oppose a standard sanity checker for all the places we accept SQL keywords. I suspect there needs to be some specific per-database-driver logic to handle unimplemented cases. PostgreSQL, for example, doesn't support MATCH PARTIAL ( http://www.postgresql.org/docs/9.1/static/sql-createtable.html ) and MySQL, naturally, completely ignores the syntax and triggers other clauses to be ignored: SQLAlchemy's current technique for this common situation is to use conditional DDL techniques: http://docs.sqlalchemy.org/en/rel_0_7/core/schema.html#sqlalchemy.schema.DDLElement.execute_if http://docs.sqlalchemy.org/en/rel_0_7/core/schema.html#metadata-ddl -- You received this message because you are subscribed to the Google Groups sqlalchemy group. To post to this group, send email to sqlalchemy@googlegroups.com. To unsubscribe from this group, send email to sqlalchemy+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/sqlalchemy?hl=en.
Re: [sqlalchemy] [PATCHES] _compiler_dispatch in visit_delete and FROM ONLY support for postgres
On Jun 10, 2012, at 12:35 PM, Ryan Kelly wrote: I'm attaching two new patches, one for the _compiler_dispatch in visit_delete/visit_update, and the other for implementing FROM ONLY using hints. Pretty similar to your patch but with some fixes to make it work with UPDATE and DELETE as well as proper tests and documentation. great these are in queue as http://www.sqlalchemy.org/trac/ticket/2506 and http://www.sqlalchemy.org/trac/ticket/2507 . -- You received this message because you are subscribed to the Google Groups sqlalchemy group. To post to this group, send email to sqlalchemy@googlegroups.com. To unsubscribe from this group, send email to sqlalchemy+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/sqlalchemy?hl=en.
