Dml Linq Update Produce Wrong Sql

[gokhanabatay]

Hi,
We have a major issue that produce wrong update statement at production environment.
Linq Dml style update queries creates same sql with different Linq queries please check, i think the issue is query expression plan cache, it does not includes update fields to cache key.

UnitTest: UpdateRetrieve_TxnIssuer_Fails

 using (ISession session = SessionProvider.ISessionFactory
                                        .WithOptions()
                                        .Interceptor(new ContextInterceptor())
                                        .OpenSession())
{
    using (ITransaction transaction = session.BeginTransaction())
    {
        var listOfGuids = session.Query<TxnIssuer>().Select(x => x.Guid).ToList();

        //Below query updates MrcDailyMoved and MrcDailyMovedDate with success
        session.Query<TxnIssuer>()
                   .Where(x => listOfGuids.Contains(x.Guid))
                   .Update(x => new TxnIssuer
                   {
                        MrcDailyMoved = "Y",
                        MrcDailyMovedDate = DateTime.Now
                   });

        Assert.That(session.Query<TxnIssuer>()
            .Where(x => x.MrcDailyMoved == "Y")
            .ToList().Count == 10);

        //This Update Statement Creates wrong Sql, I think expression cache does not include Update fields to statement so above and below linq creates or use the same statement.
        //Below query updates MrcDailyMoved and MrcDailyMovedDate instead of CycleMoved,CycleMovedDate
       session.Query<TxnIssuer>()
                  .Where(x => listOfGuids.Contains(x.Guid))
                  .Update(x => new TxnIssuer
                  {
                       CycleMoved = "Y",
                       CycleMovedDate = DateTime.Now
                  });

        Assert.That(session.Query<TxnIssuer>()
            .Where(x => x.CycleMoved == "Y")
            .ToList().Count == 10);
    }
}

Linq1 resulting SQL:

update ISSUER set
    MRC_DAILY_MOVED=:p0,
    MRC_DAILY_MOVED_DATE=:p1
  where GUID in (:p2 , :p3 , :p4 , :p5 , :p6 , :p7 , :p8 , :p9 , :p10 , :p11);
:p0 = 'Y' [Type: String (0:0:0)],
:p1 = 20200102 [Type: Int32 (0:0:0)],
:p2 = 1 [Type: Int64 (0:0:0)],
:p3 = 2 [Type: Int64 (0:0:0)],
:p4 = 3 [Type: Int64 (0:0:0)],
:p5 = 4 [Type: Int64 (0:0:0)],
:p6 = 5 [Type: Int64 (0:0:0)],
:p7 = 6 [Type: Int64 (0:0:0)],
:p8 = 7 [Type: Int64 (0:0:0)],
:p9 = 8 [Type: Int64 (0:0:0)],
:p10 = 9 [Type: Int64 (0:0:0)],
:p11 = 10 [Type: Int64 (0:0:0)]

Linq2:

update ISSUER set
    MRC_DAILY_MOVED=:p0,
    MRC_DAILY_MOVED_DATE=:p1
  where GUID in (:p2 , :p3 , :p4 , :p5 , :p6 , :p7 , :p8 , :p9 , :p10 , :p11);
:p0 = 'Y' [Type: String (0:0:0)],
:p1 = 20200102 [Type: Int32 (0:0:0)],
:p2 = 1 [Type: Int64 (0:0:0)],
:p3 = 2 [Type: Int64 (0:0:0)],
:p4 = 3 [Type: Int64 (0:0:0)],
:p5 = 4 [Type: Int64 (0:0:0)],
:p6 = 5 [Type: Int64 (0:0:0)],
:p7 = 6 [Type: Int64 (0:0:0)],
:p8 = 7 [Type: Int64 (0:0:0)],
:p9 = 8 [Type: Int64 (0:0:0)],
:p10 = 9 [Type: Int64 (0:0:0)],
:p11 = 10 [Type: Int64 (0:0:0)]

[gokhanabatay]

@bahusoid it's a really big issue, could you provide hot fix or early release of latest version?

[bahusoid]

What kind of release are you talking about? Changes on master are not yet released..

[gokhanabatay]

We are using latest version 5.2.6 that includes this bug, I kindly suggest 5.2.7 an early release of 5.2.6 that fixes this issue.

