Skip to content

problem with join with withClause in case of inheritance #2331

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

Open
wants to merge 49 commits into
base: master
Choose a base branch
from

Conversation

georgi-yakimov
Copy link

@georgi-yakimov georgi-yakimov commented Mar 19, 2020

Fixes #2330

@gliljas
Copy link
Member

gliljas commented Mar 19, 2020

Thanks! Please add tests too.

@georgi-yakimov
Copy link
Author

hi @gliljas what do I need in order to run the tests locally? I have both nunit 2 and 3 installed. For some reason the tests are not getting started. Where I can read more about the unit tests practices you're following?

@gliljas
Copy link
Member

gliljas commented Mar 19, 2020

NUnit 3 is what is used. Personally I use the ReSharper test runner, but the one built into VS should work too. You should run the ShowBuildMenu.bat file in the project root.

@georgi-yakimov
Copy link
Author

The issue is not yet finished. I have to add unit tests.

@fredericDelaporte fredericDelaporte changed the title override of GetSubclassPropertyTableNumber in JoinedSubclassEntityPer… WIP - override of GetSubclassPropertyTableNumber in JoinedSubclassEntityPer… Mar 19, 2020
@georgi-yakimov georgi-yakimov changed the title WIP - override of GetSubclassPropertyTableNumber in JoinedSubclassEntityPer… override of GetSubclassPropertyTableNumber in JoinedSubclassEntityPer… Apr 9, 2020
@georgi-yakimov
Copy link
Author

Hi can someone please check what's going on with the travis? It's been in waiting status since the weekend? Thanks in advance

@fredericDelaporte
Copy link
Member

This sometimes happens, and usually pushing new commit solves it. As anyway you have a conflict to resolve and new issues in CodeFactor, you have some commits to add (eventually with a rebase), so let's see if the issue remains after that.

@georgi-yakimov
Copy link
Author

Hi. continuous-integration/appveyor/pr is failing and I can't figure out the reason why with looking at the log. Is it possible that the failure is not due to my changes?

@hazzik
Copy link
Member

hazzik commented Apr 15, 2020

@georgi-yakimov in your issue which you have originally opened you are talking about HQL, but it seems this PR concerns only Criteria API. Can you please clarify?

@hazzik hazzik closed this Apr 15, 2020
@hazzik hazzik reopened this Apr 15, 2020
@georgi-yakimov
Copy link
Author

georgi-yakimov commented Apr 16, 2020

hi @hazzik the problem is that when there's inheritance join with a withClause and the column which is used in the withClause is present in the base and the inherited table the condition is not generated correctly. The table that is considered in the with clause is from the base table which is not yet joined. So instead of

select t1.*
from SomeTable t1
join Child c_1 on c_1.FK_Root_Id = t1.FK_Child_Id and c_1.IsActive = 1
join Root c_1_1 on c_1_1.Id = c_1.FK_Root_Id

the result is

select t1.*
from SomeTable t1
join Child c_1 on c_1.FK_Root_Id = t1.FK_Child_Id and c_1_1.IsActive = 1 -- c_1_1 is not joined yet
join Root c_1_1 on c_1_1.Id = c_1.FK_Root_Id

You can check the unit tests I've added for more details

@bahusoid
Copy link
Member

This fix with use first/last index looks more like workaround for some real core issue. Can you explain more what's behind this fix?

Also I tried to convert your query to QueryOver which is basically a typed wrapper around Criteria and for some reasons your fix is no longer working:

PersonBase f = null;
var results =
	session.QueryOver<UserEntityVisit>()
			.JoinAlias(
				x => x.PersonBase,
				() => f,
				JoinType.LeftOuterJoin,
				Restrictions.Where(() => f.Deleted == false))
			.List();

@bahusoid
Copy link
Member

bahusoid commented Apr 16, 2020

Also I might be wrong but it seems some of analysis described here #2094 might be related to your issue

@georgi-yakimov georgi-yakimov changed the title override of GetSubclassPropertyTableNumber in JoinedSubclassEntityPer… problem with join with withClause in case of inheritance Apr 23, 2020
@georgi-yakimov
Copy link
Author

hey, could somebody please take a look why a required check is hanging for like a week by now? Thanks in advance

@@ -2110,13 +2110,16 @@ public virtual int GetSubclassPropertyTableNumber(string propertyPath)
return getSubclassColumnTableNumberClosure()[idx];
}
}*/
int index = Array.IndexOf(SubclassPropertyNameClosure, rootPropertyName); //TODO: optimize this better!
return index == -1 ? 0 : GetSubclassPropertyTableNumber(index);
int index = useLastIndex
Copy link
Member

Choose a reason for hiding this comment

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

what would be the case when useLastIndex is required to be false?

@hazzik
Copy link
Member

hazzik commented May 12, 2020

This really looks like a workaround. It is easy to break this logic.

If you add a subclass of PersonBase with Deleted property, then it will be broken.

The issue here is much deeper: only properties of the class itself are supported in the "with" clause.

@bahusoid
Copy link
Member

bahusoid commented Oct 23, 2020

#2330 is actually also fixed by table group joins (#2545 for Criteria and #2361 for hql). So it seems this PR can be closed as replaced

So problematic SQL below (taken from problem description):

select t1.*
from SomeTable t1
join Child c_1 on c_1.FK_Root_Id = t1.FK_Child_Id and c_1_1.IsActive = 1 -- c_1_1 is not joined yet
join Root c_1_1 on c_1_1.Id = c_1.FK_Root_Id

Become after fix:

select t1.*
from SomeTable t1
join 
   (Child c_1  join Root c_1_1 on c_1_1.Id = c_1.FK_Root_Id)  on c_1.FK_Root_Id = t1.FK_Child_Id and c_1_1.IsActive = 1

@fredericDelaporte
Copy link
Member

After having undone the fix in this PR, its tests work locally. So I have pushed the undo to check this with the CI.

@fredericDelaporte
Copy link
Member

So the test case alone without any change to NHibernate current master branch succeeds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

join on multiple conditions
7 participants