Skip to content

Fix future queries with non lazy associations #2174

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

Merged
merged 6 commits into from
Jul 16, 2019

Conversation

bahusoid
Copy link
Member

@bahusoid bahusoid commented Jun 12, 2019

Fixes #2173 (some technical details behind this issue can be found here)

Implemented the following fix:

  • Moved actual collection initialization logic after all queries are processed (as it's done in legacy MultiCriteriaImpl/MultiQueryImpl)
  • Introduced LoadingCollectionEntry.StopLoading flag that indicates that collection should not be populated. This allows to delay actual collection initialization till all other batch queries are processed (so it's a fix for NH-3350 - Duplicate records using Future() #1293)

Issue is introduced in 5.2 with new QueryBatch implementation.

@bahusoid bahusoid changed the title Test case for #2173 (future with non lazy association) WIP Test case for #2173 (future with non lazy association) Jun 12, 2019
@bahusoid bahusoid force-pushed the gh2173FutureNonLazy branch from 03927b6 to 1148aa9 Compare June 12, 2019 08:39
@bahusoid bahusoid changed the title WIP Test case for #2173 (future with non lazy association) Test case for #2173 (future with non lazy association) Jun 12, 2019
@bahusoid

This comment has been minimized.

@bahusoid bahusoid force-pushed the gh2173FutureNonLazy branch from 27a36e5 to 10f9746 Compare June 12, 2019 15:11
foreach (var query in _queries)
{
if (query.CachingInformation != null)
{
foreach (var cachingInfo in query.CachingInformation.Where(ci => ci.IsCacheable))
Copy link
Member Author

Choose a reason for hiding this comment

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

cacheBatcher is also needed when query is not cacheable but loaded entities are cacheable - so it's better to always set it.

@bahusoid bahusoid changed the title Test case for #2173 (future with non lazy association) Fix future queries with non lazy associations Jun 12, 2019
@hazzik
Copy link
Member

hazzik commented Jun 13, 2019

@bahusoid is it regression? If so, could you please re-target to 5.2.x?

@bahusoid bahusoid changed the base branch from master to 5.2.x June 13, 2019 10:34
@bahusoid bahusoid force-pushed the gh2173FutureNonLazy branch from f12beb8 to 5e39340 Compare June 13, 2019 10:36
@@ -103,7 +103,7 @@ public async Task<int> ProcessResultsSetAsync(DbDataReader reader, CancellationT

queryInfo.Result = tmpResults;
if (queryInfo.CanPutToCache)
queryInfo.ResultToCache = tmpResults;
queryInfo.ResultToCache = new List<object>(tmpResults);
Copy link
Member Author

@bahusoid bahusoid Jun 13, 2019

Choose a reason for hiding this comment

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

When merging to master this change should be ignored. The following code is expected in master (no need to wrap in new List):

queryInfo.ResultToCache = queryCacheBuilder.Result;

@hazzik hazzik added this to the 5.2.6 milestone Jul 16, 2019
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.

2 participants