[bahusoid]

We are using latest version 5.2.6

Please confirm you are using official 5.2.6 without any modifications? I thought it's about 8c7444d which was commited only for 5.3

That's very strange - 5.2.6 shouldn't cache update queries at all. That's why you created #2222 - isn't it?

[gokhanabatay]

We are using official 5.2.6 that's right without any modification could you look unit test project provided below.
NHibernate 5.2.6: UpdateRetrieve_TxnIssuer_Fails

#2222 Thats strange but this one hits cache and uses wrong sql statement.

[bahusoid]

Then my revert PR doesn't solve your issue...
You test case isn't really helpful - you are not using https://github.com/nhibernate/nhibernate-core-testcase as a base and I don't have postgre installed and DB schema seems not restored in your tests Replacing hibernate.cfg with my configuration doesn't work for me. Too much time is required to make your test work for me..

[gokhanabatay]

Test case is so simple actually, let me try to explain.
You only need to change hibernate.cfg.xml configuration with your own database config: dialect and connection string thats it.
It creates database and tables first with "hbm2ddl.auto" every time when you run the test case.

If still too much than;

1. copy TxnIssuer class and mapping
2. insert some dummy data to TxnIssuer
3. try above Dml updates in that order, you could see that second one uses first one's Sql Statement.

<property name="dialect">NHibernate.Dialect.PostgreSQL83Dialect</property>
    <property name="connection.connection_string">User ID=postgres;Password=1;Host=localhost;Port=5432;Database=postgres;Pooling=true;Minimum Pool Size=5;Maximum Pool Size=100</property>
    <property name="adonet.batch_size">500</property>
    <property name="command_timeout">600</property>
    <property name="use_proxy_validator">false</property>
    <property name="show_sql">false</property>
    <property name="format_sql">false</property>
    <property name="track_session_id">true</property>
    <property name="current_session_context_class">thread_static</property>
    <property name="default_flush_mode">Commit</property>
    <property name="hbm2ddl.auto">create</property>
    <property name="cache.use_second_level_cache">false</property>
    <property name="cache.use_query_cache">false</property>
    <property name="default_batch_fetch_size">50</property>

[bahusoid]

OK. I've been able to reproduce it

[gokhanabatay]

@bahusoid, will you fix the issue in 5.2.x ?
When will you plan to release if you decide to fix?
If you decide not to fix, then what will we do? We have a lot of linq update expressions

[fredericDelaporte]

The trouble exists since 5.0 and the introduction of Linq DML, and concerns only cases using parameter lists (typically a .Contains on an array or list fetched previously to querying).
We consider fixing it in 5.3.

[bahusoid]

@fredericDelaporte
I double checked if it's really an issue since 5.0 as the following comment looked suspicious (could be interpreted as a hack that avoided constants to be detected as parameters):

nhibernate-core/src/NHibernate/Linq/DmlExpressionRewriter.cs

Lines 103 to 105 in 1920603

	//The ListInit is intentionally "infected" with the lambda parameter (param), in the form of an IIF.
	//The only relevance is to make sure that the ListInit is not evaluated by the PartialEvaluatingExpressionTreeVisitor,
	//which could turn it into a Constant

And indeed in 5.0 constants in Update statement are not transformed to parameters. So the following Update

.Update(x => new Customer {CompanyName = "Constant1"});

In cache key looks like:

(new Dictionary`2(Equal(, NULL) ? 1 : 1, )CompanyNameConstant1)), )

So I'm afraid it's a regression. Seems logic for parameters detection was changed in 5.1 and hack no longer works. In 5.1 all constants are parametrized:

Quote((, ) => (new System.Collections.Generic.Dictionary`2[[System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089],[System.Object, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089]], mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089(Equal(, NULL) ? p3 : p4, )p5p6)), )

CanCachePlan check also introduced in 5.1 hides this issue by skipping caching for common DML queries. So only expanded queries (with parameters list) are affected. So it seems at least #2300 should be backported back to 5.1

[fredericDelaporte]

So that would be a 5.1 regression ? Two patches to issue then.

(By the way, I have read somewhere you were not doing releases. I think you could do it or most of it, and doing it for a patch is likely easier, if you wish. There are instructions here. Nothing very stimulating, but someone has to do it.)