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

Conversation

fredericDelaporte
Copy link
Member

fix #3046

@fredericDelaporte fredericDelaporte added this to the 5.3.12 milestone Apr 24, 2022
<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.

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.

@@ -175,7 +175,7 @@ public JoinFragment ToJoinFragment(IDictionary<string, IFilter> enabledFilters,
{
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.

Comment on lines +200 to +201
var enabledFiltersForJoin = ForceFilter ? enabledFilters : FilterHelper.GetEnabledForManyToOne(enabledFilters);
condition = new SqlString(ForceFilter || enabledFiltersForJoin.Count > 0
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.

@bahusoid bahusoid changed the title Fix many-to-one disabled filters for arbitrary joins Fix many-to-one disabled filters for entity joins Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants