Inserting multiple associations of the same entity fails

[mihaicodrean]

This works in 5.3.3, for example, but not in 5.5.0.
Thoughts as to why / how to fix?

Here's the failing test message:

NHibernate.Hql.Ast.ANTLR.QuerySyntaxException : A recognition error occurred. [insert into Enrolment (Course, Student) select e.Course, e.Student from Enrolment e]
  ----> Antlr.Runtime.MismatchedTreeNodeException : A recognition error occurred.

And the stack trace:

ErrorCounter.ThrowQueryException() line 71
BasicExecutor.ctor(IStatement statement, IQueryable persister) line 36
QueryTranslatorImpl.BuildAppropriateStatementExecutor(IStatement statement) line 478
QueryTranslatorImpl.DoCompile(IDictionary`2 replacements, Boolean shallow, String collectionRole) line 430
QueryTranslatorImpl.Compile(IDictionary`2 replacements, Boolean shallow) line 116
ASTQueryTranslatorFactory.CreateQueryTranslators(IQueryExpression queryExpression, IASTNode ast, String queryIdentifier, String collectionRole, Boolean shallow, IDictionary`2 filters, ISessionFactoryImplementor factory) line 53
ASTQueryTranslatorFactory.CreateQueryTranslators(IQueryExpression queryExpression, String collectionRole, Boolean shallow, IDictionary`2 filters, ISessionFactoryImplementor factory) line 21
QueryExpressionPlan.CreateTranslators(IQueryExpression queryExpression, String collectionRole, Boolean shallow, IDictionary`2 enabledFilters, ISessionFactoryImplementor factory) line 37
QueryExpressionPlan.ctor(IQueryExpression queryExpression, Boolean shallow, IDictionary`2 enabledFilters, ISessionFactoryImplementor factory) line 19
QueryPlanCache.GetHQLQueryPlan(IQueryExpression queryExpression, Boolean shallow, IDictionary`2 enabledFilters) line 76
AbstractSessionImpl.GetHQLQueryPlan(IQueryExpression queryExpression, Boolean shallow) line 584
AbstractSessionImpl.CreateQuery(String queryString) line 563
HqlBulkOperations.InsertFromSelectWithMultipleAssociations() line 46
--MismatchedTreeNodeException
TreeParser.RecoverFromMismatchedToken(IIntStream input, Int32 ttype, BitSet follow)
BaseRecognizer.Match(IIntStream input, Int32 ttype, BitSet follow)
SqlGenerator.selectClause() line 2099

And the standard output:

13:24:05,570 ERROR Parser:199 - MismatchedTreeNodeException(151!=3)
13:24:05,680 ERROR Parser:199 - MismatchedTreeNodeException(3!=49)
13:24:05,683 ERROR Parser:199 - MismatchedTreeNodeException(49!=3)

[mihaicodrean]

I have narrowed it down to this commit: 10393df for #2551.

@maca88 & @bahusoid, can I please bring your attention to this, maybe you have an idea on how to address it?

[mihaicodrean]

@fredericDelaporte, I see that the "NHibernate (NHibernate Core)" check step in TeamCity is now failing as expected, for the contributed test. Is the merge just not possible in this case, e.g. can't really contribute failing tests?

[hazzik]

Need to mark the test with [KnownBug] attribute

[mihaicodrean]

Need to mark the test with [KnownBug] attribute

Thanks Alex for pointing that out! Added.

[fredericDelaporte]

Squashed and rebased on 5.4.x (11984a0), it does fail too.
The same on the commit prior to #2551 succeeds, see 491e6da. So, it is confirmed it is a 5.4 regression. (And this has been introduced by #2551: I have checked it fails too when applying the test case onto 10393df.)

We should fix it on 5.4.x. As such, the test case needs to be rebased on 5.4.x and the PR needs to target the 5.4.x branch.

For fixing it, I think the troubles lies within SelectClause, but I am not knowledgeable about that code.

[fredericDelaporte]

I have narrowed it to this change, but have not yet devise a proper fix.

The above link points a comment in a long to load file in the review of the PR having introduced this change. Here is a direct link to current code involved in this bug:

nhibernate-core/src/NHibernate/Hql/Ast/ANTLR/Tree/SelectClause.cs

Lines 497 to 505 in d1790e7

	if (processedElements.Add(fromElement))
	{
	combinedFromElements.Add(fromElement);
	RenderNonScalarIdentifiers(fromElement, inheritedExpressions.ContainsKey(e) ? null : e, appender);
	}
	else if (!inheritedExpressions.ContainsKey(e) && node.Parent != null)
	{
	RemoveChildAndUnsetParent(node);
	}

At the second column, the from is already added, causing the execution to go into the else and not rendering the second column.

In debug, forcing the execution to jump directly to RenderNonScalarIdentifiers (so, not adding again the from to combinedFromElements) allows the test to succeed.

[fredericDelaporte]

I have now overridden your branch in order to target 5.4.x for a potential fix I have added.

[mihaicodrean]

In debug, forcing the execution to jump directly to RenderNonScalarIdentifiers (so, not adding again the from to combinedFromElements) allows the test to succeed.

Thanks much for identifying the root cause!

[fredericDelaporte]

The processedElements set serves a de-duplication purpose of selected entities. But, alone, it fails accounting for associations selected in shallow queries which will be seen as having the same fromElement although being actually different entities being selected.

Shallow queries may still have duplicated entities selected and cannot be entirely ignored for the de-duplication, if we want to keep the test HqlDuplicateEntitySelectionSubQuery as currently is. (I fail to understand why this tests is valid, why should we support the queries in this test, but I did not get answers about this here. And anyway, it has been release in 5.4, so, it is legacy.)

The DOT check allows to recognize cases in shallow queries where the same fromElement may be used by different expressions selecting different things, and a such, needing to be not falsely de-duplicated.

(I thought about checking if the expressions e where "the same" (having same Path) instead when sharing a common fromElement, but that would imply building a dictionary of sets instead of processedElements. I do not consider it is worth it.)

[maca88]

I fail to understand why this tests is valid, why should we support the queries in this test

To be honest, I don't remember why I've added such test. Looking back at it, I agree that the test shouldn't be added.

Perhaps we could add a TODO for removing test HqlDuplicateEntitySelectionSubQuery in version 5.5 and remove the node.Type == SqlGenerator.DOT part from the condition.

[fredericDelaporte]

Hazzik and Maca, since the fix is actually from me, I will not approve this PR myself.

[maca88]

The fix looks good to me.

[fredericDelaporte]

An dev package is available here. See also Nighlty builds for more instructions.

@mihaicodrean, may you check this dev package solves the issue for your application, and that none other regression remains?

[mihaicodrean]

@mihaicodrean, may you check this dev package solves the issue for your application, and that none other regression remains?

Thanks for the fix! I have applied the patch to my local 5.5.0 branch, and all works as expected for me.