Skip to content

Fix many-to-one disabled filters for entity joins #3048

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

Merged
merged 5 commits into from
Apr 28, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 83 additions & 0 deletions src/NHibernate.Test/Async/NHSpecificTest/GH3046/Fixture.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
//------------------------------------------------------------------------------
// <auto-generated>
// This code was generated by AsyncGenerator.
//
// Changes to this file may cause incorrect behavior and will be lost if
// the code is regenerated.
// </auto-generated>
//------------------------------------------------------------------------------


using System.Linq;
using NUnit.Framework;
using NHibernate.Linq;

namespace NHibernate.Test.NHSpecificTest.GH3046
{
using System.Threading.Tasks;
// Duplicated from GH2549 with minor changes to trigger the trouble.
[TestFixture]
public class FixtureAsync : BugTestCase
{
protected override void OnSetUp()
{
using (var s = OpenSession())
using (var t = s.BeginTransaction())
{
s.Save(new Person {Id = 1, Name = "Name"});
s.Save(new Customer {Deleted = false, Name = "Name", Id = 1});
s.Save(new Customer {Deleted = true, Name = "Name", Id = 2});

t.Commit();
}
}

protected override void OnTearDown()
{
using (var s = OpenSession())
using (var t = s.BeginTransaction())
{
s.CreateQuery("delete from System.Object").ExecuteUpdate();
t.Commit();
}
}

[Test]
public async Task EntityJoinFilterLinqAsync()
{
using (var s = OpenSession())
{
var list = await ((from p in s.Query<Person>()
join c in s.Query<Customer>() on p.Name equals c.Name
select p).ToListAsync());

s.EnableFilter("DeletedCustomer").SetParameter("deleted", false);

var filteredList = await ((from p in s.Query<Person>()
join c in s.Query<Customer>() on p.Name equals c.Name
select p).ToListAsync());

Assert.That(list, Has.Count.EqualTo(2));
Assert.That(filteredList, Has.Count.EqualTo(1));
}
}

[Test]
public async Task EntityJoinFilterQueryOverAsync()
{
using (var s = OpenSession())
{
Customer c = null;
Person p = null;
var list = await (s.QueryOver(() => p).JoinEntityAlias(() => c, () => c.Name == p.Name).ListAsync());

s.EnableFilter("DeletedCustomer").SetParameter("deleted", false);

var filteredList = await (s.QueryOver(() => p).JoinEntityAlias(() => c, () => c.Name == p.Name).ListAsync());

Assert.That(list, Has.Count.EqualTo(2));
Assert.That(filteredList, Has.Count.EqualTo(1));
}
}
}
}
71 changes: 71 additions & 0 deletions src/NHibernate.Test/NHSpecificTest/GH3046/Fixture.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
using System.Linq;
using NUnit.Framework;

namespace NHibernate.Test.NHSpecificTest.GH3046
{
// Duplicated from GH2549 with minor changes to trigger the trouble.
[TestFixture]
public class Fixture : BugTestCase
{
protected override void OnSetUp()
{
using (var s = OpenSession())
using (var t = s.BeginTransaction())
{
s.Save(new Person {Id = 1, Name = "Name"});
s.Save(new Customer {Deleted = false, Name = "Name", Id = 1});
s.Save(new Customer {Deleted = true, Name = "Name", Id = 2});

t.Commit();
}
}

protected override void OnTearDown()
{
using (var s = OpenSession())
using (var t = s.BeginTransaction())
{
s.CreateQuery("delete from System.Object").ExecuteUpdate();
t.Commit();
}
}

[Test]
public void EntityJoinFilterLinq()
{
using (var s = OpenSession())
{
var list = (from p in s.Query<Person>()
join c in s.Query<Customer>() on p.Name equals c.Name
select p).ToList();

s.EnableFilter("DeletedCustomer").SetParameter("deleted", false);

var filteredList = (from p in s.Query<Person>()
join c in s.Query<Customer>() on p.Name equals c.Name
select p).ToList();

Assert.That(list, Has.Count.EqualTo(2));
Assert.That(filteredList, Has.Count.EqualTo(1));
}
}

[Test]
public void EntityJoinFilterQueryOver()
{
using (var s = OpenSession())
{
Customer c = null;
Person p = null;
var list = s.QueryOver(() => p).JoinEntityAlias(() => c, () => c.Name == p.Name).List();

s.EnableFilter("DeletedCustomer").SetParameter("deleted", false);

var filteredList = s.QueryOver(() => p).JoinEntityAlias(() => c, () => c.Name == p.Name).List();

Assert.That(list, Has.Count.EqualTo(2));
Assert.That(filteredList, Has.Count.EqualTo(1));
}
}
}
}
26 changes: 26 additions & 0 deletions src/NHibernate.Test/NHSpecificTest/GH3046/Mappings.hbm.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?xml version="1.0" encoding="utf-8" ?>
<hibernate-mapping xmlns="urn:nhibernate-mapping-2.2" assembly="NHibernate.Test"
namespace="NHibernate.Test.NHSpecificTest.GH3046" >

