ISession.Merge() not returning the entity id for the new entity on version 5.3.x

[Afshin1980]

I updated the NHibernate from 5.2.7 to 5.3.0 and noticed that there is a difference between the returning object from the merge function. In version 5.3.x the merge function doesn't return the entity id if the entity is not in the database but in version 5.2.7 the merge function returns the entity id. I don't know this is a bug or not

Here is the code

using NHibernate.Cfg;
using NHibernate.Dialect;
using NHibernate.Driver;
using NHibernate.Mapping.ByCode;
using NHibernate.Mapping.ByCode.Conformist;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Text;
using System.Threading.Tasks;

namespace TestNHibernate
{
    class Program
    {
        static void Main(string[] args)
        {
            var cfg = new Configuration();

            cfg.DataBaseIntegration(x =>
            {
                x.ConnectionString = "Data Source=.;integrated security=true;Initial Catalog=TestDB;MultipleActiveResultSets=True";
                x.Driver<SqlClientDriver>();
                x.Dialect<MsSql2008Dialect>();
            });

            cfg.AddAssembly(Assembly.GetExecutingAssembly());

            var sefact = cfg.BuildSessionFactory();

            using (var session = sefact.OpenSession())
            {
                var person = new Person
                {
                    Name= "Jamie"
                };

                using (var tx = session.BeginTransaction())
                {
                    var merged = session.Merge(person);

                    session.SaveOrUpdate(merged);
                    tx.Commit();
                }
                                                
                Console.ReadLine();
            }
        }
    }

    public class Person
    {
        public virtual int ID { get; set; }
        public virtual string Name { get; set; }
        public virtual string Tel { get; set; }
    }
}

[fredericDelaporte]

This is a change coming from #1754. But it is unclear for me whether Merge should follow that change or not.

We should at least document it in the possible breaking changes of 5.3.

[bahusoid]

I'm also not sure it's intended behavior. Current behavior might be considered useful if batch entities insert is used.
For now it seems you need to wrap this call in additional session.Save if you need to save transient entity and get its ID on merge

[bahusoid]

it seems you need to wrap this call in additional session.Save

Nope. It won't save it.. Seems there is now no way to save entity on merge without actual session flush.

[fredericDelaporte]

I am going to document it as a possible breaking change of 5.3, as part of releasing 5.3.6.

@hazzik, what do think of this subject?

[hazzik]

I think Merge should behave like Save rather than Persist. So it should trigger a flush on access to the id.

[fredericDelaporte]

There is no mechanism in NHibernate for triggering a flush when accessing the id property of an entity. I guess you mean Merge should flush to db entities having db-generated ids.

In such case we should consider this issue as a regression needing to be fixed in 5.3.x.

[bahusoid]

I guess you mean Merge should flush to db entities having db-generated ids.

If you are referring to my comment - no I just meant that now saving is deferred till session.Flush is called. I didn't mean it as suggested fix for Merge.

Regarding fix to behave like Save it seems it's one liner:

nhibernate-core/src/NHibernate/Event/Default/DefaultMergeEventListener.cs

Line 293 in 6ab71d0

SaveWithGeneratedId(entity, entityName, copyCache, source, false);

changing last parameter requiresImmediateIdAccess to true.

Maybe we should also introduce Merge with additional parameter to control this behavior.

[hazzik]

it seems it's one liner

I love this fix.

Maybe we should also introduce Merge with additional parameter to control this behavior.

Why?

[hazzik]

There is no mechanism in NHibernate for triggering a flush when accessing the id property of an entity.

There is... the one which @bahusoid just wrote above.

[hazzik]

Probably we would need this as well (from SaveOrUpdate event listener):

        object id = SaveWithGeneratedOrRequestedId(@event);

->>>    source.PersistenceContext.ReassociateProxy(@event.Entity, id);

        return id;

[bahusoid]

Why?

Not sure... Maybe to allow safely Merge outside of transaction or for batch insert. Basically same reasons why #1754 is needed.

[fredericDelaporte]

When I read

it should trigger a flush on access to the id.

I understand than given an entity reference named a by example, having its id mapped as property Id, a reading call to a.Id should trigger the entity flush.

As far as I known there is no mechanism for this.

Save semantic is to return the identifier of the saved entity, so naturally it uses the requiresImmediateIdAccess parameter, it does actually need it.

Merge does not seem to forcibly need to access the id. It returns the entity, and currently does not care about its id being hydrated or not. So when I read your comment, I was wondering if you were meaning that right after a Merge, no flush should have occur whatever may the id generator be, and only on the entity id property get the flush would occur. Such a thing would require yielding a new special kind of proxy.

Anyway your answer confirms for me what I was guessing, you suggest forcing immediate id hydration during Merge operations.

About what is the right choice, well Merge, as Persist, is defined in the JSR-220 specification. That is not the case of Save. What it says about Merge does not give much guidance about our case at hand. But since both Merge and Persist are defined in that specification, and not Save, I would say Merge should behave more like Persist if we wish to account for that specification.

But do we care much about JSR-220?

Causing Merge to behave like Save about identity ids removes a possible breaking change. Having it working like Persist gives the same advantages, it avoids an individual flush for each merged entity using an identity identifier.

Having the choice could be better, but is it worth it?

I am for having Merge not triggering these flushes in case of identity ids. So I am for the possible breaking change. I have no strong arguments for it, and no strong feeling either. So if there is more people thinking we should avoid the possible breaking change, lets fix it with the requiresImmediateIdAccess parameter.

Probably we would need this as well (from SaveOrUpdate event listener)

       object id = SaveWithGeneratedOrRequestedId(@event);

->>>    source.PersistenceContext.ReassociateProxy(@event.Entity, id);

       return id;

No Merge does not need it. It does not attach the supplied object to the session. It merges it into another object, either already attached to the session or newly hydrated for the session. entity in SaveWithGeneratedId call is already a copy, see:

nhibernate-core/src/NHibernate/Event/Default/DefaultMergeEventListener.cs

Line 244 in bad7e99

this.SaveTransientEntity(copy, entityName, requestedId, source, copyCache);

nhibernate-core/src/NHibernate/Event/Default/DefaultMergeEventListener.cs

Lines 286 to 293 in bad7e99

	private void SaveTransientEntity(object entity, string entityName, object requestedId, IEventSource source, IDictionary copyCache)
	{
	// this bit is only *really* absolutely necessary for handling
	// requestedId, but is also good if we merge multiple object
	// graphs, since it helps ensure uniqueness
	if (requestedId == null)
	{
	SaveWithGeneratedId(entity, entityName, copyCache, source, false);

[hazzik]

Ok. Let's just document the behaviour and move on.

[bahusoid]

See #3234 (comment) if you need old behavior.