Full control of entities fetching in Criteria #1599

bahusoid · 2018-03-08T19:15:37Z

Old behavior: ManyToOne entities joined via JoinAlias/JoinQueryOver/Join... are always fetched.
OneToMany collections are only fetched when they are joined via LeftOuterJoin, but element entities are fetched anyway.
And you have no control over it.

With this PR:
You can explicitly control fetching behavior via setting SelectMode enum for criteria association paths. So with:
1) SelectMode.Fetch you can fetch current criteria/alias association path (if persister supports it). So now it's possible to fetch collections with inner or whatever join you want:

var list = session.QueryOver<EntityComplex>()
		.Fetch(SelectMode.Fetch, ec => ec.Child1)
		.JoinQueryOver(ec => ec.ChildrenList, JoinType.InnerJoin)
		//now we can fetch inner joined collection
		.Fetch(SelectMode.Fetch, childrenList => childrenList)
		.TransformUsing(Transformers.DistinctRootEntity)
		.List();

link to test

2) SelectMode.JoinOnly you can skip current criteria/alias association path to be fetched (so it won't be included in SELECT statement):

root = session.QueryOver(() => root)
		//Child1 is required solely for filtering, no need to be fetched, so skip it from select statement
		.JoinQueryOver(r => r.Child1, JoinType.InnerJoin)
		.Fetch(SelectMode.JoinOnly, child1 => child1)
		.Take(1)
		.SingleOrDefault();

link to test
Note: JoinOnly for root entity is not implemented (It's not trivial task and it's too much additional code change noise, so maybe later with another PR). But you can use SelectMode.ChildFetch instead (see below).

3) SelectMode.FetchLazyProperties you can fetch entities with lazy properties initialized:

var root = session.QueryOver<EntityComplex>()
			.Fetch(SelectMode.FetchLazyProperties, ec => ec)
			.Where(ec => ec.LazyProp != null)
			.Take(1)
			.SingleOrDefault();

link to test

4) SelectMode.Undefined - Basically no-op for queries:

//SelectMode.Undefined is no op - fetching is controlled by mapping:
//SameTypeChild won't be loaded, and ChildrenList collection won't be fetched due to InnerJoin
var list = session.QueryOver<EntityComplex>()
		.Fetch(SelectMode.Undefined, ec => ec.SameTypeChild)
		.JoinQueryOver(ec => ec.ChildrenList, JoinType.InnerJoin)
		.Fetch(SelectMode.Undefined, childrenList => childrenList)
		.TransformUsing(Transformers.DistinctRootEntity)
		.List();

//So it's a full equivalent of the following query (without SelectMode)
session.QueryOver<EntityComplex>()
		.JoinQueryOver(ec => ec.ChildrenList, JoinType.InnerJoin)
		.TransformUsing(Transformers.DistinctRootEntity);

link to test

5) SelectMode.ChildFetch - Experimental feature. The main idea is to allow effective multiple collections fetching in one database roundtrip. In this mode only entity Identifier columns are added to SELECT statement. Retrieved identifier is used only to obtain object from session and use it further for child entities fetching. If object is not found in session it's returned as null. Example of usage:

EntityComplex root = null;
var futureRoot = session.QueryOver(() => root)
			.Where(r => r.Id == _parentEntityComplexId)
			.FutureValue();
session
	.QueryOver(() => root)
	//Only ID is added to SELECT statement for root so it's index scan only
	.Fetch(SelectMode.ChildFetch, ec => ec)
	.Fetch(SelectMode.Fetch, ec => ec.ChildrenList)
	.Where(r => r.Id == _parentEntityComplexId)
	.Future();
session
	.QueryOver(() => root)
	.Fetch(SelectMode.ChildFetch, ec => ec)
	.Fetch(SelectMode.Fetch, ec => ec.ChildrenListEmpty)
	.Where(r => r.Id == _parentEntityComplexId)
	.Future();
root = futureRoot.Value;

link to test
Combining JoinOnly, ChildFetch and Fetch allows to do some crazy multilevel collections fetching in one database roundtrip:

EntityComplex root = null;
var rootFuture = session
			.QueryOver(() => root)
			.Future();

session
	.QueryOver(() => root)
	.Fetch(SelectMode.ChildFetch, () => root)
	.Fetch(SelectMode.Fetch, () => root.ChildrenList)
	.Future();
session
	.QueryOver(() => root)
	.Fetch(SelectMode.ChildFetch, () => root, () => root.ChildrenList)
	.Fetch(SelectMode.Fetch, () => root.ChildrenList[0].Children)
	.Future();
