Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix subquery alias with Oracle #689

Open
wants to merge 1 commit into from

4 participants

@laucall

When working with subqueries with the ModelCriteria::addSelectQuery
method, BasePeer::createSelectSql method concatenate an AS keyword
between the subquery and the alias name.
Unfortunately, this is not a valid Oracle syntax, leading in an
"ORA-00933: SQL command not properly ended" error.

The trick is to delegate the table alias management to adapter.

I added a class constant called TABLE_ALIAS_KEYWORD, both in DBAdapter
abstract class and DBOracle to handle this issue and updated
BasePeer::createSelectSql method.

@lcallarec lcallarec Subquery alias keyword delegated to adapters
When working with subqueries with the ModelCriteria::addSelectQuery
method, BasePeer::createSelectSql method concatenate an AS keyword
between the subquery and the alias name.
Unfortunately, this is not a valid Oracle syntax, leading in an
"ORA-00933: SQL command not properly ended" error.

The trick is to delegate the table alias management to adapter.

I added a class constant called TABLE_ALIAS_KEYWORD, both in DBAdapter
abstract class and DBOracle to handle this issue and updated
BasePeer::createSelectSql method.
d7b76d5
@staabm staabm commented on the diff
runtime/lib/util/BasePeer.php
@@ -824,7 +824,7 @@ public static function createSelectSql(Criteria $criteria, &$params)
// add subQuery to From after adding quotes
foreach ($criteria->getSelectQueries() as $subQueryAlias => $subQueryCriteria) {
- $fromClause[] = '(' . BasePeer::createSelectSql($subQueryCriteria, $params) . ') AS ' . $subQueryAlias;
+ $fromClause[] = '(' . BasePeer::createSelectSql($subQueryCriteria, $params) . ') ' . $db::TABLE_ALIAS_KEYWORD . ' ' . $subQueryAlias;
@staabm Collaborator
staabm added a note

Is this the only place where the alias is a problem?

@laucall
laucall added a note

I will investigate that point tomorrow.

@laucall
laucall added a note

You were right : there is a lot of places where hard coded 'AS' keyword for table alias can / should be replaced by a keyword related to adapter :

In adapters :

  • DBAdapter::getDeleteFromClause
  • DBPostgres::getDeleteFromClause

In unit tests :
ModelCriteriaTest::testDeleteUsingTableAlias()
In a lot of places inside SubQueryTest and BasePeerTest.

Let me know if you are ok by the way I managed this issue in my first commit. If so, I will pull a new commit once the job is done.

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

I am ok with your patch but I am wondering that noone had issues with those queries?

Does Oracle really not support this keyword or maybe you need to adjust the db configuration or so?

@laucall

I was very surprised too when I fell on this issue. I'm working with Propel / Oracle for 4 years, and I never had to deal with subqueries. When I write raw SQL statements, I never use the AS keywords before table / subquery correlation name (with Oracle, MySQL, SQLServer...)

Anyway, Oracle (at least from 9 to 11) does not support the 'AS' keyword here (it is optionnal according to SQL-92 standard, and this does not rely on database configuration)

There is a lot of posts that we can found on Internet, like :

Finally, there is two ways to handle this issue.

  • Because I think the AS keyword is never mandatory for a table / subquery alias - but I still have to validate this point for all major versions of all RDBMS supported by Propel - we could remove the AS keyword in all cases, leading in non-fully compliant SQL code but a cleaner code (*);
  • Using DBAdapter::TABLE_ALIAS_KEYWORD everywhere it should be applied

(*) I can confirm right now that the AS keyword is not mandatory for these rdbms :

  • SQLServer 2005 to 2012
  • Oracle 9 to 11
  • MySQL 5.5
@staabm
Collaborator

@willdurand @marcj @jaugustin @havvg any opinions on this topic?

@havvg

Well, the SQL standard defines the AS keyword as optional in use e.g. on aliasing a column or correlation. This means, it is optional to write it, not optional to support it when writing an RDBMS implementing this standard. Therefore it's a bug of the RDBMS in use.

There are specifications where the AS keyword is mandatory e.g. when type casting, creating a view etc.

However, as already mentioned, I would second to properly patch this on all occurrences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 16, 2013
  1. @lcallarec

    Subquery alias keyword delegated to adapters

    lcallarec authored
    When working with subqueries with the ModelCriteria::addSelectQuery
    method, BasePeer::createSelectSql method concatenate an AS keyword
    between the subquery and the alias name.
    Unfortunately, this is not a valid Oracle syntax, leading in an
    "ORA-00933: SQL command not properly ended" error.
    
    The trick is to delegate the table alias management to adapter.
    
    I added a class constant called TABLE_ALIAS_KEYWORD, both in DBAdapter
    abstract class and DBOracle to handle this issue and updated
    BasePeer::createSelectSql method.
This page is out of date. Refresh to see the latest.
View
6 runtime/lib/adapter/DBAdapter.php
@@ -37,6 +37,12 @@
const ID_METHOD_SEQUENCE = 2;
/**
+ * Generic table alias keyword
+ * @var string
+ */
+ const TABLE_ALIAS_KEYWORD = 'AS';
+
+ /**
* Propel driver to Propel adapter map.
*
* @var array
View
8 runtime/lib/adapter/DBOracle.php
@@ -23,6 +23,14 @@
class DBOracle extends DBAdapter
{
/**
+ * Oracle specifi table alias keyword
+ * (AS keyword is not allowed by Oracle RDBMS)
+ *
+ * @var string
+ */
+ const TABLE_ALIAS_KEYWORD = '';
+
+ /**
* This method is called after a connection was created to run necessary
* post-initialization queries or code.
* Removes the charset query and adds the date queries
View
2  runtime/lib/util/BasePeer.php
@@ -824,7 +824,7 @@ public static function createSelectSql(Criteria $criteria, &$params)
// add subQuery to From after adding quotes
foreach ($criteria->getSelectQueries() as $subQueryAlias => $subQueryCriteria) {
- $fromClause[] = '(' . BasePeer::createSelectSql($subQueryCriteria, $params) . ') AS ' . $subQueryAlias;
+ $fromClause[] = '(' . BasePeer::createSelectSql($subQueryCriteria, $params) . ') ' . $db::TABLE_ALIAS_KEYWORD . ' ' . $subQueryAlias;
@staabm Collaborator
staabm added a note

Is this the only place where the alias is a problem?

@laucall
laucall added a note

I will investigate that point tomorrow.

@laucall
laucall added a note

You were right : there is a lot of places where hard coded 'AS' keyword for table alias can / should be replaced by a keyword related to adapter :

In adapters :

  • DBAdapter::getDeleteFromClause
  • DBPostgres::getDeleteFromClause

In unit tests :
ModelCriteriaTest::testDeleteUsingTableAlias()
In a lot of places inside SubQueryTest and BasePeerTest.

Let me know if you are ok by the way I managed this issue in my first commit. If so, I will pull a new commit once the job is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
// build from-clause
Something went wrong with that request. Please try again.