<class name="Customer">
<id name="Id">
<generator class="assigned" />
</id>
<property name="Name" />
<property name="Deleted" type="Boolean" not-null="true" />

<filter name="DeletedCustomer" condition="Deleted = :deleted" />
</class>

<class name="Person">
<id name="Id">
<generator class="assigned" />
</id>
<property name="Name" />
</class>

<filter-def name="DeletedCustomer" use-many-to-one="false">
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use-many-to-one="false" is the only change compared to #2549 tests.

<filter-param name="deleted" type="Boolean"/>
</filter-def>

</hibernate-mapping>
15 changes: 15 additions & 0 deletions src/NHibernate.Test/NHSpecificTest/GH3046/Model.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
namespace NHibernate.Test.NHSpecificTest.GH3046
{
public class Customer
{
public virtual int Id { get; set; }
public virtual bool Deleted { get; set; }
public virtual string Name { get; set; }
}

public class Person
{
public virtual int Id { get; set; }
public virtual string Name { get; set; }
}
}
17 changes: 12 additions & 5 deletions src/NHibernate/Engine/JoinSequence.cs
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ internal virtual JoinFragment ToJoinFragment(
{
Join join = joins[i];
string on = join.AssociationType.GetOnCondition(join.Alias, factory, enabledFilters);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially though the GetOnCondition implementation may be troublesome in some cases, because it does filter enabledFilters according to their use-many-to-one setting:

public string GetOnCondition(string alias, ISessionFactoryImplementor factory, IDictionary<string, IFilter> enabledFilters)
{
if (IsReferenceToPrimaryKey)
{
//TODO: this is a bit arbitrary, expose a switch to the user?
return string.Empty;
}
else
{
return GetAssociatedJoinable(factory).FilterFragment(alias, FilterHelper.GetEnabledForManyToOne(enabledFilters));
}
}

And later here, we take the on clause if it is not empty, instead of computing it with all filters if ForceFilter. It appears this on clause will always be empty in our case, arbitrary joins, because GetOnCondition current implementation always yields that excepted for property-ref associations.

public bool IsReferenceToPrimaryKey
{
get { return string.IsNullOrEmpty(uniqueKeyPropertyName); }
}

So, the current change is enough to fix the trouble. I cannot think of a related test case demonstrating a need to change GetOnCondition. This implementation looks a bit brittle to me, but a patch change is not the place to get things better from that viewpoint, unless we can demonstrate a failing case.

SqlString condition = new SqlString();
SqlString condition;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little cleanup by the way.

if (last != null &&
IsManyToManyRoot(last) &&
((IQueryableCollection)last).ElementType == join.AssociationType)
Expand All @@ -195,10 +195,17 @@ internal virtual JoinFragment ToJoinFragment(
{
// NH Different behavior : NH1179 and NH1293
// Apply filters for entity joins and Many-To-One association
var enabledForManyToOne = FilterHelper.GetEnabledForManyToOne(enabledFilters);
condition = new SqlString(string.IsNullOrEmpty(on) && (ForceFilter || enabledForManyToOne.Count > 0)
? join.Joinable.FilterFragment(join.Alias, enabledForManyToOne)
: on);
if (string.IsNullOrEmpty(on))
{
var enabledFiltersForJoin = ForceFilter ? enabledFilters : FilterHelper.GetEnabledForManyToOne(enabledFilters);
condition = new SqlString(enabledFiltersForJoin.Count > 0
? join.Joinable.FilterFragment(join.Alias, enabledFiltersForJoin)
: on);
}
else
{
condition = new SqlString(on);
}
}

if (withClauseFragment != null)
Expand Down
5 changes: 3 additions & 2 deletions src/NHibernate/Loader/JoinWalker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -853,9 +853,10 @@ protected JoinFragment MergeOuterJoins(IList<OuterJoinableAssociation> associati
oj.AddJoins(outerjoin);
// NH Different behavior : NH1179 and NH1293
// Apply filters for entity joins and Many-To-One associations
if (oj.ForceFilter || enabledFiltersForManyToOne.Count > 0)
var enabledFiltersForJoin = oj.ForceFilter ? enabledFilters : enabledFiltersForManyToOne;
if (enabledFiltersForJoin.Count > 0)
{
string manyToOneFilterFragment = oj.Joinable.FilterFragment(oj.RHSAlias, enabledFiltersForManyToOne);
string manyToOneFilterFragment = oj.Joinable.FilterFragment(oj.RHSAlias, enabledFiltersForJoin);
bool joinClauseDoesNotContainsFilterAlready =
outerjoin.ToFromFragmentString.IndexOfCaseInsensitive(manyToOneFilterFragment) == -1;
if (joinClauseDoesNotContainsFilterAlready)
Expand Down