#4448 Fixed delete query with alias fails on most platforms

[simonberger]

Q	A
Type	bug
BC Break	no
Fixed issues	#4448

Fixed delete query with alias usage on the following platforms

• mysql
• mssql
• mariadb

The existing syntax works for PostgreSQL.

For backward compatible reasons this is also the default syntax for all platforms not especially configured. On SQLite there is no alias support in delete queries. For Oracle I could not find informations either.

I am open for a change of the method name in the platform classes. I just like to be clear at those points even if a name gets lengthy.

[morozov]

What's the point in keeping the alias in a statement that mentions only one table? Semantically, DELETE FROM users u looks identical to DELETE FROM users regardless of the platform.

Given that the query builder doesn't allow building complex DELETE statements, e.g. with JOINs or sub-queries, why don't we just ignore the alias in this case?

[simonberger]

Personally I don't really care that functionality and didn't excessively use the QueryBuilder but if you want to remove the alias it would be necessary to remove (the correct) aliases from the where conditions as well. And is there something that stops the user from adding subqueries to the where conditions?

If you want to go deny or remove aliases, imho it would be better to throw an exception when aliases are used together with delete. Also that aliases are not really working right now this would not be backward compatible...

[morozov]

And is there something that stops the user from adding subqueries to the where conditions?

This is a good point. With that in mind, I don't think it makes much sense to make any changes in this regard: the builder currently is very little aware of the underlying platform, it's essentially a string builder. If the user who builds the query doesn't need to use the alias, they shouldn't specify it. Otherwise, what's the point in leaving it in the query?

[simonberger]

If the user who builds the query doesn't need to use the alias, they shouldn't specify it. Otherwise, what's the point in leaving it in the query?

I don't understand the last question.

I agree that having an alias in delete queries is not an important feature with the limited functionality of the QueryBuilder.
To try to sum up why I think this should be handled as an issue:

• A requirement of the QueryBuilder is that the user has to know the features of the underlying platform(s) and may just use the available ones. The alias in delete queries is a special case because the syntax differs. I don't know any other place in the QueryBuilder which has a similar issue. The feature fails on platforms it is generally available.
• Even that a provided feature does not work the followup Question is if this feature is in any way useful.
  The user may want to add a Subselect to the where condition of the delete query and use an alias there. Or just use an alias in a normal condition (which is not needed for sure).

I can't answer if it is worth to tackle this problem. It obviously does not affect a lot of users. Some more may have noticed the limitation while implementing something and simply do not use the alias there. Yet it would stay a known problem in dbal.

[morozov]

If the user who builds the query doesn't need to use the alias, they shouldn't specify it. Otherwise, what's the point in leaving it in the query?

I don't understand the last question.

If the user knows that the alias provided to the query builder is not used in the query, why would the user need to provide it?

Yet it would stay a known problem in dbal.

So far I don't see it being a problem. The builder adds the alias if it's provided by the user. What's the use case when a user wants to build a portable DELETE statement with an alias that should be ignored on certain platforms but not on others?

[simonberger]

What's the use case when a user wants to build a portable DELETE statement with an alias that should be ignored on certain platforms but not on others?

This is not my use case and I also do not try to achieve it. I want to give users implementing for mysql, mssql or mariadb a working alias in delete queries. Not postresql only.

In case there is any misunderstanding here an example of a real query:

The following query with a constructed "real use case" could be created by the current QueryBuilder but works on PostreSQL only. For all other platforms it fails.

DELETE FROM user u WHERE EXISTS (SELECT 1 FROM user_property up WHERE up.user_id = u.id AND up.property1 = 1)

Now the "patched one" which works on all other platforms that provide the feature. It would fail on PostreSQL though so I added the platform check.

DELETE u FROM user u WHERE EXISTS (SELECT 1 FROM user_property up WHERE up.user_id = u.id AND up.property1 = 1)

Platforms like oracle which do not have alias support at all are unhandled like in any other function of the QueryBuilder. Using an alias will always fail there.

If instead the goal are best possible portable queries we should just disallow alias use in delete queries in 4.0 or close the PR here and ignore the small issue.

[morozov]

Thank you for the explanation. This is why I requested an integration test in the beginning: neither of the examples in the unit tests and the linked issue describe such a case.

While the statements above are syntactically valid SQL that does require the builder to be platform-aware, the question is, is it possible to build them using the builder? It doesn't support sub-queries, so this use case is beyond the scope of the current API.

If instead the goal are best possible portable queries we should just disallow alias use in delete queries in 4.0 or close the PR here and ignore the small issue.

I believe this would be the best course of action.

[simonberger]

While the statements above are syntactically valid SQL that does require the builder to be platform-aware, the question is, is it possible to build them using the builder?

Possible, absolutely...

$qb = new QueryBuilder($this->em->getConnection());
$qb
    ->delete('users', 'u')
    ->where('EXISTS (SELECT 1 FROM user_property up WHERE up.user_id = u.id AND up.property1 = 1)')
    ->getSQL()
;

Probably not an intended way.
On the other hand also a simple delete with (unnecessary) alias breaks the query on mysql.

$qb = new QueryBuilder($this->em->getConnection());
$qb
    ->delete('users', 'u')
    ->where('u.id = 1')
    ->getSQL()
;

[morozov]

The changes like this should be covered with an integration test first of all. Unit tests in this case are not that important IMO. Please see the testing guildelines.

[simonberger]

The changed and added unit test was more about making it work with the changes.
I'll add integration tests if we decide that we want this change.