HHH-12492 Jpa DELETE Query generated has missing table alias and thus incorrect semantics #2439

yrodiere · 2018-07-19T07:30:52Z

https://hibernate.atlassian.net//browse/HHH-12492

Given the very sensitive section of the code I had to change, maybe we should wait for 5.4 before merging this. That being said, my first attempt was definitely buggy and at least 3 previously existing tests failed as a result, which is reassuring.

…t query's aliases

gsmet

Added one minor comment.

gsmet · 2018-07-19T08:26:36Z

hibernate-core/src/main/java/org/hibernate/hql/internal/ast/tree/FromElement.java

@@ -172,7 +172,7 @@ public String getClassAlias() {
 		//return classAlias == null ? className : classAlias;
 	}

-	private String getTableName() {
+	public String getTableName() {


I don't think this is needed?

Right, I removed that.

…ueries in DELETE/UPDATE queries Don't try to duplicate the logic from org.hibernate.hql.internal.ast.tree.FromElementType#toColumns(java.lang.String, java.lang.String, boolean, boolean) in other classes, it's complex enough and already seems to handle all the cases we might encounter. In this specific case, we want the table name to be used to qualify column names, because the target table doesn't have any alias (it's not supported by every version of every RDBMS), and not qualifying columns at all may lead to a confusing statement, in particular if tables referenced in the subquery contain columns with the same name. Since we use aliases for every other table in the query, referencing the table should not lead to any conflict.

gsmet · 2018-07-19T08:44:28Z

hibernate-core/src/main/java/org/hibernate/hql/internal/ast/tree/IdentNode.java

@@ -197,14 +197,6 @@ private boolean resolveAsAlias() {

 		String[] columnExpressions = element.getIdentityColumns();


So the general idea is to fix this getIdentityColumns() to avoid needing the workaround below.

This is done by using an existing logic doing exactly what we need.

Yep. Also, getIdentityColumns is only used in IdentNode#resolveAsAlias, so changing its behavior should not affect any other node or any other resolution of an IdentNode.

After some scrolling through the related code, and having played with FromElement recently, I can confirm that the replaced code in getIdentityColumns() was indeed a simplified version of what happened in the toColumns function now. The latter indeed has a special handling for subqueries. This change looks good to me!

gbadner · 2018-07-25T18:26:26Z

I'm not familiar enough with this code to know for sure. @dreab8, can you take a look?

dreab8 · 2018-07-26T11:52:29Z

The PR looks fine to me just not 100% sure the change to the IdentNode is safe so I would limit the changes to the FromElement class

if ( getWalker().getStatementType() == HqlSqlTokenTypes.SELECT ) {
			return getPropertyMapping( propertyName ).toColumns( table, propertyName );
		}
		else {
			return toColumns(
					table, propertyName,
					getWalker().getStatementType() == HqlSqlTokenTypes.SELECT
			);
		}

this seems enough to solve the issue.

gsmet · 2018-07-26T12:25:28Z

@dreab8 keep in mind that hte change in IdentNode is specifically manipulating a FromElement. As the element as been qualifier in FromElement itself, we don't need to do it again.

@yrodiere can you think of a case where keeping the code in IdentNode fails? I suppose it won't as there is this qualifyItNot() thing. But it's still better for correctness IMHO.

…in subqueries in DELETE/UPDATE queries

yrodiere · 2018-07-26T12:42:18Z

@yrodiere can you think of a case where keeping the code in IdentNode fails? I suppose it won't as there is this qualifyItNot() thing. But it's still better for correctness IMHO.

I also think it's confusing and just wrong to keep this piece of code. But I doubt it will ever get executed, so I guess in this regard it's "safer" to keep it.

I pushed a fix-up commit that restores this piece of code, let's see if any test fails.

beikov · 2018-07-26T12:52:00Z

I haven't checked the details of the PR or specific DBMS, I just wanted to mention that I think this was done this way because some DBMS do not support an explicit alias for DML tables and instead require the use of the table name.

sebersole · 2018-07-26T13:03:13Z

This.

…

On Thu, Jul 26, 2018, 7:52 AM Christian Beikov ***@***.***> wrote: I haven't checked the details of the PR or specific DBMS, I just wanted to mention that I think this was done this way because some DBMS do not support an explicit alias for DML tables and instead require the use of the table name. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2439 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOUE0UAe0wWr8ncs2WP6tSwIyS2PvxTks5uKbtygaJpZM4VV47L> .

yrodiere · 2018-07-26T13:19:02Z

I haven't checked the details of the PR or specific DBMS, I just wanted to mention that I think this was done this way because some DBMS do not support an explicit alias for DML tables and instead require the use of the table name.

Yes, this was accounted for.

yrodiere · 2018-07-26T13:20:41Z

To be clearer: FromElementType.toColumns will use the table name to qualify columns when the "fromelement" is the target of an update/delete query, where aliases cannot be used.

gsmet · 2018-07-27T13:03:29Z

Merged, thanks!

HHH-12492 Test HQL Delete query with a subquery referencing the paren…

258b5d3

…t query's aliases

yrodiere added the Ready for review label Jul 19, 2018

gsmet reviewed Jul 19, 2018

View reviewed changes

yrodiere force-pushed the HHH-12492 branch from ed8a65b to 5d6130c Compare July 19, 2018 08:42

gsmet added Requires Gail 5.3 labels Jul 19, 2018

gsmet reviewed Jul 19, 2018

View reviewed changes

jwgmeligmeyling approved these changes Jul 21, 2018

View reviewed changes

gbadner requested a review from dreab8 July 25, 2018 18:26

fixup! HHH-12492 Qualify references to columns from the target table …

41db30f

…in subqueries in DELETE/UPDATE queries

gsmet closed this Jul 27, 2018

yrodiere deleted the HHH-12492 branch June 26, 2019 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HHH-12492 Jpa DELETE Query generated has missing table alias and thus incorrect semantics #2439

HHH-12492 Jpa DELETE Query generated has missing table alias and thus incorrect semantics #2439

yrodiere commented Jul 19, 2018

gsmet left a comment

gsmet Jul 19, 2018

yrodiere Jul 23, 2018

gsmet Jul 19, 2018 •

edited

Loading

yrodiere Jul 19, 2018

jwgmeligmeyling Jul 21, 2018

gbadner commented Jul 25, 2018

dreab8 commented Jul 26, 2018

gsmet commented Jul 26, 2018

yrodiere commented Jul 26, 2018

beikov commented Jul 26, 2018

sebersole commented Jul 26, 2018 via email

yrodiere commented Jul 26, 2018

yrodiere commented Jul 26, 2018

gsmet commented Jul 27, 2018

		@@ -197,14 +197,6 @@ private boolean resolveAsAlias() {

		String[] columnExpressions = element.getIdentityColumns();

HHH-12492 Jpa DELETE Query generated has missing table alias and thus incorrect semantics #2439

HHH-12492 Jpa DELETE Query generated has missing table alias and thus incorrect semantics #2439

Conversation

yrodiere commented Jul 19, 2018

gsmet left a comment

Choose a reason for hiding this comment

gsmet Jul 19, 2018

Choose a reason for hiding this comment

yrodiere Jul 23, 2018

Choose a reason for hiding this comment

gsmet Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

yrodiere Jul 19, 2018

Choose a reason for hiding this comment

jwgmeligmeyling Jul 21, 2018

Choose a reason for hiding this comment

gbadner commented Jul 25, 2018

dreab8 commented Jul 26, 2018

gsmet commented Jul 26, 2018

yrodiere commented Jul 26, 2018

beikov commented Jul 26, 2018

sebersole commented Jul 26, 2018 via email

yrodiere commented Jul 26, 2018

yrodiere commented Jul 26, 2018

gsmet commented Jul 27, 2018

gsmet Jul 19, 2018 •

edited

Loading