Support to join not associated entities in Criteria (aka Entity Join) #1545

bahusoid · 2018-01-20T14:23:16Z

With this PR it's possible to join entities with not mapped association in QueryOver via JoinEntityQueryOver/JoinEntityAlias methods:

Entity root = null;
NotRelatedEntity notRelatedEntity = null;

root = session.QueryOver(() => root)
//Join on not mapped entity
.JoinEntityQueryOver(() => notRelatedEntity, () => notRelatedEntity.Id == root.NotRelatedEntityId)
//Also preload child for NotRelatedEntity
.JoinQueryOver(nre => nre.SomeChild)
.Take(1)
.SingleOrDefault();

//Obtain from session already preloaded notRelatedEntity along with notRelatedEntity.SomeChild
notRelatedEntity = session.Get<NotRelatedEntity>(root.NotRelatedEntityId);

See other options in tests

"Entity Join" name comes from Hibernate (where it's implemented for hql only):
http://in.relation.to/2016/02/10/hibernate-orm-510-final-release/

fredericDelaporte

This is just a quick review for now, I should do a more thorough one later.

fredericDelaporte · 2018-01-22T19:20:21Z

src/NHibernate/Criterion/QueryOver.cs

@@ -280,7 +284,8 @@ IQueryOver<TRoot> IQueryOver<TRoot>.ReadOnly()
 	/// Implementation of the <see cref="IQueryOver&lt;TRoot, TSubType&gt;"/> interface
 	/// </summary>
 	[Serializable]
-	public class QueryOver<TRoot,TSubType> : QueryOver<TRoot>, IQueryOver<TRoot,TSubType>
+	public class QueryOver<TRoot,TSubType> : QueryOver<TRoot>, IQueryOver<TRoot,TSubType>,
+		ISupportEntityJoinQueryOver<TRoot>


I have not seen the reason for implementing this interface on this class rather than on QueryOver<TRoot>. Why this choice?

Maybe because that is the way it looks to be done for other similar cases. But then, ISupportEntityJoinQueryOver<TRoot> "6.0 todo" should be "merge in IQueryOver<TRoot, TSubType>.

Yeah I've just put it next to existing JoinAlias/JoinQueryOver methods. Looks more natural to have them in the same place.

fredericDelaporte · 2018-01-22T19:22:21Z

src/NHibernate/Criterion/ISupportEntityJoinQueryOver.cs

+namespace NHibernate.Criterion
+{
+	//TODO: Make interface more flexible for changes (maybe it should take only 2 params alias + EntityJoinConfig)
+	public interface ISupportEntityJoinQueryOver


I would add a " 6.0 TODO: merge with IQueryOver". Is there a reason for not aiming at this? The same apply to below interface, but of course with IQueryOver<TRoot>.

fredericDelaporte · 2018-01-22T19:25:17Z

src/NHibernate/Impl/ISupportEntityJoinCriteria.cs

+
+namespace NHibernate.Impl
+{
+	public interface ISupportEntityJoinCriteria


I would add a " 6.0 TODO: merge with ICriteria". Is there a reason for not aiming at this?

fredericDelaporte · 2018-01-22T19:28:23Z

src/NHibernate/EntityJoinExtensions.cs

+	public static class EntityJoinExtensions
+	{
+		// 6.0 TODO: merge into 'IQueryOver<TType, TSubType>
+		public static TThis JoinEntityAlias<TThis, TAlias>(this TThis queryOver, Expression<Func<TAlias>> alias, JoinType joinType, ICriterion withClause, string entityName = null) where TThis : IQueryOver


So that is somewhat the todo I am asking for on interface.

fredericDelaporte · 2018-01-22T19:30:55Z

src/NHibernate/Loader/Criteria/CriteriaQueryTranslator.cs

+			{
+				last = entityJoinInfo.Persister;
+				lastEntity = (IPropertyMapping) last;
+				++i;


i++; is more common place, and we do not need pre-increment here.

Just to add to this: if preferring ++i; other i++; for "performance", this is an old performance "trick", obsolete for C, potentially still relevant for C++ when applied to an object but well, we are using C#. See here. Optimizing such trivial things is not a developer job but a compiler job. I am on the side on optimizing the developer work by letting compiler responsible of such optimizations, and letting developer write most readable code possible. This includes using most common places syntaxes unless there is a real and significant impact for the application.

No, of course It's not about optimization :) it's about habit - I just use ++i; since C++ and it's more common for me.
I have no objections to change it but curious why do you think that i++ is more common for C#? Is it because VS/Resharper expands for with i++?

P.S. I don't have much time this week but I will try to address all the questions/concerns next week.

Is it because VS/Resharper expands for with i++?

No. Just because I have far more seen post-increments than pre-increments in C#, and I do not think I am alone. Alexander had made the same demand, switching away from ++i to i++ on a previous PR. The opener argued for performance as far a I remember, and it was discarded as not relevant in C#.

This was here.

fredericDelaporte · 2018-01-23T11:24:12Z

src/NHibernate/Impl/CriteriaImpl.cs

@@ -71,7 +72,7 @@ public CriteriaImpl(string entityOrClassName, string alias, ISessionImplementor
 			rootAlias = alias;
 			subcriteriaByAlias[alias] = this;
 		}
-
+		


Undue whitespace addition.

fredericDelaporte · 2018-01-23T11:39:30Z

src/NHibernate/Criterion/QueryOver.cs

@@ -280,7 +284,8 @@ IQueryOver<TRoot> IQueryOver<TRoot>.ReadOnly()
 	/// Implementation of the <see cref="IQueryOver&lt;TRoot, TSubType&gt;"/> interface
 	/// </summary>
 	[Serializable]
-	public class QueryOver<TRoot,TSubType> : QueryOver<TRoot>, IQueryOver<TRoot,TSubType>
+	public class QueryOver<TRoot,TSubType> : QueryOver<TRoot>, IQueryOver<TRoot,TSubType>,
+		ISupportEntityJoinQueryOver<TRoot>


Maybe because that is the way it looks to be done for other similar cases. But then, ISupportEntityJoinQueryOver<TRoot> "6.0 todo" should be "merge in IQueryOver<TRoot, TSubType>.

fredericDelaporte · 2018-01-23T11:45:51Z

src/NHibernate/Impl/CriteriaImpl.cs

@@ -668,6 +675,16 @@ internal Subcriteria(CriteriaImpl root, ICriteria parent, string path, string al
 			internal Subcriteria(CriteriaImpl root, ICriteria parent, string path, JoinType joinType)
 				: this(root, parent, path, null, joinType) { }

+			/// <summary>
+			/// Entity name for "Entity Join" - join for enitity with not mapped association


Typo, enitity.

fredericDelaporte · 2018-01-23T11:47:39Z

src/NHibernate/Loader/Criteria/CriteriaJoinWalker.cs

+				var tableAlias = translator.GetSQLAlias(entityJoinInfo.Criteria);
+				var criteriaPath = entityJoinInfo.Criteria.Alias; //path for entity join is equal to alias
+				var persister
+					= entityJoinInfo.Persister as IOuterJoinLoadable;


A single line here should be enough. (And it is not an usual formatting to have the affectation operator on the next line instead of at the end of line.)

fredericDelaporte · 2018-01-23T12:03:54Z

src/NHibernate/Loader/Criteria/CriteriaQueryTranslator.cs

+			{
+				last = entityJoinInfo.Persister;
+				lastEntity = (IPropertyMapping) last;
+				++i;


Just to add to this: if preferring ++i; other i++; for "performance", this is an old performance "trick", obsolete for C, potentially still relevant for C++ when applied to an object but well, we are using C#. See here. Optimizing such trivial things is not a developer job but a compiler job. I am on the side on optimizing the developer work by letting compiler responsible of such optimizations, and letting developer write most readable code possible. This includes using most common places syntaxes unless there is a real and significant impact for the application.

fredericDelaporte · 2018-01-23T14:17:29Z

src/NHibernate/Loader/OuterJoinableAssociation.cs

+			{
+				on = on.Count == 0
+					? on.Append(withClause)
+					: on.Append(" and ( ").Append(withClause).Append(" )");


For me, that is an undue change. It looks to me the way it is generated, on is either empty or starts with " and ". Testing on Count here changes behavior. Better not do that and keep previous code.

fredericDelaporte · 2018-01-23T14:20:17Z

src/NHibernate/SqlCommand/ANSIJoinFragment.cs

+			if (fkColumns.Length == 0)
+			{
+				_fromFragment.Add(on);
+				return;


Here, an additional adjustment should be done. on is usually generated with a leading " and ", which should be removed before adding to _fromFragment.

fredericDelaporte · 2018-01-23T14:20:48Z

src/NHibernate/SqlCommand/JoinFragment.cs

@@ -62,7 +62,7 @@ protected bool AddCondition(SqlStringBuilder buffer, SqlString on)
 		{
 			if (SqlStringHelper.IsNotEmpty(on))
 			{
-				if (!on.StartsWithCaseInsensitive(" and"))
+				if (buffer.Count > 0 && !on.StartsWithCaseInsensitive(" and"))


I doubt this change is legit. Was it causing a failure not having this change done? It looks to me code expects condition clauses to always potentially starts with an " and", removing it if needed.

The intention here is the same as in ANSIJoinFragment.AddJoin - avoid bare withClause to be added with " and" prefix to SQL. But yeah - maybe I should handle it in each dialect explicitly.

You are right - I've removed this change. I've struggled with this " and" prefix and thought this change is required for Oracle (but I was wrong)

…oin)

bahusoid · 2018-01-25T06:27:49Z

@fredericDelaporte It seems I've addressed all your requests.

I also checked how to make Fetch work with entity joins. And I think I found a bug. Interface for Fetch is defined as:

nhibernate-core/src/NHibernate/IQueryOver.cs

Line 369 in 8d2b123

    
           IQueryOverFetchBuilder<TRoot,TSubType> Fetch(Expression<Func<TRoot, object>> path);

Expression<Func<TRoot, object>> path - type for root criteria is provided (which is TRoot) in expression parameter . But in implementation fetch mode is applied to UnderlyingCriteria(which is TSubType based) instead of RootCriteria:

nhibernate-core/src/NHibernate/Criterion/Lambda/QueryOverFetchBuilder.cs

Line 44 in 8d2b123

this.root.UnderlyingCriteria.SetFetchMode(path, FetchMode.Eager);

So basically fetch doesn't work after JoinQueryOver call as property name from root entity is provided to current query over entity.

I think it's actually an "interface bug" - as this TRoot restriction is artificial and not really necessary. So it really should be:

IQueryOverFetchBuilder<TRoot,TSubType> Fetch(Expression<Func<**TSubType**, object>> path);

Any ideas how to resolve it properly? Maybe in 5.x we should made a more straightforward fix - use RootCriteria in fetch builder and in 6.x change interface to use Expression<Func<TSubType, object>> path.

fredericDelaporte · 2018-01-27T15:27:57Z

I also checked how to make Fetch work with entity joins. And I think I found a bug.

I think it will need to be addressed as another PR, that is why I approve this one as is.

Any ideas how to resolve it properly? Maybe in 5.x we should made a more straightforward fix - use RootCriteria in fetch builder and in 6.x change interface to use Expression<Func<TSubType, object>> path.

Well, as usual, for a minor, we cannot change anything in an interface. Does it work to define a suitable extension, that would handle only NHibernate QueryOver internal implementation, while obsoleting the interface method? That should be the preferred way, with some "6.0 TODO" for merging it back into the interface.

hazzik · 2018-01-27T20:46:17Z

It seems this PR has fixed another issue btw.

bahusoid · 2018-01-28T04:16:42Z

I've made last small adjustments:

Fixed exception message. Don't know how nameof(T) got there and why it's even allowed :)
Added one more extension to Criteria. Just to make things even for CreateEntityCriteria/CreateEntityAlias

fredericDelaporte · 2018-01-28T08:40:41Z

It seems this PR has fixed another issue btw.

Which is?

hazzik · 2018-01-28T10:22:16Z

I was not near the PC when wrote this. It should be something along NH-3506 (#1366) and NH-2029

fredericDelaporte · 2018-01-29T13:35:52Z

I have cherry-picked tests for #1366 (NH-3506) from #268, and tested them on this PR and on its base commit: no changes, fails the same way, with the same resulting queries (catched inside SQL-Profiler). So it does not look like this PR is fixing anything additional. (NH-2029 was already fixed in 5.0.)

fredericDelaporte · 2018-01-29T13:36:14Z

So I intend to merge this soon.

fredericDelaporte · 2018-03-17T11:00:39Z

There is something forgotten: documenting the new feature in the reference documentation. I think this new feature deserve a mention into criteria and queryover chapters.

bahusoid force-pushed the customJoin branch 2 times, most recently from aed4628 to aaaf9fc Compare January 21, 2018 04:58

fredericDelaporte added t: New Feature c: Core p: Minor c: QueryOver labels Jan 22, 2018

fredericDelaporte reviewed Jan 22, 2018

View reviewed changes

fredericDelaporte reviewed Jan 23, 2018

View reviewed changes

bahusoid added 4 commits January 24, 2018 20:52

Support to join not associated entities in Criteria API (aka Entity J…

a49c739

…oin)

Interface changes

fffd3c5

clean up

817d85b

JoinFragment: explicitly remove " and " clause

1f77db9

bahusoid force-pushed the customJoin branch from d3f8a63 to 53e12b9 Compare January 24, 2018 15:22

async regen

b75d6a5

bahusoid force-pushed the customJoin branch from 53e12b9 to b75d6a5 Compare January 24, 2018 15:28

bahusoid added 2 commits January 25, 2018 15:38

test cleanup

5e089e6

Simplified ISupportEntityJoinQueryOver interface

8ac4b7b

bahusoid force-pushed the customJoin branch from d88bc7d to 8ac4b7b Compare January 26, 2018 05:45

fredericDelaporte previously approved these changes Jan 27, 2018

View reviewed changes

fredericDelaporte self-assigned this Jan 27, 2018

fredericDelaporte added this to the 5.1 milestone Jan 27, 2018

Fixed exception message, added one more Criteria extension

3f6bb94

bahusoid dismissed fredericDelaporte’s stale review via 3f6bb94 January 28, 2018 04:12

fredericDelaporte removed their assignment Jan 28, 2018

fredericDelaporte self-assigned this Jan 29, 2018

fredericDelaporte merged commit 909fac2 into nhibernate:master Jan 30, 2018

fredericDelaporte added the r: Fixed label Jan 30, 2018

fredericDelaporte mentioned this pull request Mar 17, 2018

Ability to select entities in Criteria projections #1471

Merged

bahusoid mentioned this pull request Mar 18, 2018

Document "entity join" and "entity projection" #1619

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support to join not associated entities in Criteria (aka Entity Join) #1545

Support to join not associated entities in Criteria (aka Entity Join) #1545

bahusoid commented Jan 20, 2018 •

edited

Loading

fredericDelaporte left a comment

fredericDelaporte Jan 22, 2018

fredericDelaporte Jan 23, 2018

bahusoid Jan 23, 2018

fredericDelaporte Jan 22, 2018

fredericDelaporte Jan 22, 2018

fredericDelaporte Jan 22, 2018

fredericDelaporte Jan 22, 2018

fredericDelaporte Jan 23, 2018

bahusoid Jan 23, 2018

bahusoid Jan 23, 2018

fredericDelaporte Jan 23, 2018

fredericDelaporte Jan 23, 2018

fredericDelaporte Jan 23, 2018

fredericDelaporte Jan 23, 2018

fredericDelaporte Jan 23, 2018

fredericDelaporte Jan 23, 2018

fredericDelaporte Jan 23, 2018

fredericDelaporte Jan 23, 2018

fredericDelaporte Jan 23, 2018

fredericDelaporte Jan 23, 2018

bahusoid Jan 23, 2018

bahusoid Jan 25, 2018 •

edited

Loading

bahusoid commented Jan 25, 2018 •

edited

Loading

fredericDelaporte commented Jan 27, 2018

hazzik commented Jan 27, 2018

bahusoid commented Jan 28, 2018

fredericDelaporte commented Jan 28, 2018

hazzik commented Jan 28, 2018 •

edited by fredericDelaporte

Loading

fredericDelaporte commented Jan 29, 2018 •

edited

Loading

fredericDelaporte commented Jan 29, 2018

fredericDelaporte commented Mar 17, 2018

Support to join not associated entities in Criteria (aka Entity Join) #1545

Support to join not associated entities in Criteria (aka Entity Join) #1545

Conversation

bahusoid commented Jan 20, 2018 • edited Loading

fredericDelaporte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bahusoid Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

bahusoid commented Jan 25, 2018 • edited Loading

fredericDelaporte commented Jan 27, 2018

hazzik commented Jan 27, 2018

bahusoid commented Jan 28, 2018

fredericDelaporte commented Jan 28, 2018

hazzik commented Jan 28, 2018 • edited by fredericDelaporte Loading

fredericDelaporte commented Jan 29, 2018 • edited Loading

fredericDelaporte commented Jan 29, 2018

fredericDelaporte commented Mar 17, 2018

bahusoid commented Jan 20, 2018 •

edited

Loading

bahusoid Jan 25, 2018 •

edited

Loading

bahusoid commented Jan 25, 2018 •

edited

Loading

hazzik commented Jan 28, 2018 •

edited by fredericDelaporte

Loading

fredericDelaporte commented Jan 29, 2018 •

edited

Loading