nhibernate / nhibernate-core

NHibernate Object Relational Mapper
https://nhibernate.info
GNU Lesser General Public License v2.1
2.11k stars 921 forks source link

Inserting multiple associations of the same entity fails #3489

Closed mihaicodrean closed 3 months ago

mihaicodrean commented 4 months ago

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 commented 4 months ago

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

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

mihaicodrean commented 4 months ago

@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 commented 4 months ago

Need to mark the test with [KnownBug] attribute

mihaicodrean commented 4 months ago

Need to mark the test with [KnownBug] attribute

Thanks Alex for pointing that out! Added.

fredericDelaporte commented 4 months ago

Squashed and rebased on 5.4.x (11984a06), 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 commented 3 months ago

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: https://github.com/nhibernate/nhibernate-core/blob/d1790e7d3bb408346059e21279635fb7ac1a902d/src/NHibernate/Hql/Ast/ANTLR/Tree/SelectClause.cs#L497-L505 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 commented 3 months ago

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

mihaicodrean commented 3 months ago

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 commented 3 months ago

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

fredericDelaporte commented 3 months ago

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 commented 3 months ago

@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.