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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
10a5d4c
Fix many-to-one disabled filters for arbitrary joins
fredericDelaporte Apr 24, 2022
0eb73ae
Refix discriminator cases
fredericDelaporte Apr 24, 2022
9f99166
Reuse original fixture class
fredericDelaporte Apr 26, 2022
0420362
Just extend GH2549 tests instead
fredericDelaporte Apr 27, 2022
890a131
Adjust a comment
fredericDelaporte Apr 27, 2022
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
13 changes: 7 additions & 6 deletions src/NHibernate.Test/Async/NHSpecificTest/GH2549/Fixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
namespace NHibernate.Test.NHSpecificTest.GH2549
{
using System.Threading.Tasks;
// Test GH3046 too, when useManyToOne is <c>true</c>.
[TestFixture]
public class FixtureAsync : BugTestCase
{
Expand All @@ -41,16 +42,16 @@ protected override void OnTearDown()
}
}

[Test]
public async Task EntityJoinFilterLinqAsync()
[Theory]
public async Task EntityJoinFilterLinqAsync(bool useManyToOne)
{
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);
s.EnableFilter(useManyToOne ? "DeletedCustomer" : "DeletedCustomerNoManyToOne").SetParameter("deleted", false);

var filteredList = await ((from p in s.Query<Person>()
join c in s.Query<Customer>() on p.Name equals c.Name
Expand All @@ -61,16 +62,16 @@ join c in s.Query<Customer>() on p.Name equals c.Name
}
}

[Test]
public async Task EntityJoinFilterQueryOverAsync()
[Theory]
public async Task EntityJoinFilterQueryOverAsync(bool useManyToOne)
{
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);
s.EnableFilter(useManyToOne ? "DeletedCustomer" : "DeletedCustomerNoManyToOne").SetParameter("deleted", false);

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

Expand Down
13 changes: 7 additions & 6 deletions src/NHibernate.Test/NHSpecificTest/GH2549/Fixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

namespace NHibernate.Test.NHSpecificTest.GH2549
{
// Test GH3046 too, when useManyToOne is <c>true</c>.
[TestFixture]
public class Fixture : BugTestCase
{
Expand All @@ -29,16 +30,16 @@ protected override void OnTearDown()
}
}

[Test]
public void EntityJoinFilterLinq()
[Theory]
public void EntityJoinFilterLinq(bool useManyToOne)
{
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);
s.EnableFilter(useManyToOne ? "DeletedCustomer" : "DeletedCustomerNoManyToOne").SetParameter("deleted", false);

var filteredList = (from p in s.Query<Person>()
join c in s.Query<Customer>() on p.Name equals c.Name
Expand All @@ -49,16 +50,16 @@ join c in s.Query<Customer>() on p.Name equals c.Name
}
}

[Test]
public void EntityJoinFilterQueryOver()
[Theory]
public void EntityJoinFilterQueryOver(bool useManyToOne)
{
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);
s.EnableFilter(useManyToOne ? "DeletedCustomer" : "DeletedCustomerNoManyToOne").SetParameter("deleted", false);

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

Expand Down
5 changes: 5 additions & 0 deletions src/NHibernate.Test/NHSpecificTest/GH2549/Mappings.hbm.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
<property name="Deleted" type="Boolean" not-null="true" />

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

<class name="Person">
Expand All @@ -23,4 +24,8 @@
<filter-param name="deleted" type="Boolean"/>
</filter-def>

<filter-def name="DeletedCustomerNoManyToOne" use-many-to-one="false">
<filter-param name="deleted" type="Boolean"/>
</filter-def>

</hibernate-mapping>
17 changes: 12 additions & 5 deletions src/NHibernate/Engine/JoinSequence.cs
View file
Open in desktop
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:

nhibernate-core/src/NHibernate/Type/EntityType.cs

Lines 501 to 512 in e99cf43

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.

nhibernate-core/src/NHibernate/Type/EntityType.cs

Lines 496 to 499 in e99cf43

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(ForceFilter || enabledFiltersForJoin.Count > 0
Comment on lines +200 to +201
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 have considered ForceFilter was meant to apply all filters, not just many-to-one enabled filters. But maybe I am wrong and it is here just for discriminator filters and other non session filters related filtering. In such case this change may not be suitable.

ForceFilter is enabled for entity joins. I think entity joins are arbitrary joins and should not respect the many-to-one filter setting, which seems to me about entity association joins. In such case, this fix would be legit.

But if instead we consider the many-to-one filter setting should be taken into account for arbitrary joins, meaning the filters should be disabled on arbitrary joins if that setting is disabled, then we should instead just add a breaking change in the releases note for v5.3.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
View file
Open in desktop
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 (oj.ForceFilter || 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