Quoting for column names in SELECT statements #825

Open
wants to merge 2 commits into
from

Projects

None yet

4 participants

@gossi

And better quoting for SQLite that doesn't trouble with table.col identifiers. See also: https://www.sqlite.org/lang_keywords.html

@staabm staabm commented on the diff Feb 6, 2014
runtime/lib/adapter/DBAdapter.php
@@ -439,7 +439,7 @@ public function createSelectSqlPart(Criteria $criteria, &$fromClause, $aliasAll
$tableName = null;
- $selectClause[] = $columnName; // the full column name: e.g. MAX(books.price)
+ $selectClause[] = $this->quoteIdentifier($columnName); // the full column name: e.g. MAX(books.price)
@staabm
staabm Feb 6, 2014

could you add a unit test for this change?

@gossi
gossi Feb 6, 2014

What values can $columnName take? table.col, MAX(table.col), NESTED(FUNCTION(table.col)) ?

If this is not just the rare columName, how to get it?

@marcj
marcj Feb 6, 2014

yepp, all of them, that's probably the reason why there hasn't been such a quoteIdentifier call. I guess we need here some regex magic.

@willdurand willdurand commented on the diff Feb 6, 2014
runtime/lib/adapter/DBSQLite.php
@@ -103,7 +103,7 @@ public function strLength($s)
*/
public function quoteIdentifier($text)
{
- return '[' . $text . ']';
+ return '"' . $text . '"';
@staabm
staabm Feb 6, 2014

they are considered non-standard and are only supported for compat reasons... nothing important though.

@willdurand
willdurand Feb 6, 2014

Yep I know, I think we can switch but it will impact pretty much every SQLite users.

@staabm
staabm Feb 6, 2014

right.. the risk might not outweigh the benefit (there is actually no benefit)

@gossi does this change fix a issue for you?

@gossi
gossi Feb 6, 2014

square brackets are ok, but they need splitted when dots are present. E.g. quoting name ends in [name] which is fine. Quoting table.name results in [table.name] which is wrong. The correct quoting here would be [table].[name]. Using double quotes instead works.

@willdurand
willdurand Feb 6, 2014

ok, then let's change that.

@marcj
marcj Feb 6, 2014

Nah, sure it doesn't throw then any error with double quotes but it doesn't select anything from the column then. It's the same as in other databases when you execute SELECT "Peter", it displays "Peter" not the data of the peter-column. See http://sqlfiddle.com/#!7/6dee2/1

@gossi
gossi Feb 6, 2014

Ah, that might explain that many failing tests 😉 So, splitting on dot and surround each segment with square brackets.

RTFM:

If a keyword in double quotes (ex: "key" or "glob") is used in a context where it cannot be resolved to an identifier but where a string literal is allowed, then the token is understood to be a string literal instead of an identifier.

@marcj
marcj Feb 6, 2014

yupp, a simple str_replace('.', '].[') would do the trick

@staabm
Propel member

btw: there are a LOT of test failures triggered by this PR

@gossi

Yes, I have a table with a keyword as column name. Works fine in mysql for example. However, I did write some tests using the QuickPropelBuilder which uses SQLite. This throwed me errors and I couldn't continue. Sorry for my ignorance here, I just edited these two lines online and let travis do the testing. I will provide a proper pull request with.

I guess this problem is also present in propel2

@gossi
<table name="keywords">
    <column name="id" type="INTEGER" primaryKey="true" autoIncrement="true"/>
    <column name="group" type="VARCHAR"/>
    <column name="on" type="VARCHAR"/>
    <column name="exists" type="VARCHAR"/>
    <column name="order" type="VARCHAR"/>
</table>

this table will be a lotta fun.

@gossi

Is this worth investigating in propel1, while propel2 is on the line?

@staabm
Propel member

Since the fix will be only a few lines it shouldn't hurt to have it in propel1

@gossi

Nope, I don't think it's just a few lines. It's basically how propel handles keywords as column names. I think this also affects propel2.

1) Missing escaping

Try make a complete propel build with my table above. The resulting sql file will be.

DROP TABLE IF EXISTS keywords;

CREATE TABLE keywords
(
    id INTEGER NOT NULL AUTO_INCREMENT,
    group VARCHAR(255),
    on VARCHAR(255),
    exists VARCHAR(255),
    order VARCHAR(255),
    PRIMARY KEY (id)
) ENGINE=InnoDB;

There is no escaping around column names here.

2) column name vs column statement

In runtime/lib/adapater/DBAdapter.php line 442 there is this statement: $selectClause[] = $columnName; however, $columnName also contains column statements, such as COUNT(table.col), so calling a method escaping the whole expressions won't do the job. Good thing here is, there is a already a parser below.

@gossi

Just tested, this affects both propel1 and propel2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment