Collection filter on joined subclass #3079

micmerchant · 2022-07-14T07:56:51Z

Hello,

there is an problem with the generation of SQL-Code when a collection filter is set on a joined subclass. Here is an example mapping (modified existing joined-subclass.hbm.xml).

<?xml version='1.0' encoding='utf-8'?>
<hibernate-mapping	assembly='NHibernate.Test'
                    namespace='NHibernate.Test.SubclassFilterTest'
                    xmlns='urn:nhibernate-mapping-2.2'>
	<class name="Person" table="JPerson">

		<id name="Id" column="person_id">
			<generator class="native"/>
		</id>

		<property name="Name"/>
		<property name="Company"/>
		<property name="Region"/>

		<joined-subclass name="Employee">
			<key column="person_id" />
			<property name="Title"/>
			<property name="Department" column="dept"/>
			<many-to-one name="Manager" column="mgr_id" class="Employee" cascade="none"/>
      <many-to-one name="SharedCar"/>
			<set name="Minions" inverse="true" lazy="true" cascade="all">
				<key column="mgr_id"/>
				<one-to-many class="Employee"/>
			</set>
		</joined-subclass>

		<joined-subclass name="Customer">
			<key column="person_id" />
			<many-to-one name="ContactOwner" class="Employee"/>
		</joined-subclass>

		<filter name="region" condition="Region = :userRegion"/>

	</class>

  <class name="Car">

    <id name="Id" column="car_id" >
      <generator class="native"/>
    </id>
    
    <property name="LicensePlate"/>

    <bag cascade="all-delete-orphan" inverse="true" name="Drivers">
      <key>
        <column name="SharedCar" />
      </key>
      <one-to-many class="Employee" />
      <filter name="region" condition="Region = :userRegion"/>
    </bag>

  </class>

	<filter-def name="region">
		<filter-param name="userRegion" type="string"/>
	</filter-def>
  
</hibernate-mapping>

Test case to display the issue:

JoinedSubclassFilterTest::

[Test(Description = "Tests the joined subclass collection filter of a single table with a collection mapping " +
					"on the parent class.")]
public void FilterCollectionJoinedSubclass()
{
	using(ISession session = OpenSession())
		using(ITransaction t = session.BeginTransaction())
		{
			PrepareTestData(session);

			// sets the filter
			session.EnableFilter("region").SetParameter("userRegion", "US");

			var result = session.Query<Car>()
								.Where(c => c.Drivers.Any())
								.ToList();
			
			Assert.AreEqual(1, result.Count);

			t.Rollback();
			session.Close();
		}
}

The filter on the collection mapping of the Car entity targeting a joined subclass causes this issue. The property is defined in the base class of the inheritance relation.

But there is a difference in SQL generation between your Master (developing branch?) and your 5.3. (stable?) branch.

Master:

-- Statement #1
select
	
        car0_.car_id as car1_4_,
	
        car0_.Tenant as tenant2_4_,
	
        car0_.LicensePlate as licenseplate3_4_ 
    
from
        car car0_ 
    
where
        (
	
            car0_.Tenant & 1 /* @p0 */
        )
 != 0 
        and
	 (
		
            exists (
			
                select
				
                    drivers1_.person_id 
                
			from
                    
                inner join
                    (
				
                        empl drivers1_ 
                    inner join
                        person drivers1_1_ 
                            on drivers1_.person_id=drivers1_1_.person_id
                        )
			 
                            on car0_.car_id=drivers1_.SharedCar 
                            and
				 (
					
                                drivers1_1_.Tenant & 1 /* @p0 */
                            )
				 != 0
                    )
			
                )
		

-- Statement #2
ERROR: 
Could not execute query: select car0_.car_id as car1_4_, car0_.Tenant as tenant2_4_, car0_.LicensePlate as licenseplate3_4_ from car car0_ where (car0_.Tenant & @p0) != 0 and (exists (select drivers1_.person_id from inner join  (empl drivers1_ inner join person drivers1_1_ on drivers1_.person_id=drivers1_1_.person_id) on car0_.car_id=drivers1_.SharedCar and (drivers1_1_.Tenant & @p0) != 0))
Incorrect syntax near the keyword 'inner'.

--//////////////////////////////////////////////////

It seems there is a issue with join generation.

5.3:

-- Statement #1
select car0_.car_id       as car1_3_,
       car0_.LicensePlate as licenseplate2_3_
