Skip to content

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

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

Closed
wants to merge 3 commits into from

Conversation

bahusoid
Copy link
Member

@bahusoid bahusoid commented Nov 28, 2017

Fixes #948.
This PR allows to select entities (lazy/eagerly) via Criteria API (via Projections.Entity and Projections.RootEntity helper methods).

I rebased #302 pull request +

  1. Added more tests (lazy and eager version);
  2. Refactored BaseEntityProjection to use ILoadable interface instead of AbstractEntityPersister;
  3. Extended Projections helper class to expose lazy setting and added helper for QueryOver.

Thanks Ricardo Peres for his work!

@bahusoid bahusoid changed the title GH948(NH-3435) - Ability to select entities in a criteria projection (rebased PR #302) #948(NH-3435) - Ability to select entities in a criteria projections (rebased PR #302) Nov 28, 2017
@fredericDelaporte fredericDelaporte self-requested a review November 28, 2017 18:32
@bahusoid
Copy link
Member Author

bahusoid commented Nov 28, 2017

Sorry. Somehow I messed up with tests in final commit.... Fixed

1) Added more tests (lazy and eager version);
2) Refactored BaseEntityProjection to use ILoadable interface instead of AbstractEntityPersister;
3) Extended Projections helper class to expose lazy setting and added helper for QueryOver.
@bahusoid
Copy link
Member Author

And just realized that eager version is not really eager - criteria leads to 2 selects ID only and full entity... ( And fixing it doesn't look easy...

@fredericDelaporte
Copy link
Member

#302/#948 were about choosing the root entity to load. Does this point works as expected? (Loads fully the desired entity, not just uninitialized proxies.) If yes, better re-target this PR at just that. And maybe open new issues/pr for the eager fetching issues. (Check before if none are already existing for this other subject.)

Test failures for Postgres, Oracle and Firebird are all alike: for some reason, it seems it generates bad column names, alias names or badly combine both.

@bahusoid
Copy link
Member Author

bahusoid commented Nov 29, 2017

@fredericDelaporte But is it really useful?
The root of the issue described in https://nhibernate.jira.com/browse/NH-3435 based on stackoverflow question http://stackoverflow.com/questions/15744057/nhibernate-how-to-select-the-root-entity-in-a-projection.

Both JIRA issue and stackoverflow question by the same author.

And the problem he tries to solve - implement Criteria API variant of the following HQL query:

session.CreateQuery("select b, rowcount() from Blog b")
              .SetFirstResult(5)
              .SetMaxResults(10)
              .List();

And HQL does load entities eagerly.

Also see comment in sackoverflow question:

Yes, I know I could just use CriteriaTransform.TransformToRowCount() but that would require executing the query twice - once for the results and once for the row count. Using Futures may help a little by reducing it to one round-trip, but it's still executing the query twice on the SQL Server. For intensive queries this is unacceptable. I want to avoid the overhead, and return the row count and the results in the same query.

So I would say issue was created specifically for supporting projections for fully initialized entities.

And if I understand correctly @rjperes first comment. His first implementation was exactly about eager loading (via result transformer). Comment from his linked article:

RootEntityProjection selects all the properties of the criteria’s root entity and EntityProjection selects all the properties of any joined entity, selected by its alias.

And for some reasons final proposed implementation transformed into lazy loading.

IMHO standard implementation if exists should support eager loading . Otherwise it's not very useful.

@fredericDelaporte
Copy link
Member

Ok, so the issue (the migrated one on GitHib, #948) should be updated for better matching this. May you add a comment redefining the issue? I would then update its title and first comment.

@bahusoid
Copy link
Member Author

bahusoid commented Nov 29, 2017

@fredericDelaporte It seems we have some kind of misunderstanding (maybe I'm using wrong terminology).

#302/#948 were about choosing the root entity to load. Does this point works as expected? (Loads fully the desired entity, not just uninitialized proxies.)

Current implementation doesn't fully load entities (no matter root or not) - it loads uninitialized proxies instead (only ID column retrieved from database). And that's the problem I complain about - I expect them to be fully initialized.

@fredericDelaporte
Copy link
Member

Well, there is some mis-understanding. I would not use "eager loading" term for the case where we select an entity along with some additional data (the rowcount here). From what you write, I now understand we happen to have a bug in this PR because instead of actually selecting the entities it just returns uninitialized proxies. Then for me such a point has to be fixed in this PR, and we should not need to specify lazy false or anything like that for having the entity fully loaded by the this query.

When I see "eager loading", I am thinking about related entities, say by example having also a rootEntity.SomeRelatedEntity loaded along. If such a case was also needing some work, then it should likely have its own PR.

Now I have to say I am not using Criteria much, so my view may not be the right one.

@bahusoid
Copy link
Member Author

bahusoid commented Dec 1, 2017

As I suspected retrieving initialized entities wasn't an easy fix. I made a draft with such support and there is not much left from initial Ricardo's implementation.
Entities are initialized but lazy properties are not fetched by default. Can be configured in fluent syntax:

Projections.Entity(()=> entityAlias)
.SetLazy(lazy)
.SetAllPropertyFetch(lazyProps)
.SetReadonly(readOnly)

@fredericDelaporte , @hazzik Could you please review specifically my latest commit. I temporarily isolated changes in test solution (see EntityProjection.cs and EntityProjectionType.cs. I also temporarily modified test csproj to include only related tests (new feature doesn't affect any existing functionality - and I hope it will make tests executed on server much quicker)

If it's not convenient - please advice how to proceed. Maybe I should simply create anther pull request with only my changes.

_entityAlias = criteria.Alias;
}

Persister = (IQueryable) criteriaQuery.Factory.GetEntityPersister(RootEntity.FullName);
Copy link
Member Author

@bahusoid bahusoid Dec 1, 2017

Choose a reason for hiding this comment

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

Is this valid cast? Any more proper way to obtain IQueryable. This interface gives convenient methods for SQL generation: IdentifierSelectFragment and PropertySelectFragment

return entity;
}

private static object CreateInitializedEntity(DbDataReader rs, ISessionImplementor session, IQueryable persister, object identifier, string[][] propertyAliases, LockMode lockMode, bool fetchLazyProperties, bool readOnly)
Copy link
Member Author

Choose a reason for hiding this comment

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

The main concern is this method. If/when it's properly implemented I think it should be moved to some utility class. As finding a way to properly initialize entity from data reader wasn't fun.

@fredericDelaporte
Copy link
Member

I will not have time for reviewing until next week.

@bahusoid bahusoid force-pushed the GH948 branch 2 times, most recently from 0ae9ca6 to a43f698 Compare December 2, 2017 02:22
@bahusoid
Copy link
Member Author

bahusoid commented Dec 3, 2017

To make things more clear I decided to prepare another pull request - #1471

@fredericDelaporte fredericDelaporte removed their request for review December 4, 2017 11:17
@fredericDelaporte fredericDelaporte removed this from the 5.1 milestone Jan 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants