Incorrect fetch of Many-to-Many relation

[mm-rpodstawa]

Recently I've upgraded NH to version 5.4.2 and I've found an issue related to many-to-many relation. Here is a test case:

    public class Entity
    {
        public virtual Guid Id { get; set; }

        public virtual string Name { get; set; }
        
        public virtual ICollection<Entity> Children { get; set; } = new HashSet<Entity>();

        public virtual ICollection<Entity> Parents { get; set; } = new HashSet<Entity>();
    }



    public class EntityMapping : ClassMapping<Entity>
    {
        public EntityMapping()
        {
            Id(x => x.Id, map => map.Generator(Generators.GuidComb));

            Property(
                x => x.Name
            );

            Set(
                x => x.Children,
                v =>
                {
                    v.Table("OrganisationUnitParent_OrganisationUnitChildren");
                    v.Cascade(Cascade.None);
                    v.Inverse(true);
                    v.Key(x =>
                    {
                        x.Column("ParentOuId");
                        x.NotNullable(true);
                    });
                    v.Lazy(CollectionLazy.Lazy);
                    v.Fetch(CollectionFetchMode.Join);
                },
                h => h.ManyToMany(m => m.Column("ChildOuId"))
            );

            Set(
                x => x.Parents,
                v =>
                {
                    v.Table("OrganisationUnitParent_OrganisationUnitChildren");
                    v.Cascade(Cascade.All);

                    v.Key(x =>
                    {
                        x.Column("ChildOuId");
                        x.NotNullable(true);
                    });
                    v.Lazy(CollectionLazy.Lazy);
                    v.Fetch(CollectionFetchMode.Join);
                },
                h => h.ManyToMany(m => m.Column("ParentOuId"))
            );
        }
    }


        [Test]
        public void Test1()
        {
            var person = new Entity();
            person.Name = "pers";
            Session.SaveOrUpdate(person);
            var job = new Entity();
            job.Name = "job";

            Session.SaveOrUpdate(job);

            job.Children.Add(person);
            person.Parents.Add(job);
            Session.SaveOrUpdate(job);
            Session.SaveOrUpdate(person);
            Session.Flush();

            using (var newSession = _sessionFactory.OpenSession())
            {
///////1////////
                //this query causes the problem. If you comment it, this test will pass
                var allOUs = newSession
                    .QueryOver<Entity>()
                    .Fetch(SelectMode.Fetch, x => x.Children)
                    .Fetch(SelectMode.Fetch, x => x.Parents)
                    .TransformUsing(Transformers.DistinctRootEntity)
                    .List();
                
                var jobDb = newSession.Get<Entity>(job.Id);
                var personDb = newSession.Get<Entity>(person.Id);
                
                Assert.AreEqual(1, jobDb.Children.Count);

///////2////////
                Assert.AreEqual(1, personDb.Parents.Count);//because of this issue, here we have 0 instead of 1
                
                //now let's remove all members
                personDb.Parents.Clear();
                jobDb.Children.Clear();
                newSession.Update(personDb);
                newSession.Flush();
            }

            using (var newSession = _sessionFactory.OpenSession())
            {
                var jobDb = newSession.Get<Entity>(job.Id);
                var personDb = newSession.Get<Entity>(person.Id);
                Assert.AreEqual(0, jobDb.Children.Count);
                Assert.AreEqual(0, personDb.Parents.Count);
            }
        }

In this test personDb.Parents.Count (///////2////////) has no elements and should be 1 item. This is because of query above (///////1////////). If you comment this query, the test will pass.
This test works good in version 5.3, and in version v5.4.0+ it fails.

If you need more info, please let me know.

[bahusoid]

Looks like regression caused by #2718

[BhaaLseN]

We also see this on Bags that contain Components which then refer to other mapped entities (so not just many-to-many). The curious thing is that it fails to re-insert the new entries because the old ones aren't deleted properly (DB says no due to PK violations), but this only happens when one of references is set but the other is null. So it is only half broken for us.

Reverted to 5.3.16 until 5.4.x is fixed. Bisect confirms it is #2718:

$ git bisect bad
2f7250b4d0febbd5a071963e4fce817c51c64ceb is the first bad commit
commit 2f7250b4d0febbd5a071963e4fce817c51c64ceb
Author: deAtog <***>
Date:   Tue Sep 6 03:24:26 2022 -0400

    Fix loop detection for associations with fetch="join" mapping (#2718)

    Add 'detect_fetch_loops' option to disable loop detection
    Fixes #2201

    Co-authored-by: Roman Artiukhin <***>
    Co-authored-by: Alex Zaytsev <***>

[bahusoid]

We also see this on Bags that contain Components which then refer to other mapped entities (so not just many-to-many).

Please provide such mapping example.

[BhaaLseN]

Please provide such mapping example.

Yeah, on it, we wasted like 2 days trying to reproduce this from scratch until we caved, deleted everything and copy/pasted our code over. After reducing it from ~800 lines down to ~100 lines, we finally got a case that reproduces as minimalistically as possible:

public class RootEntity
{
	public virtual Guid Id { get; set; }
	public virtual IList<ComponentListEntry> Entries { get; set; } = new List<ComponentListEntry>();
}
public class ComponentListEntry
{
	public virtual EntityWithParent ComponentReference { get; set; }
}

public class EntityWithParent
{
	public virtual Guid Id { get; set; }
	public virtual ParentEntity Parent { get; set; }
}
public class ParentEntity
{
	public virtual Guid Id { get; set; }
}

public class Test3290 : TestCaseMappingByCode
{
	[Test]
	public void ThisFails()
	{
		RootEntity root;
		using (var session = Sfi.OpenSession())
		{
			root = new RootEntity();
			root.Entries.Add(new ComponentListEntry
			{
				// loading (and also subsequent updates) fail when this is null
				ComponentReference = null,
			});

			session.Save(root);
			session.Flush();
		}

		using var newSession = Sfi.OpenSession();
		var reloadedRoot = newSession.Get<RootEntity>(root.Id);
		Assert.AreEqual(1, reloadedRoot.Entries.Count);
	}

	[Test]
	public void ThisWorks()
	{
		RootEntity root;
		using (var session = Sfi.OpenSession())
		{
			var parent = new ParentEntity();
			session.Save(parent);
			var entityWithParent = new EntityWithParent { Parent = parent };
			session.Save(entityWithParent);

			root = new RootEntity();
			root.Entries.Add(new ComponentListEntry
			{
				// loading (and also subsequent updates) work when this is set
				ComponentReference = entityWithParent,
			});

			session.Save(root);
			session.Flush();
		}

		using var newSession = Sfi.OpenSession();
		var reloadedRoot = newSession.Get<RootEntity>(root.Id);
		Assert.AreEqual(1, reloadedRoot.Entries.Count);
	}

	protected override void OnTearDown()
	{
		using var session = OpenSession();
		using var transaction = session.BeginTransaction();
		session.Delete("from System.Object");
		transaction.Commit();
	}

	protected override HbmMapping GetMappings()
	{
		var mapper = new ModelMapper();

		mapper.Class<RootEntity>(rc =>
		{
			rc.Id(x => x.Id, map => map.Generator(Generators.GuidComb));

			rc.Bag(
				x => x.Entries,
				v =>
				{
					v.Access(Accessor.Property);
					v.Lazy(CollectionLazy.NoLazy);
					v.Fetch(CollectionFetchMode.Select);
				},
				h => h.Component(cmp =>
				{
					// real world code has other members here, both nullable and not-nullable
					cmp.ManyToOne(x => x.ComponentReference);
				}));
		});
		mapper.Class<EntityWithParent>(rc =>
		{
			rc.Id(x => x.Id, map => map.Generator(Generators.GuidComb));
			rc.ManyToOne(x => x.Parent, m => m.NotNullable(true));
		});
		mapper.Class<ParentEntity>(rc =>
		{
			rc.Id(x => x.Id, map => map.Generator(Generators.GuidComb));
		});

		return mapper.CompileMappingForAllExplicitlyAddedEntities();
	}
}

Both tests only differ in ComponentListEntry.ComponentReference being set or not; with the null case being the failing one.

The real code obviously has other things in there (that are not nullable), but for the purpose of this test it was enough to have just the single nullable member with its parent reference.

[fredericDelaporte]

Have you tried disabling detect_fetch_loops in the configuration? This setting has been introduced by #2718 and is enabled by default. (Unfortunately it has not been documented in the reference documentation.)

[bahusoid]

@BhaaLseN
Your component issue looks completely unrelated. Can you please double check it's really caused by #2718

Update Your test case seems invalid. If fails with 5.3.16 too

[bahusoid]

Have you tried disabling detect_fetch_loops in the configuration?

But be aware that disabling fetch loop detection can lead to infinite loop. It can be safely disabled if you are really sure you have no loops in your entity graph or have explicitly set max_fetch_depth (see #2718 (comment))

This setting has been introduced by #2718 and is enabled by default.

Setting allowing to disable existing behavior. So it's OK when it's enabled by default.

[BhaaLseN]

@BhaaLseN Your component issue looks completely unrelated. Can you please double check it's really caused by #2718

We ran multiple git bisect and always ended up at 2f7250b. I hope we didn't mess that up multiple times though.

Update Your test case seems invalid. If fails with 5.3.16 too

Huh, maybe I deleted too much there. I'll report back once I have something more useful. Sorry about the noise, but we spent way too long on getting anywhere close to a working testcase and didn't consider checking with 5.3.16 (which does work in the real application)

Have you tried disabling detect_fetch_loops in the configuration? This setting has been introduced by #2718 and is enabled by default. (Unfortunately it has not been documented in the reference documentation.)

Not yet, we basically didn't know it might affect us until we got the test case done. And by that time, we were ready to go home; so I'll update you as soon as I know more.

[fredericDelaporte]

Actually with the first test case, it goes into an infinite loop if setting detect_fetch_loops to false without setting a max_fetch_depth.
Setting both does work around the issue for that first test case.

[BhaaLseN]

Thats curious though, there are no obvious loops in there 🤔

But I can see this being related, as the null case just flat-out doesn't load the entries; and it might determine that it needs to fetch more in this case? Didn't dig into the code changes, so I'm just guessing here.

[fredericDelaporte]

For these two settings to be a workaround, max_fetch_depth needs to be at least 2. This is because the first entity loaded, either a "person" or a "job" as per the test case, will also fetch its children and parents.

If the fetch depth is set to one, or the fetch loop detection enabled, the children and parents own children or parents will not be loaded, causing them to be loaded with empty collections.

The subsequent entity load, which will load the other entity, will then actually be discarded because that other entity is among the parents and children of the first loaded one, and so is already in the first level cache, but with empty collections.

To support such a case with the loop detection enabled, we would have to enable some kind of "fetch loops depth", allowing to walk duplicate associations as many time as asked depth. Or any better idea?

Test case: bef3bb7.

[BhaaLseN]

Not really, but that makes max_fetch_depth even more interresting to look at (if I understood that correctly). We have some historically deep structures that regularly cause us performance issues in some cases; and limiting the depth to a reasonable size could be a workaround until we get around to breaking them up properly.