session
	.QueryOver(() => root)
	.Fetch(SelectMode.JoinOnly, () => root.ChildrenList)
	.Fetch(SelectMode.ChildFetch,() => root, () => root.ChildrenList[0].Children)
	.Fetch(SelectMode.Fetch, () => root.ChildrenList[0].Children[0].Children)
	.Future();
root = rootFuture.ToList().First(r => r.Id == _parentEntityComplexId);

link to test
So here root.ChildrenList.Children.Children - 3 level deep collections are fetched in 1 db roundtrip.

6) `SelectMode.Skip'- skips join (and so fetching) from query. Useful only to disable eager mapping association.

Functionality is exposed via Fetch extension methods on IQueryOver interface taking current criteria paths or aliased criteria paths as parameters:

queryOver
.Fetch(SelectMode.Fetch, curEntity => curEntity, curEntity => curEntity.ChildProp1, curEntity => curEntity.ChildProp2, ... )
.Fetch(SelectMode.JoinOnly, () => criteriaAlias, () => criteriaAlias.ChildProp1, () => criteriaAlis2.ChildProp1, ...)
.Fetch(SelectMode.FetchLazyProperties, ...)

hazzik · 2018-03-11T09:20:53Z

src/NHibernate/ISupportSelectModeCriteria.cs

+		/// <param name="selectMode"></param>
+		/// <param name="associationPath">Criteria association path. If empty root entity for given criteria is used.</param>
+		/// <param name="alias">Criteria alias. If empty current <see cref="ICriteria"/> criteria is used.</param>
+		void SetSelectMode(SelectMode selectMode, string associationPath, string alias);


Shouldn't it return ICriteria?

Yeah. It should. Fixed.

fredericDelaporte · 2018-03-17T08:31:20Z

The point 3 is NH-2888 (#972), so its opened PR (#264) would be replaced by this one.

fredericDelaporte · 2018-03-17T11:06:04Z

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.

(Ok you have been notified of this already twice on two other already merged PR. If updating the doc here, do not do it for the other PR at the same time, because the other PRs should be documented in a 5.1.1 PR if possible, while this PR here would be merged in 5.2.)

hazzik · 2018-03-22T21:38:06Z

Why the decision has been made to put this as a separate method? For me it's counterintuitive. Could it be just an additional parameter on existing methods (JoinQueryOver, etc.)?

bahusoid · 2018-03-23T04:13:35Z

Because functionality is not bounded to (Join...) or any other existing methods. SelectMode for instance can be applied for root query (or any other current criteria):

//fetch lazy properties for root and for root.ChildEntity
session.QueryOver<Entity>()
.With(SelectMode.FetchLazyProperties, 
   e => e,
   e => e.ChildEntity)...

//fetch lazy properties for current criteria  only - ChildEntity
session.QueryOver<Entity>()
.JoinQueryOver(e => e.ChildEntity)
.With(SelectMode.FetchLazyProperties, childEntity => childEntity)

But it also can be applied only to particular list of aliases (so select mode can be set in the end after query is written).

session.QueryOver<Entity>(()=>root)
.JoinQueryOver(e => e.ChildEntity)
.With(SelectMode.FetchLazyProperties, () => root)...

It's more like existing Fetch method but with alias support and more control. May be instead of With it can be exposed as Fetch override?

hazzik · 2018-05-14T12:01:19Z

Approved in principle. However I'm still not convinced with the method name: I think that this feature would be hard to discover.

It's more like existing Fetch method but with alias support and more control. May be instead of With it can be exposed as Fetch override?

Can you show some examples of what you mean?

bahusoid · 2018-05-15T06:08:23Z

I think that this feature would be hard to discover.

If you really need it - you will find it :) But if you have in mind a better name - just let me know.

@hazzik
Not sure which part of quoted text should I explain. But I meant that SelectMode.Fetch can fully replace existing Fetch method and adds additional functionality.
Code below

query
.Fetch(p => p.Child1).Eager
.Fetch(p => p.Child2).Eager

can be rewritten as

query
.With(SelectMode.Fetch, p => p.Child1, p => p.Child2)

Plus it supports fetching by aliases (not supported by Fetch and criteria SetFetchMode methods);

NotAssociatedEntity notAssociatedAlias = null;
ChildEntity aliasForList = null;
query.JoinEntityAlias(() => notAssociatedAlias, ... )
.JoinAlias(p => p.ChildrenList, () => childList);
.With(SelectMode.Fetch, () => notAssociatedAlias.Child, () => childList)

So SelecMode currently is the only option for fetching inner joined collections and entity joins children. Plus all other SelectMode options explained in pull request.

And instead of With as an option I suggested to name it Fetch. So after rename it would be:

query
.Fetch(SelectMode.Fetch, p => p.Child1, p => p.Child2)

and

query.JoinEntityAlias(() => notAssociatedAlias, ... )
.JoinAlias(p => p.ChildrenList, () => childList);
.Fetch(SelectMode.Fetch, () => notAssociatedAlias.Child, () => childList)

But if you agree Fetch is a better name I need to check it doesn't break anything (like compilation issues due to ambiguous method resolution due to existing Fetch methods)

bahusoid · 2018-05-15T09:29:35Z

rebased + fixed CriteriaImpl.Clone

fredericDelaporte · 2018-05-15T11:32:54Z

src/NHibernate/Loader/SelectMode.cs

+		/// Use it for fetching child objects for already loaded entities.
+		/// Note: Missing entities in session will be returned as null
+		/// </summary>
+		ChildFetch,


Note: Missing entities in session will be returned as null

What does that mean? That using this mode could result in persisted entities to be returned as null if they were not already loaded in the session? If yes, this is a very unusual behavior for NHibernate. It should yield a proxy instead. (Or throw an error, or trigger a load, whatever, but guarantee no inconsistent data is returned.)

If yes, this is a very unusual behavior for NHibernate

You should be ready for null if you are doing right join for instance. This option is useful strictly for "fetch only" queries - you should not really use result of query with SelectMode.ChildFetch.
You must use result of parent query instead. So I don't think of it as inconsistency - it's just the implementation details: you just can't load parent for one ID and try to fetch child for another ID and expect it to work. So I think nulls are quite appropriate results for the following case:

var parentResultsToUse = s.QueryOver<Parent>.Where(p => p.Id =1).List(); // Correct usage - just load query to initialize children; ignore returning results s.QueryOver<Parent>() .With(SelectMode.ChildFetch, p => p) .With(SelectMode.Fetch, p => p.Child1, p => p.Child2) .Where(p => p.Id = 1) .List(); //Incorrect Usage - child fetch for different ID; results are saved for further usage var resultWithNulls = s.QueryOver<Parent>() .With(SelectMode.ChildFetch, p => p) .With(SelectMode.Fetch, p => p.Child1, p => p.Child2) .Where(p => p.Id = 2) .List();

You should be ready for null if you are doing right join for instance.

Yes, but that does not look to me as a right join case here.

There are not any cases in NHibernate where a query result correctness depends on having already loaded these entities in the session. Yielding null in the query results instead of an entity (or proxy of an entity) because this entity was not already loaded in the session is not acceptable in my opinion.

We are talking about Loader logic. We are processing an open data reader with only ID columns present for ChildFetch entities. It's not the right place to load any entities here. And returning a proxy is also a bad idea - object might not be proxified. To me null here is just logical and easiest solution - results of those queries should not be processed as is anyway.
I would agree with you if I've changed existing behavior. But that's completely new feature - why can't it have specific documented behavior. Those nulls are not going anywhere in session except resulting list returned by query execution.

I don't see how it's any different with current collections fetches - by default root entities are duplicated in results. Yep it's not nulls but you can't just process results as is - you need to know this behavior and use appropriate result transformer.

You suggestion will considerable complicate Loader code with no added benefits (I'm not even sure how to proceed - you can check how Loader.IsChildFetchEntity is used now).

It adds a notable benefit: it avoids a case of query yielding incorrect results. I maintain my point: adding a query case which can yield null instead of the expected entity does not seem acceptable. This can only end up reported as a bug.

I do not think adding a feature with this kind of special semantic is a good move.

returning a proxy is also a bad idea - object might not be proxified

A load that does not actually load the returned entities calls for proxy availability. The feature should just throw an exception when used on entities for which proxies are disabled. (Or default to fully load the entity in a dedicated query, as the Loader is already capable of, like illustrated by this case.)

The collection fetching case is a legacy users have to cope with. (Due to this kind of troubles with NHibernate fetching, my personal recommendation with NHibernate is to use batching of lazy loads instead. It solves the n+1 loads trouble while keeping things simple, without needs to split queries or de-duplicate results or give up paging, ...)

You suggestion will considerable complicate Loader code with no added benefits (I'm not even sure how to proceed - you can check how Loader.IsChildFetchEntity is used now).

This case demonstrates the Loader is already able of loading non proxified entities when required with an additional dedicated query. Of course this may then end up with potentially a n+1 loads when a user somewhat incorrectly uses this ChildFetch feature on non-proxifiable entities which are not already loaded in the session. But that looks to me still better than yielding null instead of the queried entities. As of whether this capability is usable in the case of the ChildFetch or not, I have not yet studied it.

It appears to me that yielding null will moreover wreck child collection loading, defeating the purpose of the feature. See Loader.ReadCollectionElement, which is called on the row array. It may end up calling EntityType.ResolveIdentifier which has an early exit bypassing instantiating the element if the owner is null.

fredericDelaporte · 2018-05-16T16:06:29Z

src/NHibernate/Persister/Collection/AbstractCollectionPersister.cs

@@ -1646,8 +1646,27 @@ public object NotFoundObject

 		public abstract SqlString WhereJoinFragment(string alias, bool innerJoin, bool includeSubclasses);

+		// 6.0 TODO: Remove (Replace with ISupportSelectModeJoinable.SelectFragment)
 		public abstract string SelectFragment(IJoinable rhs, string rhsAlias, string lhsAlias, string currentEntitySuffix,


Things that are to be removed have to be obsoleted. This eases the migration path.

fredericDelaporte · 2018-05-16T16:47:45Z

src/NHibernate/Type/TypeHelper.cs

@@ -14,6 +14,12 @@ public static partial class TypeHelper
 	{
 		public static readonly IType[] EmptyTypeArray = Array.Empty<IType>();

+		internal static T CastOrThrow<T>(object obj, string supportMessage) where T : class


TypeHelper is about NHibernate types (IType). Better put this in ReflectHelper.

fredericDelaporte · 2018-05-17T00:10:36Z

src/NHibernate/Loader/SelectMode.cs

+		/// Use it for fetching child objects for already loaded entities.
+		/// Note: Missing entities in session will be returned as null
+		/// </summary>
+		ChildFetch,


It appears to me that yielding null will moreover wreck child collection loading, defeating the purpose of the feature. See Loader.ReadCollectionElement, which is called on the row array. It may end up calling EntityType.ResolveIdentifier which has an early exit bypassing instantiating the element if the owner is null.

fredericDelaporte

About the naming, I am for changing With to Fetch, but also obsolete the previous Fetch, since the new one covers all its use cases.

hazzik · 2018-05-17T00:47:58Z

About the naming, I am for changing With to Fetch, but also obsolete the previous Fetch, since the new one cover all its use cases.

💯% agree.

I would also request to change parameter positions:

Fetch(path, SelectMode)

If this is possible

bahusoid · 2018-05-17T08:27:10Z

@fredericDelaporte Wow. Thanks for you changes. If it works it would really be better approach. For some reasons I thought that another entity loading in loader will lead to multiple open data readers that might not be supported by database. I've added test to ensure that this scenario works.

fredericDelaporte · 2018-05-17T08:33:47Z

For some reasons I thought that another entity loading in loader will lead to multiple open data readers that might not be supported by database.

Indeed, but this is already handled by NHibernate. See IDriver.SupportsMultipleOpenReaders and the NDataReader/NHybridDataReader which allow to work around this limitation.

bahusoid · 2018-05-17T08:42:10Z

Regarding
Fetch(path, SelectMode)
Yeah with name Fetch path is more logical as first param. But I think it's convenient to have path as params parameter - so you don't need to write repetitive code with many Fetch calls.

How about FetchWith(SelectMode, path1, path2,...)?

fredericDelaporte · 2018-05-17T08:39:13Z

src/NHibernate/Loader/Loader.cs

+						if (!alreadyLoaded && mustLoadMissingEntity)
+						{
+							// Missing in session while its data has not been selected: fallback on immediate load
+							obj = session.ImmediateLoad(key.EntityName, key.Identifier);


I have first attempted to do an InternalLoad here instead, without accounting for returnProxies/mustLoadMissingEntity, and without changing anything to GetRowcaller. But this was (as expected) potentially yielding a proxy, which GetRow caller was not supporting: it was forcing the proxy initialization with itself as implementation, resulting in stack overflows when attempting to access the entity state.

For this reason, when proxy are supported, better yield null here and let GetRow caller handle returning a proxy.

fredericDelaporte · 2018-05-17T08:48:57Z

src/NHibernate/Loader/Loader.cs

+							obj = session.ImmediateLoad(key.EntityName, key.Identifier);
+						}
+						rowResults[i] = obj;
+						continue;


Now completely skipping InstanceAlreadyLoaded and CacheByUniqueKey for child-fetch cases.
InstanceAlreadyLoaded handle the locking: should we lock a thing for which we only have selected its identifier, and which the query was designed to not fetch?
Likewise, handling caching for an entity which the query is not meant to actually load should not be done.

fredericDelaporte · 2018-05-17T08:54:45Z

src/NHibernate/Loader/Loader.cs

+						if (entity != proxy)
+						{
+							// Force the proxy to resolve itself
+							((INHibernateProxy) proxy).HibernateLazyInitializer.SetImplementation(entity);


I was tempted to remove that line, because ProxyFor implementation already handles it. But well, that is not the subject of this PR.

fredericDelaporte · 2018-05-17T09:04:05Z

I am for keeping the params and so for keeping the current ordering of Fetch parameters. Renaming it to FetchWith sounds not valuable to me: more wordy for little semantic gain.

bahusoid · 2018-05-17T10:35:44Z

me: I meant that SelectMode.Fetch can fully replace existing Fetch method
Frederic: but also obsolete the previous Fetch, since the new one covers all its use cases.

Actually I've realized SelectMode covers only eager fetching cases. The following case is not supported by SelectMode (just because I didn't think of this case):
queryOver.Fetch(p => p.EagerList).Lazy;

And I'm not sure that we should drop/obsolete existing Fetch method. It operates with FetchMode - enum that if I'm not mistaken is used in mappings for defining default "fetching strategy" and it's not obsolete. So it's good to have method that allows to override it (as it might be also extended somehow in the future).

So

Are you sure that existing Fetch should be obsoleted?
Should I add SelectMode.Lazy as equivalent of FetchMode.Lazy?

fredericDelaporte · 2018-05-17T13:25:49Z

If 2 is done, what will be missing?
Yes.

It operates with FetchMode - enum that if I'm not mistaken is used in mappings for defining default "fetching strategy" and it's not obsolete.

Yes but that is, in the QueryOver case, an internal implementation detail already quite hidden away from user. And the select mode does also operate the FetchMode almost the same way, isn't it? Keeping the two looks to me like we could ask for contradictory things, with the old Fetch currently having priority over the new (CriteriaImp.GetFetchMode code by example). This could lead to unexpected results for the user.

In the criteria case, I think SetFetchMode should be likewise obsoleted, especially since it exposes the whole FetchMode enumeration while only Default, Join and Lazy are actually supported, ~~the other values~~ Select being handled as Lazy and Eager as Join. (See CriteriaJoinWalker.GetJoinType, along with FetchMode definition.)

fredericDelaporte · 2018-05-17T13:18:10Z

src/NHibernate/Impl/CriteriaImpl.cs

@@ -15,12 +16,13 @@ namespace NHibernate.Impl
 	/// Implementation of the <see cref="ICriteria"/> interface
 	/// </summary>
 	[Serializable]
-	public partial class CriteriaImpl : ICriteria, ISupportEntityJoinCriteria
+	public partial class CriteriaImpl : ICriteria, ISupportEntityJoinCriteria, ISupportSelectModeCriteria
 	{
 		private readonly System.Type persistentClass;
 		private readonly List<CriterionEntry> criteria = new List<CriterionEntry>();
 		private readonly List<OrderEntry> orderEntries = new List<OrderEntry>(10);
 		private readonly Dictionary<string, FetchMode> fetchModes = new Dictionary<string, FetchMode>();


This should likely be removed, in favor of storing only select modes, with the obsoleted Fetch being translated to the new one for keeping it working.

fredericDelaporte · 2018-05-17T13:20:33Z

src/NHibernate/ISupportSelectModeCriteria.cs

+		/// <param name="associationPath">The criteria association path. If empty, the root entity for the given
+		/// criteria is used.</param>
+		/// <param name="alias">The criteria alias. If empty, the current <see cref="ICriteria"/> criteria is used.</param>
+		ICriteria SetSelectMode(SelectMode selectMode, string associationPath, string alias);


Either rename to Fetch too or rename the ICriteria extension to SetSelectMode. Otherwise, once merged, the ICriteria API would expose two ways of doing exactly the same thing, which would be confusing for the user.

bahusoid · 2018-05-21T20:09:28Z

So it seems I've adressed all requested changes.

Some notes:
I've named FetchMode.Lazy and FetchMode.Select equivalent as SelectMode.SkipJoin.
I found that Lazy name is confusing. For instance for the following query:

s.QueryOver<Entity>().JoinQueryOver(e => e.Child).Fetch(SelectMode.Lazy, child => child)

I would expect inner join present in query - I just don't want it to be eagerly fetched. But in fact the way FetchMode.Lazy is implemented - it simply removes join from query. So JoinQueryOver logic is completely lost.
With SkipJoin name I think it's more clear.

I've added shortcuts for SelectMode.Fetch

criteria.Fetch("Child");

queryOver.Fetch(c => c.Child);

But QueryOver version is hidden by obsolete Fetch method in 5.2. So it will be exposed only in 6.0 with deletion of obsolete method.
And now shortcut version is available only for multiple properties:

queryOver.Fetch(c => c.Child, c => c.Child2 );

Do additional minor cleanup and comments adjustments Update documentation

fredericDelaporte

With SkipJoin name I think it's more clear.

So SelectMode.SkipJoin is for completely ignoring the relation (not selected, not joined) while SelectMode.Skip is for only not selecting it (but still joined, for filtering).

Maybe both should be renamed, like SelectMode.JoinOnly instead of Skip, then SkipJoin could be renamed just Skip.

I've added shortcuts for SelectMode.Fetch

Along with SelectMode.Default current description:

Default behavior (as if no select mode is specified).

This is a bit confusing, since the shortcut resolves to SelectMode.Fetch. The description of Default should be changed to the one used for FetchMode.Default.

The documentation also needs some updates.

fredericDelaporte · 2018-05-22T11:13:03Z

src/NHibernate.Test/Criteria/SelectModeTest/SelectModeTest.cs

+
+		[Test]
+		public void LazyRootEntityIsNotSupported()
+		{using (var sqlLog = new SqlLogSpy())


This looks like a forgotten line of code.

fredericDelaporte · 2018-05-22T11:15:15Z

src/NHibernate.Test/Criteria/SelectModeTest/SelectModeTest.cs

 				Assert.Throws<NotSupportedException>(() => query.SingleOrDefault());
 			}
 		}

+//6.0 TODO: Remove tests wrapped in pragma below
+#pragma warning disable 618


Just mark tests for obsolete things as obsolete themselves.

fredericDelaporte · 2018-05-22T11:41:51Z

src/NHibernate/Criterion/QueryOver.cs

@@ -482,10 +482,12 @@ protected internal QueryOver(CriteriaImpl rootImpl, ICriteria criteria)
 			get { return new QueryOverSubqueryBuilder<TRoot,TSubType>(this); }
 		}

+#pragma warning disable CS0612 // Type or member is obsolete


Mark as obsolete instead.

fredericDelaporte · 2018-05-22T11:46:35Z

src/NHibernate/Criterion/QueryOver.cs

 		IQueryOverFetchBuilder<TRoot,TSubType> IQueryOver<TRoot,TSubType>.Fetch(Expression<Func<TRoot, object>> path)
 		{ return new IQueryOverFetchBuilder<TRoot,TSubType>(this, path); }
+#pragma warning restore CS0612 // Type or member is obsolete


Mark as obsolete instead.

fredericDelaporte · 2018-05-22T11:50:34Z

src/NHibernate/Criterion/QueryOverBuilderExtensions.cs

@@ -7,6 +7,8 @@ namespace NHibernate.Criterion
 {
 	public static class QueryOverBuilderExtensions
 	{
+#pragma warning disable CS0612 // Type or member is obsoletes
+


Mark them obsolete instead.

fredericDelaporte · 2018-05-22T11:59:59Z

src/NHibernate/FetchMode.cs

@@ -33,4 +32,4 @@ public enum FetchMode
 		Lazy = Select,
 		Eager = Join


Those two values were making sens only for their usage in criteria fetches. They should be obsoleted too.

fredericDelaporte · 2018-05-22T12:02:30Z

src/NHibernate/Impl/CriteriaImpl.cs

@@ -379,10 +377,28 @@ public ICriteria AddOrder(Order ordering)

 		public ICriteria SetFetchMode(string associationPath, FetchMode mode)


This should be obsoleted too.

bahusoid · 2018-05-22T14:54:05Z

Maybe both should be renamed, like SelectMode.JoinOnly instead of Skip, then SkipJoin could be renamed just Skip.

But this SkipJoin mode doesn't look useful to me. Scenario when you have eager mappings and do "lazy" queries - doesn't look like common use case. Or am I wrong?

So I would prefer to have shorter name for more useful mode (so keep it as Skip). Do you think that "SelectMode.Skip" name is confusing?

This is a bit confusing, since the shortcut resolves to SelectMode.Fetch

Maybe I should rename SelectMode.Default to SelectMode.Undefined? As indeed Default might be confusing.

fredericDelaporte · 2018-05-22T15:56:05Z

Do you think that "SelectMode.Skip" name is confusing?

For skipping only the select part and keeping the join, I find that name somewhat confusing. It can be understood that the skip relates to the select clause only due to the name of the enumeration, even if that is not very straightforward. But along with having a SkipJoin, the purpose of each is then not very intuitive.

Maybe I should rename SelectMode.Default to SelectMode.Undefined? As indeed Default might be confusing.

I have already rewritten the description, but finding a better name is a good idea. What about AsMapped?

So a choice has to be made between:

Purpose	Naming 1	Naming 2
Keep mapping behavior	SelectMode.Undefined	SelectMode.AsMapped
Fetch the entity/collection	SelectMode.Fetch	SelectMode.Fetch
Fetch with lazy properties	SelectMode.FetchLazyProperties	SelectMode.FetchLazyProperties
Do not fetch, fetch children instead	SelectMode.ChildFetch	SelectMode.ChildFetch
Do not fetch, just join	SelectMode.Skip	SelectMode.JoinOnly
Do neither fetch nor join	SelectMode.SkipJoin	SelectMode.Skip

Looking at the list causes me to wonder, couldn't "Do not fetch, fetch children instead" and "Do not fetch, just join" be combined in a single SelectMode? But that may imply selecting ids in some cases not needing them.

bahusoid · 2018-05-22T17:46:29Z

Looking at the list causes me to wonder, couldn't "Do not fetch, fetch children instead" and "Do not fetch, just join" be combined in a single SelectMode?

But why? From implementation standpoint "Do not fetch, just join" is simple change. Why to merge it with much more complex scenario? And you can't skip that way non-proxified objects:

//collection elements won't be loaded to session
s.QueryOver<Parent>().JoinQueryOver(p => p.ChildListNonProxified).Fetch(SelectMode.Skip, l => l);

//collection elements will be loaded to session with separate query for each object
s.QueryOver<Parent>().JoinQueryOver(p => p.ChildListNonProxified).Fetch(SelectMode.ChildFetch, l => l);

Regarding JoinOnly and Skip - OK. I will rename enums. It might indeed avoid possible confusions.

Regarding AsMapped. My only concern is multilevel fetch:

s.QueryOver<Parent>().Fetch( SelectMode.AsMapped, p => p.LazyChild, p => p.LazyChild.EagerSubChild);

Do you think it's clear that both LazyChild and EagerSubChild won't be fetched. IMHO Undefined in this context is better choice. As AsMapped might be confused with the following expectations:

s.QueryOver<Parent>()
.Fetch( SelectMode.ChildFetch, p => p.LazyChild)
.Fetch( SelectMode.Fetch, p => p.LazyChild.EagerSubChild);

fredericDelaporte · 2018-05-22T18:26:31Z

About merging the two cases that are looking similar, that was just a question. If that creates to much complexity, forget about it.

Ok for Undefined.

Klausrex · 2019-10-08T11:24:34Z

The new error message in IQueryOver.Fetch "Use Fetch(SelectMode mode, Expression<Func<TSubType, object>> path) instead" does not help a lot. A Fetch method with two parameters is not part of the interface IQueryOver so it could not been called.
Is this method just missing in the interface?

bahusoid · 2019-10-08T11:33:52Z

It's not part of interface to avoid breaking changes (will be in 6.0) - it's an extension method (in namespace NHibernate). See here

Klausrex · 2019-10-08T12:17:45Z

Ah thanks a lot!

lcetinapta · 2020-02-12T10:11:01Z

Does this work on Lazy properties too? Because for mapping like this:
<property name="file_stream" type="BinaryBlob" length="2147483647" lazy="true" />
and query like this:
_session.QueryOver<StorageItem>().Fetch(SelectMode.FetchLazyProperties, x => x.file_stream).ListAsync()
It does not select the lazy property.

bahusoid · 2020-02-12T10:13:23Z

SelectMode.FetchLazyProperties should be applied to entity and not property
_session.QueryOver<StorageItem>().Fetch(SelectMode.FetchLazyProperties, x => x)

Individual lazy property fetch implemented in another PR: #2097 (not yet commited)

lcetinapta · 2020-02-12T10:16:57Z

SelectMode.FetchLazyProperties should be applied to entity and not property
_session.QueryOver<StorageItem>().Fetch(SelectMode.FetchLazyProperties, x => x)

I see, that worked. Thanks.

hazzik changed the title ~~Full control of entities fetching in Criteria~~ WIP Full control of entities fetching in Criteria Mar 9, 2018

bahusoid force-pushed the selectMode branch from 9db2e3e to 2fb2387 Compare March 9, 2018 19:33

bahusoid changed the title ~~WIP Full control of entities fetching in Criteria~~ Full control of entities fetching in Criteria Mar 11, 2018

hazzik reviewed Mar 11, 2018

View reviewed changes

bahusoid force-pushed the selectMode branch from 2fb2387 to c5f75af Compare March 11, 2018 16:38

fredericDelaporte added this to the 5.2 milestone Mar 26, 2018

hazzik previously approved these changes May 14, 2018

View reviewed changes

bahusoid dismissed hazzik’s stale review via 0fb5623 May 15, 2018 09:28

bahusoid force-pushed the selectMode branch from c5f75af to 0fb5623 Compare May 15, 2018 09:28

fredericDelaporte reviewed May 15, 2018

View reviewed changes

fredericDelaporte reviewed May 17, 2018

View reviewed changes

bahusoid added 2 commits May 19, 2018 16:40

Full control of entities fetching in Criteria

ef0e6cf

ISupportSelectModeCriteria.SetSelectMode should return ICriteria

73c1983

bahusoid changed the title ~~WIP Full control of entities fetching in Criteria~~ Full control of entities fetching in Criteria May 21, 2018

Obsolete more previous fetches members

c096716

Do additional minor cleanup and comments adjustments Update documentation

fredericDelaporte reviewed May 22, 2018

View reviewed changes

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

Renamed enums

2184f5a

fredericDelaporte approved these changes May 22, 2018

View reviewed changes

fredericDelaporte merged commit ee91396 into nhibernate:master May 23, 2018

fredericDelaporte added the r: Fixed label May 23, 2018

This was referenced May 23, 2018

NH-2888 - Add support to ICriteria to eager fetch lazy properties #264

Closed

NH-2888 - Add 'fetch all properties' support to ICriteria #972

Closed

fredericDelaporte mentioned this pull request Jun 23, 2018

Make obsolete abstract virtual #1770

Merged

fredericDelaporte mentioned this pull request Oct 12, 2018

Lazy loading and Eager initialization for Component #1861

Closed

bahusoid mentioned this pull request Mar 1, 2019

NH-3536 - LEFT JOIN makes a lazy collections IsInitialized, INNER JOIN doesn't #1030

Closed

bahusoid mentioned this pull request Apr 30, 2019

NH-3463 - LEFT OUTER JOIN subcriteria filters children #1155

Closed

jrauber mentioned this pull request Jun 13, 2019

New Fetch() method in QueryOver returns IQueryOver<> instead of QueryOver<> #2177

Closed

bahusoid mentioned this pull request Jan 2, 2020

How to eager load deeply nested entities efficiently #2297

Closed

		@@ -33,4 +32,4 @@ public enum FetchMode
		Lazy = Select,
		Eager = Join

		@@ -379,10 +377,28 @@ public ICriteria AddOrder(Order ordering)

		public ICriteria SetFetchMode(string associationPath, FetchMode mode)

Full control of entities fetching in Criteria #1599

Full control of entities fetching in Criteria #1599

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Mar 17, 2018

Uh oh!

hazzik commented Mar 22, 2018

Uh oh!

bahusoid commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hazzik commented May 14, 2018

Uh oh!

bahusoid commented May 15, 2018

Uh oh!

bahusoid commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bahusoid May 15, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

bahusoid May 15, 2018 • 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 May 15, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte 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!

hazzik commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bahusoid commented May 17, 2018

Uh oh!

fredericDelaporte commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bahusoid commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented May 17, 2018

Uh oh!

bahusoid commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bahusoid commented Mar 8, 2018 •

edited

Loading

fredericDelaporte commented Mar 17, 2018 •

edited

Loading

bahusoid commented Mar 23, 2018 •

edited

Loading

bahusoid May 15, 2018 •

edited

Loading

bahusoid May 15, 2018 •

edited

Loading

fredericDelaporte May 15, 2018 •

edited

Loading

fredericDelaporte left a comment •

edited

Loading

hazzik commented May 17, 2018 •

edited

Loading

fredericDelaporte commented May 17, 2018 •

edited

Loading

bahusoid commented May 17, 2018 •

edited

Loading

bahusoid commented May 17, 2018 •

edited

Loading

fredericDelaporte commented May 17, 2018 •

edited

Loading

bahusoid commented May 21, 2018 •

edited

Loading

bahusoid commented May 22, 2018 •

edited

Loading

bahusoid commented May 22, 2018 •

edited

Loading

bahusoid commented Feb 12, 2020 •

edited

Loading