Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NH generates incorrect SQL (ignoring property-ref mapping attribute) when querying a many-to-one relationship with a subquery that uses EXISTS(...) #3609

Open
craigfowler opened this issue Sep 23, 2024 · 3 comments

Comments

@craigfowler
Copy link

I have found this issue whilst attempting to upgrade from NHibernate 5.3.7 to 5.5.2. This issue has caused us to abort the upgrade, as it causes a number of our app's queries to return incorrect results.

I have created a minimal reproduction case for the issue and published it in this git repository. I have also reproduced the README for it below, as that contains details about the issue.


NHibernate HBM property-ref bug

This is a reproduction case for an NHibernate bug whereby a property-ref attribute on a <many-to-one /> XML HBM mapping is incorrectly used when generating SQL.
This appears to affect only queries which would cause an EXISTS( ... )-style SQL query to be generated by NHibernate.

The issue differs a little depending upon whether or not the <many-to-one /> mapping uses the not-found="ignore" attribute.

The incorrect SQL appears within that subquery. It relates to the column from the table which is the main subject of the subquery, corresponding to the foreign key column of the table which has the many-to-one mapping. NHibernate generates SQL using the primary key column, where it should instead use the column corresponding to the property identified by the property-ref attribute.

Versions affected

I have tried this out against a variety of NHibernate versions.
In our apps, where we are affected by this issue, we have also been using not-found="ignore" on the same mappings.
Whilst investigating/creating the reproduction case, I discovered that if I remove the not-found attribute from the mappings then I can reproduce this problem even in version 5.3.7.

But - for the scenario which affects us, when we are using not-found="ignore":

  • 5.3.7 is unaffected
  • 5.5.2 reproduces the problem (this is current latest version at time of writing)
  • 5.4.1 seems unaffected
  • 5.4.3 reproduces the problem
  • 5.3.15 seems unaffected
  • 5.3.17 reproduces the problem

It seems like 5.3.17 along with 5.4.3 are the first affected versions.
Muddying the picture a little, there is another related issue, predating this, affecting versions 5.3.16 & 5.4.2 only. See below for more info.
This means that if we wish to see a sample of 'good' SQL for the query, we must use 5.3.15 or 5.4.1, or earlier.

Another related issue, predating this

There is a separate-but-related issue to this which affects NH 5.3.16 and 5.4.2.
That issue might be #3269 but I can't be 100% certain just from a read of the ticket.
The related issue means that the test case included in this repository will pass on those two versions, but NHibernate would still generate incorrect SQL, causing other problems.

It's best to consider the "last good" versions to be 5.3.15 & 5.4.1 with regard to the issue that this repo demonstrates.

Sample reproduction case

There are two mapped classes in this solution, including a mapping which reproduces this issue:

  • It uses not-found="ignore"
  • It uses property-ref="..."

One of the tests included in the solution executes a query which triggers generation of a SQL EXISTS(...) subquery.

This solution uses SQLite in-memory driver/dialect so as to be self-contained.
As far as I can tell the DB driver/dialect is irrelevant though, because we reproduced this in an MS SQL Server environment.

To run this reproduction case:

  1. Clone the repository
  2. Run the tests using .NET 8 or higher: dotnet test
    • Optional, to run the tests against a specific NHibernate version set the NhVersion property to the desired NHibernate version
    • For example: dotnet test /p:NhVersion=5.4.9

The mappings include an interceptor which will capture and emit the SQL which is generated to STDERR.
This is to help illustrate/diagnose the problem.

Expected behaviour

Both unit tests should pass; the queries performed by the tests are querying against known data which has been added to the in-memory DB.

Actual behaviour

The unit test for the subquery fails.

Further info/analysis

Note that in passing scenarios (NH version 5.3.15) the SQL emitted by the interceptor is as follows.

select cast(count(*) as INTEGER) as col_0_0_
from LineItem lineitem0_
where exists (
    select order1_.Id
    from TheOrder order1_, TheOrder order2_
    where
        lineitem0_.OrderId=order2_.UniqueId
        and order1_.CreatedDate>?
        and order1_.Id=order2_.Id
    )

However in failing scenarios (NH >= 5.3.17) the SQL emitted by the interceptor is as follows. The problematic area is pointed out in a comment.

select cast(count(*) as INTEGER) as col_0_0_
from LineItem lineitem0_
where exists (
    select order1_.Id
    from TheOrder order1_
    where
        order1_.CreatedDate>?
        and order1_.Id=lineitem0_.OrderId
--                  ^^
--      This criterion should use order1_.UniqueId and not order1_.Id as
--      the column on the joined table.  That's because the property-ref attribute
--      of the many-to-one indicates that UniqueId is the property to use, instead
--      of the primary key value.
    )
@hazzik
Copy link
Member

hazzik commented Sep 23, 2024

Likely a regression from #3306 / #3312

@hazzik
Copy link
Member

hazzik commented Sep 23, 2024

Changing validOrders.Any(y => y == x.Order) to validOrders.Contains(x.Order) solves your issue. I'll take a further look.

hazzik added a commit to hazzik/nhibernate-core that referenced this issue Sep 23, 2024
@craigfowler
Copy link
Author

Changing validOrders.Any(y => y == x.Order) to validOrders.Contains(x.Order) solves your issue. I'll take a further look.

Ah, thanks, you are right.

Sadly, in our real app, the .Any(...) usage is more complex than the repro case I presented. It can't be replaced with a .Contains(...). That's a side effect of producing a self-contained repro case which doesn't include all of the complexities of our app; it oversimplified it perhaps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants