Ability to select entities in Criteria projections #1471

bahusoid · 2017-12-03T16:56:32Z

Another attempt to implement #948 (initially I re-based #302 in #1458 but it turns out only lazy entities are supported by this PR).

So with new implementatione entities are initialized by default (without lazy properties). Can be configured in fluent syntax:

Projections.Entity(()=> entityAlias)
.SetLazy(lazy)
.SetFetchLazyProperties(lazyProps)

See other examples in tests.

fredericDelaporte · 2017-12-04T11:19:54Z

Compilation warnings are turned into errors in the release build. The test has introduced some warnings, they need to be fixed.

fredericDelaporte · 2017-12-04T11:24:15Z

src/NHibernate.Test/NHSpecificTest/GH948/FixtureByCode.cs

+		public void RootEntityProjectionFullyInitializedAndWithUnfetchedLazyPropertiesByDefault()
+		{
+			using(var sqlLog = new SqlLogSpy())
+			using (var session = OpenSession())


Code formatting is not consistent, better issue a global reformat on new files (CTRL+K then CTRL+D in VS). (But it has to be avoided on already existing files, in order to avoid unrelated changes on them.)

There are some other cases of formatting glitches to fix in this file, I am not pointing them all with comments.

fredericDelaporte · 2017-12-04T11:27:35Z

src/NHibernate.Test/NHSpecificTest/GH948/FixtureByCode.cs

+			using (var session = OpenSession())
+			{
+				Sfi.Statistics.Clear();
+				EntityComplex entitypRoot = session


pRoot? I guess the p is a typo. Otherwise for what does it stand for?

fredericDelaporte · 2017-12-04T12:19:15Z

src/NHibernate/Criterion/EntityProjection.cs

+
+namespace NHibernate.Criterion
+{
+	using TThis = EntityProjection;


This is not a pattern used in NHibernate. I am not favorable to introducing it. I would rather have methods yielding their class do it under its actual name without using this alias.

fredericDelaporte · 2017-12-04T12:34:00Z

src/NHibernate.Test/NHSpecificTest/GH948/FixtureByCode.cs

+using NHibernate.Transform;
+using NUnit.Framework;
+
+namespace NHibernate.Test.NHSpecificTest.GH948


For having a more sound default ordering, please pad with 0 the number in order to have a four digits number. So the folder and the namespace should be renamed to GH0948.

In fact we favor putting tests in their functional area, so it would be even better to move them to Criteria/ProjectionsTest.cs.

I didn't do it because I have my set of entities specifically for my needs and I don't like hbm configuration (and it's not clear to me how to mix them).

Better add another test file in such case, like Criteria/EntityProjectionsTest.cs. Mixing mapping methods is not an option.

It doesn't look to me as clean solution to mix all sort of various tests depending on different data in one file and do set up/clean up for all of them for each test method. I would really prefer keep my tests in separate class

I do not really get that point since that is the fate of most test fixtures, even your isolated current one: it already sets up a composite entity for the last tests, unused by the other tests.
Of course we should try to reuse other test entities if possible by default. But when there is a good reason, not re-using them is ok too. Anyway when it is impractical to add to the existing fixture, you can add just another one in the same folder as I propose just above.

Maybe it's better to introduce Criteria/Projections folder? And put files there (for now two files - ProjectionsTest.cs and new EntityProjectionTest.cs)

They do not need their own folder. There are not so many fixtures in Criteria.

fredericDelaporte · 2017-12-04T12:42:37Z

src/NHibernate/Criterion/EntityProjection.cs

+
+		public TThis SetAllPropertyFetch(bool fetchLazyProperties = true)
+		{
+			FetchLazyProperties = fetchLazyProperties;


Naming inconsistency. I would rather have it named SetLazyPropertiesFetch.

fredericDelaporte · 2017-12-04T12:43:51Z

src/NHibernate/Criterion/EntityProjection.cs

+	using TThis = EntityProjection;
+
+	[Serializable]
+	public class EntityProjection : IProjection


This new class is exposed publicly by the criteria API, it should have xml comments on all its public members.

fredericDelaporte · 2017-12-04T12:48:12Z

src/NHibernate/Criterion/EntityProjection.cs

+
+			PropertyColumnAliases = Lazy
+				? new string[][] { }
+				: Enumerable.Range(0, Persister.PropertyNames.Length).Select(i => Persister.GetPropertyAliases(_columnAliasSuffix, i)).ToArray();


Haven't we an inconsistency here compared to ToSqlString implementation? If there is a lazy property, it looks to me it will be included here in the property aliases whatever the value of FetchLazyProperties, while it will not be in the emitted SQL if FetchLazyProperties is false. But I have not checked if this could create an issue.

Indexes for PropertyColumnAliases must be the same as in Persister.PropertyNames (it's used somewhere deep inside for property mapping - don't remember exact place)

So I guess I could only add some micro-optimization to avoid alias calculation for lazy properties and return empty string collection for it. If you think it's worth it.

So I guess I could only add some micro-optimization to avoid alias calculation for lazy properties and return empty string collection for it. If you think it's worth it.

No, how this part of the PR is currently is ok for me if it is already a constraint of current code base to always have lazy properties listed here.

fredericDelaporte · 2017-12-04T13:14:33Z

src/NHibernate.Test/NHSpecificTest/GH948/FixtureByCode.cs

+using NHibernate.Transform;
+using NUnit.Framework;
+
+namespace NHibernate.Test.NHSpecificTest.GH948


In fact we favor putting tests in their functional area, so it would be even better to move them to Criteria/ProjectionsTest.cs.

fredericDelaporte · 2017-12-04T13:22:50Z

src/NHibernate/Criterion/EntityProjection.cs

+				_entityAlias = criteria.Alias;
+			}
+
+			Persister = (IQueryable) criteriaQuery.Factory.GetEntityPersister(RootEntity.FullName);


This cast will fail if a custom persister is used. The code should handle more gracefully this case, maybe with something like:

Persister = criteriaQuery.Factory.GetEntityPersister(RootEntity.FullName) as IQueryable ?? throw new InvalidOperationException($"Projecting to entities requires a NHibernate.Persister.Entity.IQueryable persister, {RootEntity.FullName} does not have one.");

Ok.

For some reasons I can't comment under your previous comment. So answering here:

In fact we favor putting tests in their functional area, so it would be even better to move them to Criteria/ProjectionsTest.cs.

I didn't do it because I have my set of entities specifically for my needs and I don't like hbm configuration (and it's not clear to me how to mix them). It doesn't look to me as clean solution to mix all sort of various tests depending on different data in one file and do set up/clean up for all of them for each test method. I would really prefer keep my tests in separate class
Maybe it's better to introduce Criteria/Projections folder? And put files there (for now two files - ProjectionsTest.cs and new EntityProjectionTest.cs)

I have slightly simplified my proposed code in previous comment.

For some reasons I can't comment under your previous comment.

I have seen this happening on the conversation page, where comments appears somewhat regrouped by their respective review, and may appears in subsequent ones too. You have to find the "right" group of comments. Or switch to files tab and answer from there.

fredericDelaporte · 2017-12-04T13:47:20Z

src/NHibernate/Criterion/EntityProjection.cs

+
+			PropertyColumnAliases = Lazy
+				? new string[][] { }
+				: Enumerable.Range(0, Persister.PropertyNames.Length).Select(i => Persister.GetPropertyAliases(_columnAliasSuffix, i)).ToArray();


So I guess I could only add some micro-optimization to avoid alias calculation for lazy properties and return empty string collection for it. If you think it's worth it.

No, how this part of the PR is currently is ok for me if it is already a constraint of current code base to always have lazy properties listed here.

fredericDelaporte · 2017-12-04T13:59:17Z

src/NHibernate/Criterion/EntityProjectionType.cs

+			return entity;
+		}
+
+		private static object CreateInitializedEntity(DbDataReader rs, ISessionImplementor session, IQueryable persister, object identifier, string[][] propertyAliases, LockMode lockMode, bool fetchLazyProperties, bool readOnly)


I guess all that has been deducted from the loader logic. I am uncomfortable with having it re-coded here. I wonder how eager loading works for other cases like HQL or Linq. We should try to re-use its implementations for those other cases.
(From how is implemented ManyToOneType and EntityType, I tend to think that from the query results something hydrates and put eagerly fetched entities into the session cache before the ManyToOneType is hydrated, allowing it to not query the db.)

The thing is - I didn't find anything reusable.

I tend to think that from the query results something hydrates and put eagerly fetched entities into the session cache before the ManyToOneType is hydrated, allowing it to not query the db

I'm not sure I'm following. How this logic is related here - we are in the state when query is already executed. And we need to materialize our results to entities.
My current understanding ManyToOneType doesn't support loading of initialized entities. It only reads identifier from data reader. If entity needs to be fully initialized - separate query is executed (that's exact behavior of #302)

I'm not sure I'm following. How this logic is related here

Ok - I got it now ) And got me thinking how my code would behave if some entities are already in session.

And got me thinking how my code would behave if some entities are already in session.

Good point, things would likely not be coherent in such case with current state of the PR. Normally NHibernate yields the entity already in the session, with the state it has already, disregarding the data from the query. So this is another reason to check how eager loading is implemented for other cases. (I have never checked that, I am not more knowledgeable than you on this subject.)

bahusoid · 2017-12-08T08:21:29Z

@fredericDelaporte I tried to address all the requested changes.
Also now "materializing entities" logic is reused from Loader (Now this change is more intrusive - as I needed to change Loader code to make it reusable).

fredericDelaporte · 2017-12-11T16:32:38Z

I am sorry, this is not what I was asking for.
You should not have to call the loader logic from the entity type. The new entity type should not even be needed.

I think the work should be done by adapting the CriteriaLoader for it to handle your new projection semantic (projection yielding entities, which should likely be typed with ManyToOne). Currently, when it sees a projection, the logic for handling entities in the result set is by-passed, as if it were assuming a projection can not load entities. See CriteriaJoinWalker constructor.

I think your feature requires changes in CriteriaLoader/CriteriaJoinWalker for enabling the entity loading for projections. Then the loader will do its work, loading the entities by the way, without additional changes in it. Loading entities is the responsibility of the loader, it should not need to be "externalized" in an additional NHibernate type as this PR do.

This entity loading should happen in Loader.GetRow, but currently it cannot because the query "tells" the loader there are no entities to be loaded (persisters and keys array parameters are empty).

bahusoid · 2017-12-11T16:52:32Z

@fredericDelaporte Ok. Thanks for pointers. Will look into it...

fredericDelaporte · 2017-12-11T17:21:15Z

I have got hinted at these pointers by reviewing #1460. From the loader I have searched what should be changed for causing it to load the entities.

bahusoid · 2017-12-17T13:49:35Z

@fredericDelaporte I've made yet another attempt to implement it. Could you please review my latest commit and share your thoughts. The challenge with your suggested approach - how to inject info about "entities projections" as CriteriaJoinWalker has access only for single IProjection that wraps any other projections in to ProjectionList or some other projections aggregator.

I've extended CriteriaQueryTranslator (that's supplied to projection as ICriteriaQuery parameter) implementation with ISupportEntityProjectionCriteriaQuery.RegisterEntityProjection interface (ICriteriaQuery can be extended directly instead). That allows me to register entities in translator right from projection:
https://github.com/bahusoid/nhibernate-core/blob/1c6176316bc0e1b73e9155c6174b109a05b6ad8d/src/NHibernate/Criterion/EntityProjection.cs#L160-L169

fredericDelaporte

I have not yet checked all the details but it looks good.
I will do another review later.

ICriteriaQuery can be extended directly instead

This could be a "6.0 Todo". Adding methods to an interface is a breaking change and so cannot happen in a minor version. Add a comment like this above ISupportEntityProjectionCriteriaQuery:

// 6.0 Todo: merge into `ICriteriaQuery`.

fredericDelaporte · 2017-12-19T14:32:06Z

src/NHibernate/Criterion/ProjectionsExtensions.cs

+		/// </summary>
+		public static T AsEntity<T>(this T alias) where T:class
+		{
+			throw new Exception("Not to be used directly - use inside QueryOver expression");


Do not throw bare Exception. For such case we tend to throw an InvalidOperationException (as in MappedAs Linq function).

I just copy-pasted it from some method above. Actually all methods in this ProjectionsExtensions class throw this Exception("Not to be used directly.... So I guess it's better be uniform and throw the same kind of exception for all cases. And if change it - it should be done for all cases at once and not in scope of this PR. Don't you agree?

Ok, I was thinking the code was already purged from throwing bare Exception, but in fact that is only ApplicationException which were removed (#602, its title applies to the cases where it served as base class).

So yes, keep it that way.

Need to be aligned with #1498

fredericDelaporte

There is some clean-up to do and a breaking change to fix, but otherwise it still looks good.

Now since I am not using Criteria or QueryOver much, it would be better if someone else review this too.

fredericDelaporte · 2017-12-19T18:07:11Z

src/NHibernate/Criterion/EntityProjection.cs

+		/// <summary>
+		/// Entity alias
+		/// </summary>
+		public string[] Aliases


All IProjection members are explicitly implemented excepted this one, but I do not see for which reason.

By the way we tend to use expressions for such properties, when coding some new one: bool IProjection.IsAggregate => false; by example.

fredericDelaporte · 2017-12-19T18:15:52Z

src/NHibernate/Criterion/EntityProjection.cs

+
+		SqlString IProjection.ToSqlString(ICriteria criteria, int position, ICriteriaQuery criteriaQuery)
+		{
+			//return new SqlString(string.Empty);


Forgotten trash?

fredericDelaporte · 2017-12-19T18:19:13Z

src/NHibernate/Criterion/EntityProjection.cs

+
+		SqlString IProjection.ToGroupSqlString(ICriteria criteria, ICriteriaQuery criteriaQuery)
+		{
+			throw new NotImplementedException();


For this method, all existing throwing cases are throwing throw new InvalidOperationException("not a grouping projection"); instead.

fredericDelaporte · 2017-12-19T18:24:56Z

src/NHibernate/Criterion/EntityProjection.cs

+
+		TypedValue[] IProjection.GetTypedValues(ICriteria criteria, ICriteriaQuery criteriaQuery)
+		{
+			throw new NotImplementedException();


Shouldn't this be return Array.Empty<TypedValue>();? There are no SQL parameters associated with this projection, so just return empty. That is already done so for SimpleProjection by example.

fredericDelaporte · 2017-12-19T18:32:59Z

src/NHibernate/Criterion/ProjectionsExtensions.cs

+		/// Create an alias for a projection
+		/// </summary>
+		/// <param name="projection">the projection instance</param>
+		/// <param name="alias">LambdaExpression returning an alias</param>


It looks like a string here. And this method seems to be a somewhat unrelated small feature addition.

@fredericDelaporte Yep. It's accidentally leaked here. But can I keep it? It's convenient extension for using with nameof (which I think should be preferred over lambda expression):
Projections.RootEntity().WithAlias(nameof(r.Root))
vs
Projections.RootEntity().WithAlias(() => r.Root)

Though it's easily can be placed directly in user code - I'm not going to push it :) .

I think it can stay.

About lambda vs nameof, I agree. Almost all QueryOver API is somewhat obsoleted by nameof, since it allows using the Criteria API without magic strings.

fredericDelaporte · 2017-12-19T18:42:52Z

src/NHibernate/Loader/AbstractEntityJoinWalker.cs


 			InitPersisters(allAssociations, lockMode);
 			InitStatementString(whereString, orderByString, lockMode);
 		}

-		protected void InitProjection(SqlString projectionString, SqlString whereString, SqlString orderByString, SqlString groupByString, SqlString havingString, IDictionary<string, IFilter> enabledFilters, LockMode lockMode)
+		protected void InitProjection(SqlString projectionString, SqlString whereString, SqlString orderByString, SqlString groupByString, SqlString havingString, IDictionary<string, IFilter> enabledFilters, LockMode lockMode, IList<EntityProjection> entityProjections = null)


This is a breaking change. Better obsolete the previous signature, telling to call the new one, and call the new one from the old one. (The default value for the new parameter will not avoid this to be a binary compatibility issue. Drop that default value by the way.)

Ok. Will do. But are you sure regarding binary compatibility? I was under impression that default value is just a syntactic sugar - 2 methods should be generated in IL. So it should not be a breaking change (I didn't make actual tests - will do it later too)

Unfortunately it will generate one method.

Yep. Just checked with Resharper IL Viewer window.. Good to know..

fredericDelaporte · 2017-12-19T18:53:51Z

src/NHibernate/Loader/AbstractEntityJoinWalker.cs

 			InitStatementString(projectionString, whereString, orderByString, groupByString, havingString, lockMode);
+


Undue whitespace, better remove this tabs line.

fredericDelaporte · 2017-12-19T18:59:47Z

src/NHibernate/Loader/Criteria/CriteriaQueryTranslator.cs

@@ -26,6 +26,7 @@ public class CriteriaQueryTranslator : ICriteriaQuery
 		private readonly string rootEntityName;
 		private readonly string rootSQLAlias;
 		private int indexForAlias = 0;
+		private IList<EntityProjection> _entityProjections = new List<EntityProjection>();


This can be readonly.

Also, please make field type of List instead of IList to avoid unnecessary boxing.

@hazzik Ok. But could you please clarify what boxing do you expect to see? I've just checked with Resharper IL viewer and don't see any difference in initializing both IList<EntityProjection> or List<EntityProjection>. IList<EntityProjection> is directly implemented by List<EntityProjection> - no (co/contr)-variant conversion needed.

And from what I see I can directly assign IEnumerable<object> objs = new List<EntityProjection>(); without any penalties with implicit hidden conversions.

The enumerator of List is declared as struct and exposed as bare type. Both IEnumerable<>.GeEnumerator and IEnumerable.GetEnumerator implemented as explicit and there will be boxing of enumerator when it is accessed from IEnumerable.GetEnumerator

https://marcinjuraszek.com/2013/10/playing-around-with-listt-part-two-ienumerable-and-ienumerablet-implementation.html

@hazzik Interesting! Thanks. So it's about the price of single boxing per each enumeration via foreach. But that means that I also need to expose direct List<> class in GetEntityProjections() method and InitProjection method should also expect direct List<> class:

protected void InitProjection(..., *List<EntityProjection> entityProjections*)

Did I get it right? Are you sure you want me to go this way (except field type changing - it's done already)? Considering that there is no enumeration via foreach/LINQ in current implementation.

Also it seems it helps to avoid boxing only if foreach is used on list. It's still boxed for any LINQ calls (which is logical but I was hoping to see some CLR magic :) ):
https://dotnetfiddle.net/voVECl

No, just field.

fredericDelaporte · 2017-12-19T19:01:24Z

src/NHibernate/Loader/Criteria/CriteriaQueryTranslator.cs

@@ -45,6 +46,7 @@ public class CriteriaQueryTranslator : ICriteriaQuery
 		private readonly ICollection<IParameterSpecification> collectedParameterSpecifications;
 		private readonly ICollection<NamedParameter> namedParameters;
 		private readonly ISet<string> subQuerySpaces = new HashSet<string>();
+		public IList<EntityProjection> entityProjections = new List<EntityProjection>();


Forgotten trash? Public field and not used.

fredericDelaporte

The SetField logic looks a bit brittle to me, I am going to push a commit with my proposed changes.

fredericDelaporte · 2017-12-20T11:30:34Z

src/NHibernate/Criterion/EntityProjection.cs

+
+		IType[] IProjection.GetTypes(string alias, ICriteria criteria, ICriteriaQuery criteriaQuery)
+		{
+			return new[] { criteriaQuery.GetType(criteria, alias) };


I am only seeing this now although it is here from the beginning (and maybe it was here in previous PR too): when looking at how this function is implemented and used, it does not look to me as an expected implementation.
Inferring from its usage in CriteriaQueryTranslator.GetTypeUsingProjection and its implementation in other classes like AliasedProjection, I would expect more something like:

SetFields(criteria, criteriaQuery); return _entityAlias == alias ? _types : null;

@fredericDelaporte Yes. It's something that was copy-pasted from initial implementation. Actually after looking into it I think it should always return null. We reuse alias that's already present in query and it's already associated with an entity - I think we shouldn't interfere here and let query to resolve alias and handle it.
I was trying to come up with some examples where your implementation could be useful and failed.

P.S. I also realized that GetColumnAliases(string alias,.. is also wrong - it's simply provides entity projection aliases for any supplied propertyPath. I think it also should always return null.

Actually after looking into it I think it should always return null.

Agreed.

I also realized that GetColumnAliases(string alias,.. is also wrong

Good catch. It should be aligned with GetTypes(string alias, ... implementation.

fredericDelaporte · 2017-12-20T11:32:45Z

src/NHibernate/Criterion/EntityProjection.cs

+		private void SetFields(ICriteria criteria, ICriteriaQuery criteriaQuery)
+		{
+			//Persister is required, so let's use it as "initialized marker"
+			if (Persister != null)


This lazy SetFields may cause issues, because it depends on receiving the root criteria on its first call for doing properly its job, while it could receive a sub-criteria instead. criteria.Alias will then not be the correct alias for initializing _entityAlias. (Other usages have their implementation delegated to the root criteria.)

I think it should rely on criteriaQuery only, by adding another member to ISupportEntityProjectionCriteriaQuery for getting the root criteria.

@fredericDelaporte Are you sure? From what I see Subcriteria.SetProjection use rootCriteria. Or did you mean some other scenario?

Many methods of EntityProjection are calling SetFields, and these many methods can each be called with something else than the root criteria. Reading from the code, I am unable to ascertain the first SetFields call would forcibly be done with the root criteria.
Can you tell me if you are sure its first call can only be done with the root criteria, and why are you sure of this?

I do not get by the way why you refer to Subcriteria.SetProjection. Yes it delegates its implementation to the root criteria. But here I am pointing to criteria.Alias, which does not.

My understanding is the following: from one side projections are assigned to some criterias. And from the other side projections methods should be called in context of assigned criteria (what the point of calling it for anything else?).
So it's more logical to me to use exact provided criteria for fields initialization.

while it could receive a sub-criteria instead
I do not get by the way why you refer to Subcriteria.SetProjection

And so if subcriteria projections are delegated to root criteria - I expect projections methods to be called in context of root criteria.

All 3 methods (ToSqlString, GetTypes, GetColumnAliases) entry points where SetFields is currently called are executed for root criteria:

nhibernate-core/src/NHibernate/Loader/Criteria/CriteriaQueryTranslator.cs

Lines 167 to 187 in c020a40

public SqlString GetSelect()

{

return rootCriteria.Projection.ToSqlString(rootCriteria.ProjectionCriteria, 0, this);

}

internal IType ResultType(ICriteria criteria)

{

return TypeFactory.ManyToOne(GetEntityName(criteria));

//return Factory.getTypeResolver().getTypeFactory().manyToOne(getEntityName(criteria));

}

public IType[] ProjectedTypes

{

get { return rootCriteria.Projection.GetTypes(rootCriteria, this); }

}

public string[] ProjectedColumnAliases

{

get { return rootCriteria.Projection.GetColumnAliases(0, rootCriteria, this); }

}

Maybe I'm wrong but it looks like unnecessary complication with additional interface extension.

All this entry points are public and may be called by code outside of NHibernate, so even if it were true they were always called with rootCriteria, there is still no guarantee about it. So if we agree alias must be taken from the root criteria when initializing a projection for root entity (for consistency with GetRootEntityTypeIfAvailable by the way, which always takes it from root criteria), then we should ensure that we will forcibly take it from the root criteria, even if something else than the root criteria is used to call one of these entry points.

Moreover, in current NHibernate code base, IProjection.GetTypes is also called from InExpression.ToSqlString (which is from ICriterion, not from IProjection), which can be called with criteria that do not look to be the root one. Same for IProjection.ToSqlString, called from IdentifierEqExpression.ToSqlString. Granted, it is unlikely a valid query could be done we those expressions calling an entity projection. But still, better be safe in my opinion.

The rootCriteria.ProjectionCriteria sent in IProjection.ToSqlString adds also to the risk. Currently it seems always null or being equal to rootCriteria, being affected only by this. (I guess it will not be null if that call occurs by the way, otherwise we are in troubles too). But some code (the Clone) assumes it could be something else too. Maybe that is an obsolete thing, or maybe that is here for being future proof.

The contract of those entry points does not stipulate they must be called with the root criteria, so relying on this is not future proof.

fredericDelaporte · 2017-12-20T17:27:24Z

src/NHibernate.Test/Criteria/EntityProjectionsTest.cs

+		}
+
+		[Test]
+		public void EntityProjectionLockMode()


This one fails for Oracle. It generates the following query:

select id1_1_0_,name2_1_0_ from ( SELECT child1x1_.Id as id1_1_0_, child1x1_.Name as name2_1_0_ FROM EntityComplex this_ inner join EntitySimpleChild child1x1_ on this_.Child1Id=child1x1_.Id ) where rownum <=:p0 for update of child1x1_.Id

It fails with the error:

ORA-00904: "CHILD1X1_"."ID" : invalid identifier

The for update clause is bad. I think this is an unrelated bug, indeed #1352.
So better add something like

if (Dialect is Oracle8iDialect) Assert.Ignore("Oracle is not supported due to #1352 bug (NH-3902)");

fredericDelaporte · 2017-12-21T15:18:17Z

I have adjusted some assert messages, which were not much helpful due to being redundant with asserts in the same test.

bahusoid · 2017-12-22T16:39:59Z

@fredericDelaporte Ok. So it seems all pending questions are resolved - I agree with your interface extension logic.

hazzik · 2017-12-23T11:35:34Z

src/NHibernate/Criterion/EntityProjection.cs

+
+			if (!(criteriaQuery is ISupportEntityProjectionCriteriaQuery entityProjectionQuery))
+			{
+				throw new HibernateException($"Projecting to entities requires a '{criteriaQuery.GetType().FullName}' type to implement {nameof(ISupportEntityProjectionCriteriaQuery)} interface.");


Maybe it shall be ArgumentException?

hazzik

Some minor questions to exceptions, otherwise looks good.

fredericDelaporte

Minor exceptions changes done.

fredericDelaporte · 2018-03-17T11:03:01Z

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.

(Yeah, copy/paste from #1545, but here it is a different new criteria/queryover feature.)

bahusoid changed the title ~~#948(NH-3435) - Ability to select entities in Criteria projections~~ #948(NH-3435) - Ability to select entities in Criteria projections (new version) Dec 3, 2017

bahusoid mentioned this pull request Dec 4, 2017

#948(NH-3435) - Ability to select entities in a criteria projections (rebased PR #302) #1458

Closed

fredericDelaporte added the t: Fix label Dec 4, 2017

fredericDelaporte reviewed Dec 4, 2017

View reviewed changes

fredericDelaporte added the needs work label Dec 7, 2017

bahusoid force-pushed the GH948New branch from dcdebc6 to b14ce72 Compare December 8, 2017 08:13

fredericDelaporte removed the needs work label Dec 8, 2017

fredericDelaporte self-requested a review December 8, 2017 09:41

fredericDelaporte added the needs work label Dec 11, 2017

fredericDelaporte removed their request for review December 11, 2017 16:36

bahusoid added 3 commits December 16, 2017 20:51

Ability to select entities in Criteria API projections

dc5c64d

Fixed test warnings

1c9cfec

Address requested changes

43e4fd6

bahusoid force-pushed the GH948New branch 2 times, most recently from 28639ee to 1c61763 Compare December 17, 2017 02:57

fredericDelaporte removed the needs work label Dec 18, 2017

fredericDelaporte self-requested a review December 18, 2017 11:26

Inject into CriteriaLoader info about projected entities

a21fb4a

bahusoid force-pushed the GH948New branch from 1c61763 to a21fb4a Compare December 19, 2017 03:38

fredericDelaporte reviewed Dec 19, 2017

View reviewed changes

fredericDelaporte self-requested a review December 19, 2017 14:46

fredericDelaporte reviewed Dec 19, 2017

View reviewed changes

Clean up

2475764

bahusoid force-pushed the GH948New branch from 15ad5c5 to 2475764 Compare December 20, 2017 09:37

Adjustment to entity projection initialization. To be squashed.

dae9cb1

fredericDelaporte reviewed Dec 20, 2017

View reviewed changes

fredericDelaporte and others added 2 commits December 20, 2017 13:12

Merge branch 'master' into GH948New

4354fe1

Do not interfere with alias resolution

2c1d77d

fredericDelaporte reviewed Dec 20, 2017

View reviewed changes

bahusoid and others added 3 commits December 21, 2017 11:18

Skip LockMode test for Oracle due to NH-3902

36e8ff2

small code refactoring

9ece50d

Adjustments to assert messages.

bb0cc45

fredericDelaporte previously approved these changes Dec 21, 2017

View reviewed changes

fredericDelaporte added this to the 5.1 milestone Dec 21, 2017

fredericDelaporte self-assigned this Dec 21, 2017

Merge branch 'master' into GH948New

310644f

hazzik reviewed Dec 23, 2017

View reviewed changes

hazzik previously approved these changes Dec 23, 2017

View reviewed changes

fredericDelaporte and others added 2 commits December 23, 2017 12:44

Merge branch 'master' into GH948New

e1fde7a

Adjust exceptions, to be squashed.

fe3aa9d

fredericDelaporte dismissed stale reviews from hazzik and themself via fe3aa9d December 23, 2017 12:12

fredericDelaporte approved these changes Dec 23, 2017

View reviewed changes

hazzik changed the title ~~#948(NH-3435) - Ability to select entities in Criteria projections (new version)~~ Ability to select entities in Criteria projections Dec 23, 2017

hazzik merged commit f2969d0 into nhibernate:master Dec 23, 2017

fredericDelaporte added the r: Fixed label Dec 23, 2017

bahusoid mentioned this pull request Mar 11, 2018

5.1.0 release #1525

Merged

fredericDelaporte mentioned this pull request Mar 11, 2018

NH-3435 - Ability to select entities in Criteria projections #948

Closed

bahusoid mentioned this pull request Mar 18, 2018

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

Merged

bahusoid mentioned this pull request Jun 7, 2018

Release 5.1.3 #1716

Merged

		InitStatementString(projectionString, whereString, orderByString, groupByString, havingString, lockMode);

	public SqlString GetSelect()
	{
	return rootCriteria.Projection.ToSqlString(rootCriteria.ProjectionCriteria, 0, this);
	}

	internal IType ResultType(ICriteria criteria)
	{
	return TypeFactory.ManyToOne(GetEntityName(criteria));
	//return Factory.getTypeResolver().getTypeFactory().manyToOne(getEntityName(criteria));
	}


	public IType[] ProjectedTypes
	{
	get { return rootCriteria.Projection.GetTypes(rootCriteria, this); }
	}

	public string[] ProjectedColumnAliases
	{
	get { return rootCriteria.Projection.GetColumnAliases(0, rootCriteria, this); }
	}

Ability to select entities in Criteria projections #1471

Ability to select entities in Criteria projections #1471

Uh oh!

Conversation

bahusoid commented Dec 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Dec 4, 2017

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!

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!

bahusoid Dec 4, 2017 • 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 Dec 4, 2017 • 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!

fredericDelaporte Dec 4, 2017 • 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!

fredericDelaporte Dec 4, 2017 • 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!

bahusoid Dec 4, 2017 • 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 commented Dec 8, 2017

Uh oh!

fredericDelaporte commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bahusoid commented Dec 11, 2017

Uh oh!

fredericDelaporte commented Dec 11, 2017

Uh oh!

bahusoid commented Dec 17, 2017

Uh oh!

fredericDelaporte left a comment

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

bahusoid commented Dec 3, 2017 •

edited

Loading

bahusoid Dec 4, 2017 •

edited

Loading

fredericDelaporte Dec 4, 2017 •

edited

Loading

fredericDelaporte Dec 4, 2017 •

edited

Loading

fredericDelaporte Dec 4, 2017 •

edited

Loading

bahusoid Dec 4, 2017 •

edited

Loading

fredericDelaporte commented Dec 11, 2017 •

edited

Loading