from   Car car0_
where  exists ( select drivers1_.person_id
                from   Employee drivers1_
                       inner join JPerson drivers1_1_
                       on drivers1_.person_id = drivers1_1_.person_id
                where  car0_.car_id = drivers1_.SharedCar
                       and drivers1_.Region = 'US' /* @p0 */ )

-- Statement #2
ERROR: 
Could not execute query: select car0_.car_id as car1_3_, car0_.LicensePlate as licenseplate2_3_ from Car car0_ where exists (select drivers1_.person_id from Employee drivers1_ inner join JPerson drivers1_1_ on drivers1_.person_id=drivers1_1_.person_id where car0_.car_id=drivers1_.SharedCar and drivers1_.Region = @p0)
Invalid column name 'Region'.

--//////////////////////////////////////////////////

There is an issue evaluating the proper table alias.

The fix targets the 5.3 branch and tries to forward the filter generation to the underlying entity persister:

AbstractCollectionPersister::

/// <summary>
/// Get the where clause filter, given a query alias and considering enabled session filters.
///
/// When an <see cref="IEntityPersister"/> is set, the filter generated by the persister is returned. This ensures
/// that a proper filter is also generated for subclass hierarchy relations.
/// </summary>
/// <param name="alias"></param>
/// <param name="enabledFilters"></param>
/// <returns></returns>
public virtual string FilterFragment(string alias, IDictionary<string, IFilter> enabledFilters)
{
	if(!enabledFilters.Any())
	{
		// no filters enabled
		// but a potential where condition needs to be handled properly
		return FilterFragment(alias);
	}
	
	if(!FilterHelper.IsAffectedBy(enabledFilters))
	{
		// the collection filters don't match with the enabled filters
		// but a potential where condition needs to be handled properly
		return FilterFragment(alias);
	}
	
	if(ElementPersister != null && 
	   ElementPersister.FilterHelper.IsAffectedBy(enabledFilters))
	{
		// forward filter generation to persister when it is affected by the filter
		return ElementPersister.FilterFragment(alias, enabledFilters);
	}
	
	// default handling
	StringBuilder sessionFilterFragment = new StringBuilder();
	FilterHelper.Render(sessionFilterFragment, alias, enabledFilters);

	return sessionFilterFragment.Append(FilterFragment(alias)).ToString();
}

Although it works, maybe there is a better way to evaluate the necessity to forward the generation call to the persister.

BR Michael

…for Collection-Filters

micmerchant · 2022-07-26T07:17:40Z

After the Master-Merge the new Testcase fails now with the already mentioned error:

NHibernate.Exceptions.GenericADOException : could not execute query
[ select car0_.car_id as car1_3_, car0_.LicensePlate as licenseplate2_3_ from Car car0_ where exists (select drivers1_.person_id from inner join  (Employee drivers1_ inner join JPerson drivers1_1_ on drivers1_.person_id=drivers1_1_.person_id) on car0_.car_id=drivers1_.SharedCar and drivers1_1_.Region = @p0) ]
[SQL: select car0_.car_id as car1_3_, car0_.LicensePlate as licenseplate2_3_ from Car car0_ where exists (select drivers1_.person_id from inner join  (Employee drivers1_ inner join JPerson drivers1_1_ on drivers1_.person_id=drivers1_1_.person_id) on car0_.car_id=drivers1_.SharedCar and drivers1_1_.Region = @p0)]
  ----> System.Data.SqlClient.SqlException : Incorrect syntax near the keyword 'inner'.

I don't know how to fix this one. I don't think it is related to my fix, cause the new test case also fails with this message without my changes.

hazzik · 2022-07-27T04:39:31Z

So, your fix is for 5.3?

micmerchant · 2022-07-27T06:17:12Z

Yes, as I mentioned initially, the fix targets the 5.3 branch. The Master branch (i guess it's you current development branch) causes further issues at generating proper SQL in case of joins.

hazzik · 2022-07-29T00:43:03Z

the fix targets the 5.3 branch

The PR was pointing to the master branch, so I got confused.

Another question @micmerchant. Did it work before?

micmerchant · 2022-07-29T07:03:05Z

I guess, we are both confused now.

I branched from here (but I haven't really thought about it, why I've started from here :) )

But the point is .. the following test causes different issues in Master and in 5.3.x with the above mapping:

[Test(Description = "Tests the joined subclass collection filter of a single table with a collection mapping " +
					"on the parent class.")]
public void FilterCollectionJoinedSubclass()
{
	using(ISession session = OpenSession())
		using(ITransaction t = session.BeginTransaction())
		{
			PrepareTestData(session);

			// sets the filter
			session.EnableFilter("region").SetParameter("userRegion", "US");

			var result = session.Query<Car>()
								.Where(c => c.Drivers.Any())
								.ToList();
			
			Assert.AreEqual(1, result.Count);

			t.Rollback();
			session.Close();
		}
}

1) 5.3.x: proper SQL-Syntax is generated, but the query isn't executable, because the wrong table alias is resolved.

ERROR: 
Could not execute query: select car0_.car_id as car1_3_, car0_.LicensePlate as licenseplate2_3_ from Car car0_ where exists (select drivers1_.person_id from Employee drivers1_ inner join JPerson drivers1_1_ on drivers1_.person_id=drivers1_1_.person_id where car0_.car_id=drivers1_.SharedCar and drivers1_.Region = @p0)
Invalid column name 'Region'.

2) Master: invalid SQL-Syntax is generated; there is some kind of a Join SQL problem.

ERROR: 
Could not execute query: select car0_.car_id as car1_4_, car0_.Tenant as tenant2_4_, car0_.LicensePlate as licenseplate3_4_ from car car0_ where (car0_.Tenant & @p0) != 0 and (exists (select drivers1_.person_id from inner join  (empl drivers1_ inner join person drivers1_1_ on drivers1_.person_id=drivers1_1_.person_id) on car0_.car_id=drivers1_.SharedCar and (drivers1_1_.Tenant & @p0) != 0))
Incorrect syntax near the keyword 'inner'.

My fix targets the first issue with resolving the wrong table alias.

I hope that clears it a bit up.

BR,
Michael

bahusoid · 2022-08-08T09:03:11Z

It's not a regression so it's not applicable for 5.3 branch. You should target master branch. And master SQL syntax problem needs to be investigated anyway.

bahusoid · 2022-08-10T14:05:07Z

I believe I fixed issue with invalid SQL syntax on master in #3106. I also reproduced your issue using existing test data (see JoinedSubclassFilterTest.FilterCollectionWithSubclass2)

…_JoinedSubclass # Conflicts: # src/NHibernate.Test/Async/SubclassFilterTest/JoinedSubclassFilterTest.cs # src/NHibernate.Test/SubclassFilterTest/JoinedSubclassFilterTest.cs # src/NHibernate.Test/SubclassFilterTest/joined-subclass.hbm.xml

micmerchant · 2022-11-25T12:09:13Z

I finally found the time to merge the current Master (5.4) into this Branch. Yes, it seems that the SQL generation issues are fixed and at least my test cases are working.

But my changes don't fix the alias resolution for your mapping changes (@bahusoid), what you also mention in one of the test cases. I'm not sure if got the time to take a look at it or if i have enough knowledge about the inner working of NHibernate to fix it.

But, what I know is, if my fix doesn't fix all cases .. then it's not a real fix :)

micmerchant · 2022-11-25T17:20:31Z

Honestly, I initially thought that the filter name of a collection filter must match the entity filter name. In our application this is the case, but we are using the session filters as a tenant filter which is applied to all entities. In your changed mapping the filter names don't match, but if you change them to an equal name

the filter is properly applied but it could lead to a different semantic.

In this example there are at least two options:

Do you want to load managers from the US with employees from the US?
Do you want to load managers where only the employees are from the US?

So i guess, my fix doesn't work for the latter case.

Additionally the filter is not applied to the lazy loading of minions itself .. but I don't know where that lazy evaluation happens in code.

micmerchant · 2022-11-25T19:21:57Z

After taking a shower and rethinking the issue I came to the conclusion, that the mentioned issues may still be solved:

lazy evaluation of collection:

Minions.Count failed because the minions were loaded from the session. I haven't cleared the session after data creation and before loading the data. After clearing the expected 2nd query was executed and the test case was successfull.

different semantics:
Well, honestly I don't think session filters are appropriate for this use case. I believe Linq filters or similar are way more flexible and appropriate.

What if you point out the limitations of session filters and their suitable areas of application in the documentation? A quick note about inheritance mappings and collection mappings should do it :). Of course only if you accept the PR which makes a matching session filter name in such mappings mandatory. I think a documentation update about the limitations is necessary anyway.

bahusoid

I initially thought that the filter name of a collection filter must match the entity filter name

Collection filters are separate from entity filters. The fact that your fix only works if the same entity filter exists makes your fix invalid.

src/NHibernate/Persister/Collection/AbstractCollectionPersister.cs

micmerchant · 2022-11-28T06:59:01Z

Collection filters are separate from entity filters. The fact that your fix only works if the same entity filter exists makes your fix invalid.

Legit.

Now that I understand a little bit more, I'll rethink it and try to find an appropriate fix.

…persister - entity persister only provides the column to table alias mapping anymore

micmerchant · 2022-11-28T15:23:10Z

Update:
I've decoupled the generation of the filter code from the EntityPersister now. The EntityPersister just provides the column to table alias mapping anymore. So for collections and entities, the same logic to resolve the proper table alias for the filter generation is used.

micmerchant · 2022-12-06T16:06:53Z

@bahusoid

How to resolve "Changes requested" state? I guess, i was too fast with requesting another review, before i resolved the conversation :).

.

bahusoid

It's a breaking change in current state - you shouldn't extend interfaces but only implementations (like add IFilterable only to classes)
But anyway, I don't think we need FilterHelper exposed everywhere. I spend some time on it and going to prepare a replacement PR with minimal API changes.

fredericDelaporte · 2023-04-03T21:17:35Z

Fixed by #3264 instead.

mika added 3 commits July 14, 2022 09:16

TestCase added which fails due to a wrongly created filter condition …

4a21b51

…for Collection-Filters

fixes the generation of Collection-Filters on a JoinedSubclass

cdeb20d

multiple blank lines issue fixed

8c3c86c

hazzik force-pushed the CollectionFilter_On_JoinedSubclass branch from 0e40fdb to 8c3c86c Compare July 27, 2022 04:42

hazzik changed the base branch from master to 5.3.x July 27, 2022 04:43

bahusoid mentioned this pull request Aug 18, 2022

Skip table group join processing for implicit join #3106

Merged

bahusoid changed the base branch from 5.3.x to master November 25, 2022 14:42

bahusoid and others added 2 commits November 25, 2022 17:03

Enable test

94a2cd9

Generate async files

c898946

mapping fixed to fix the test cases

c57be19

bahusoid previously requested changes Nov 26, 2022

View reviewed changes

src/NHibernate/Persister/Collection/AbstractCollectionPersister.cs Outdated Show resolved Hide resolved

- generation of the filter for collections decoupled from the entity …

9afc08c

…persister - entity persister only provides the column to table alias mapping anymore

micmerchant requested a review from bahusoid November 28, 2022 15:25

bahusoid reviewed Mar 22, 2023

View reviewed changes

bahusoid mentioned this pull request Mar 22, 2023

Fix collection filter on subclass columns #3264

Merged

fredericDelaporte added t: Bug c: Core p: Minor labels Mar 23, 2023

hazzik added r: Fixed r: Rejected and removed r: Fixed labels Apr 2, 2023

hazzik closed this in 160e35d Apr 3, 2023

fredericDelaporte added r: Replaced and removed r: Rejected labels Apr 3, 2023

Collection filter on joined subclass #3079

Collection filter on joined subclass #3079

Uh oh!

Conversation

micmerchant commented Jul 14, 2022

Uh oh!

micmerchant commented Jul 26, 2022

Uh oh!

hazzik commented Jul 27, 2022

Uh oh!

micmerchant commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hazzik commented Jul 29, 2022

Uh oh!

micmerchant commented Jul 29, 2022

Uh oh!

bahusoid commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bahusoid commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

micmerchant commented Nov 25, 2022

Uh oh!

micmerchant commented Nov 25, 2022

Uh oh!

micmerchant commented Nov 25, 2022

Uh oh!

bahusoid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

micmerchant commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

micmerchant commented Nov 28, 2022

Uh oh!

micmerchant commented Dec 6, 2022

Uh oh!

bahusoid left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Apr 3, 2023

Uh oh!

Uh oh!

micmerchant commented Jul 27, 2022 •

edited

Loading

bahusoid commented Aug 8, 2022 •

edited

Loading

bahusoid commented Aug 10, 2022 •

edited

Loading

micmerchant commented Nov 28, 2022 •

edited

Loading

bahusoid left a comment •

edited

Loading