Invalid SQL when referencing nullable entity in correlated subquery

[craigfowler]

We have recently tried upgrading NHibernate for our private project and found that the upgrade caused crashes against some particular queries that had previously returned the expected results.

I have narrowed this problem down to having been introduced in NHibernate 5.3.11 (last working version was 5.3.10). I have also created a standalone reproduction project, which I have placed online. I have tried to keep the example as minimal as possible, to match the query & techniques that we are actually using. Some of the query/mappings/entities might prove to be irrelevant though.

Full repro steps and the analysis I have already performed are available in the Readme to that repo. You'll also find a sample schema creation script in there. It also includes copies of the SQL produced by 5.3.10 and 5.3.11 and an indication of where they differ. In short, in a multi-table subquery which NHibernate creates, it omits a comma between two tables in the FROM part of the query. This then causes a crash error because the DB rejects it.

FROM tableOne tableOneAlias tableTwo tableTwoAlias
-- ----------------------- ^ This is where the comma is missing

There is another difference in the SQL though, which might prove to be another problem entirely (I'm happy to open a new ticket if someone determines that it is indeed unrelated). The criterion which uses the formula for a many-to-one association (which is mapped using a formula) is omitted from the WHERE from 5.3.11 onward. I think that even if the invalid SQL (missing comma) issue were fixed, the query would still return incorrect results because of the omitted criterion.

I have reproduced this with T-SQL (MS SQL Server) but I have not tried it with other database drivers/dialects such as SQLite. It may or may not affect those too. I have also reproduced it with the current latest NH version (at time of writing) 5.4.2.

[fredericDelaporte]

Likely a regression from #2989.

[hazzik]

I tried to put your test case to the code and I think you have some invalid data in the script:

https://github.com/craigfowler/NHibernateInvalidSql/blob/2dc096a227967da9ef12fc6e3cd930833e04dc6c/InitialSchema.sql#L44-L56

INSERT INTO t_entity (id) VALUES (1), (2), (3);
INSERT INTO p_entity (id, p_status) VALUES (1, 'Good PStatus'), (2, 'Bad PStatus');
INSERT INTO u_entity (p_id, id) VALUES (1, 1), (1, 2), (1, 3), (2, 1);
INSERT INTO l_entity (id, t_status, t_id, p_id, u_id) VALUES
    (1, 'Good TStatus', 1, 1, 1),
    (2, 'Bad TStatus', 1, 3, 2), -- the p_id=3 does not exist
    (3, 'Good TStatus', 1, 2, 2),
    (4, 'Good TStatus', 2, 2, 3);
INSERT INTO d_entity (id, linked_entity_id, linked_entity_type) VALUES
    (1, 1, 'T'),
    (2, 2, 'T'),
    (3, 3, 'T'),
    (4, 0, 'Z');

[craigfowler]

Good spot @hazzik - I put the a value in the wrong place. I'll fix it in that repo in a few mins.

Edit: That data fix is done!

[fredericDelaporte]

I was also looking for converting it into our test case, but stopped when I had to check whether foreign key constraint violations were due to voluntarily invalid data (since the mapping uses not-found="ignore"), or not. (Meaning, whether some foreign constraints inferred from the mapping need to be drop, or if that is the data which need fixing.)

Here is the commit with this point not handled: c6efeb8

[craigfowler]

Hi @fredericDelaporte - the database in our real app is unfortunate in this case. I'd love to burn that schema to the ground but I am forced to retain backwards compatibility with a suite of legacy apps.

The only voluntarily/intentionally missing data in that data set is the d_entity with id of 4. That has a linked_entity_id of zero, which indeed does not exist. It also has a linked_entity_type of 'Z' which wouldn't match the condition specified in the mapping.

The others should relate to rows in the t_entity table. If it doesn't then that's my bad when I wrote the sample data.

[fredericDelaporte]

Your current insert script is:

INSERT INTO t_entity (id) VALUES (1), (2), (3);
INSERT INTO p_entity (id, p_status) VALUES (1, 'Good PStatus'), (2, 'Bad PStatus');
INSERT INTO u_entity (p_id, id) VALUES (1, 1), (1, 2), (1, 3), (2, 1);
INSERT INTO l_entity (id, t_status, t_id, p_id, u_id) VALUES
    (1, 'Good TStatus', 1, 1, 1),
    (2, 'Bad TStatus', 1, 1, 3),
    (3, 'Good TStatus', 1, 2, 2),
    (4, 'Good TStatus', 2, 2, 3);
INSERT INTO d_entity (id, linked_entity_id, linked_entity_type) VALUES
    (1, 1, 'T'),
    (2, 2, 'T'),
    (3, 3, 'T'),
    (4, 0, 'Z');

l_entity 3 looks invalid too. u_entity 2 is linked with p_id 1, so a l_entity linked to u_id 2 cannot be linked to p_id 2. If l_entity 3 has to be valid, either its insert has to change, or u_entity 2 has to be linked to p_id 2.
l_entity 4 is no better: it is linked to u_entity 3, which is linked to p_entity 1 too. So l_entity 4 cannot be linked to p_id 2.

[bahusoid]

No need to bother with data consistency. I think I know what's going on and simply adjust existing unit test.

[craigfowler]

Sorry guys, the sample data was an afterthought, because the main nature of the problem was generation of invalid SQL. The DB was rejecting the query regardless of data.

The intent was to provide some data which should return one row, assuming that the CASE WHEN ... that's missing from the 5.3.11 SQL (but present in the 5.3.10 SQL) is included, but would incorrectly produce more than one row if that piece of SQL were missing from the generated query.

[fredericDelaporte]

Here is a working failing test case on 5.3.x: a06e6db
And succeeding if cherry-picked on 5.3.10: 7650b96

[bahusoid]

Fixed by #3312

[fredericDelaporte]

You can try the fix with the 5.3.17.dev build on Cloudsmith. Also see Nightly builds.