NH-3596 - DistinctRootEntityResultTransformer breaks Cache #383

rjperes · 2014-11-26T10:25:33Z

https://nhibernate.jira.com/browse/NH-3596 (duplicate of https://nhibernate.jira.com/browse/NH-2867, #1243)
Fix for QueryOver and unit test
HQL still not working

Added HQL unit test

hazzik · 2014-11-27T00:43:02Z

@rjperes, can you please add test for case with Fetch(xxx).Eager ?

Also
CachingWithTransformerTests.UsingHqlToFutureWithCacheAndTransformerDoesntThrow
http://teamcity.codebetter.com/viewLog.html?buildId=138537&buildTypeId=bt7&guest=1#testNameId2115394835934985140
test is failing.

rjperes · 2014-11-27T05:11:06Z

Didn't you read what I wrote? I said that I only fixed QueryOver, not HQL. And I copied existing tests, didn't create new ones.

hazzik · 2014-11-27T08:20:03Z

Of course I did not read JIRA. Actually I did so, but after I wrote my
comment

rjperes · 2014-11-27T08:58:12Z

I thought the solution would be the same, but it wasn't... trickier, I think...

hazzik · 2014-11-27T09:24:48Z

There is one place you can change to get the same error as with QueryOver

QueryLoader.cs L#333

hazzik · 2014-11-27T09:29:14Z

src/NHibernate.Test/NHSpecificTest/NH3596/CachingWithTransformerTests.cs

+		[Test]
+		public void UsingHqlToFutureWithCacheAndTransformerDoesntThrow()
+		{
+			//NH-3596


This is NH-2867

It's the same problem

hazzik · 2014-11-27T09:50:01Z

BTW, for HQL it works completely incorrect. It returns 2 roles: null and Role
With commented result transformer in second query it throws InvalidCastException object[] -> Role

rjperes · 2014-11-27T09:50:40Z

Yes, I know...

hazzik · 2014-11-27T09:54:04Z

It seems that the problem is with self-referenced entities, and not with your solution. Can you adjust tests to use non-recursive entities?

rjperes · 2014-11-27T09:55:12Z

But why, if this has to be fixed as well?

rjperes · 2014-11-27T09:55:33Z

"not with your solution"?

hazzik · 2014-11-27T10:01:05Z

Ok, just forget. I'm talking bullshit.

rjperes · 2014-11-27T10:01:49Z

:-D
No, you are not! You must be sleepy, though... go to bed!!

hazzik · 2014-11-27T10:05:51Z

Ok.... nevermind... We need following tests to cover all the cases:

MultiCriteria/MultiQueryOver
Criteria/QueryOver + Future
MulitQuery
Query + Future
Linq + Future

All of the above with and without cache
All of the above with self-reference and normal entities

hazzik · 2014-11-27T10:33:01Z

So... what I'm talking about... following test query does not work at all

var roles = session
    .CreateQuery("from Role r left join r.Children left join r.Parent")
    .SetResultTransformer(Transformers.DistinctRootEntity)
    .List<Role>();

    Assert.AreEqual(5, result.Count); // always 2

rjperes · 2014-11-27T10:33:53Z

Yes, I know! That's why I said it has an error! But it should work!

hazzik · 2014-11-27T10:35:04Z

Not as part of solving this issue... The query does not use multiquery/future nor cache.

rjperes · 2014-11-27T10:36:42Z

But where did that code come from? It's not in my commit, for sure.

hazzik · 2014-11-27T10:38:26Z

I'm playing with your pull request. So I modified the query (removed multi-query and cahce) to see if this query works on it's own.

rjperes · 2014-11-27T10:41:38Z

I have code like this in a project (NH 4.0.2):

var ordersInHql = session.CreateQuery("from Order o left join fetch o.Details d left join fetch d.Product").List();

And it works perfectly...

hazzik · 2014-11-27T10:45:49Z

And it works perfectly...

Yes, Order-Details-Product works... But Role-Role-Role does not. And that's why I said this: "It seems that the problem is with self-referenced entities", "All of the above with self-reference and normal entities"

rjperes · 2014-11-27T10:47:02Z

Yes, you are right. I don't have much time ATM, maybe later, or if you can handle it, fine! BTW, congrats on the new job! ;-)

hazzik · 2014-11-27T10:58:46Z

Thanks

hazzik · 2014-11-27T11:33:56Z

src/NHibernate.Test/NHSpecificTest/NH3596/CachingWithTransformerTests.cs

+			{
+				//store values in cache
+				session
+					.CreateQuery("from Role r left join r.Children left join r.Parent where r.Parent is null")


should be

"select r from Role r left join r.Children left join r.Parent where r.Parent is null"

I know that it is different from the saved query, but it was like this in the unit test (QueryOver version).

rjperes · 2014-11-27T11:53:54Z

TeamCity does that for you!! :-P

hazzik · 2014-11-27T11:55:02Z

src/NHibernate.Test/NHSpecificTest/NH3596/CachingWithTransformerTests.cs

+					.SetCacheMode(CacheMode.Normal)
+					.Future<Role>();
+
+				//get values from cache


Are you sure that this will be from cache?

It should be, shouldn't it? It's the same query, no parameters...

Don't forget that they are future queries.

The query cache key will be

"select role0_.Id as Id0_0_, children1_.Id as Id0_1_, role2_.Id as Id0_2_, role0_.Name as Name0_0_, role0_.ParentId as ParentId0_0_, children1_.Name as Name0_1_, children1_.ParentId as ParentId0_1_, children1_.ParentId as ParentId0__, children1_.Id as Id0__, role2_.Name as Name0_2_, role2_.ParentId as ParentId0_2_ from Role role0_ left outer join Role children1_ on role0_.Id=children1_.ParentId left outer join Role role2_ on role0_.ParentId=role2_.Id where role0_.ParentId is null;\r\nselect role0_.Id as Id0_0_, children1_.Id as Id0_1_, role2_.Id as Id0_2_, role0_.Name as Name0_0_, role0_.ParentId as ParentId0_0_, children1_.Name as Name0_1_, children1_.ParentId as ParentId0_1_, children1_.ParentId as ParentId0__, children1_.Id as Id0__, role2_.Name as Name0_2_, role2_.ParentId as ParentId0_2_ from Role role0_ left outer join Role children1_ on role0_.Id=children1_.ParentId left outer join Role role2_ on role0_.ParentId=role2_.Id where role0_.ParentId is null;\r\n"

Which means that both queries' results will go to the same cache item...

In other words the first future is pending and not executed before the second one gets executed. So both queries (although being the same) ends-up batched together in a single round-trip to database. So it has nothing to retrieve from cache, since only one actual sql-query is performed.

The previous test has the same issue.

hazzik · 2014-11-27T12:08:04Z

So you need revert last commits and call these methods twice

hazzik · 2014-11-27T12:11:33Z

And then it brakes....

rjperes · 2014-11-27T12:14:25Z

"Which means that both queries' results will go to the same cache item..."
That's the intention, isn't it?
But I think I spotted a problem in QueryParameter parameterless constructor: it is disabling cache by default (the bool cacheable parameter is set to false, and not with the value of cacheMode). This is in SessionImpl, line 606. So, in Loader, line 1525, it won't be use cache.

hazzik · 2014-11-27T12:16:50Z

Ok, just revert last 2 commits, fix HQL queries and call each test method twice...

hazzik · 2014-11-27T12:17:45Z

That's the intention, isn't it?

Yes, and... no...

rjperes · 2014-11-27T12:20:31Z

I don't understand what you mean.
Right now, HQL is identical to QueryOver. In both cases, we issue a first future query that should be cached, and then we issue another one, and we execute it. Results are as expected. While debugging, I noticed that query cache is not being used because QueryParameters is initialized without the cache parameters (cacheable, cacheRegion).

hazzik · 2014-11-27T12:26:20Z

we issue a first future query that should be cached, and then we issue another one, and we execute it.

No, it does not work as this... Because this is a future...

2 queries are combined together (it does not matter if they are identical or not), and after then they are treated as one single query. So they miss/hit cache together. So what I'm trying to say, that in your test you actually only put queries to cache, and do not retrieve them

See the code:

[Test]
public void UsingQueryOverToFutureWithCacheAndTransformerDoesntThrow()
{
    //store values in cache
    UsingQueryOverToFutureWithCacheAndTransformerDoesntThrowCore();
    //get values from cache
    UsingQueryOverToFutureWithCacheAndTransformerDoesntThrowCore();
}

private void UsingQueryOverToFutureWithCacheAndTransformerDoesntThrowCore()
{
//NH-3596
    using (var session = this.OpenSession())
    using (session.BeginTransaction())
    {
        session
            .QueryOver<Role>()
            .Where(x => x.Parent == null)
            .TransformUsing(Transformers.DistinctRootEntity)
            .Cacheable()
            .CacheMode(CacheMode.Normal)
            .Future(); // schedule query for execution

        Role children = null;
        Role parent = null;

        var roles = session
            .QueryOver<Role>()
            .Left.JoinAlias(x => x.Children, () => children)
            .Left.JoinAlias(x => x.Parent, () => parent)
            .TransformUsing(Transformers.DistinctRootEntity)
            .Cacheable()
            .CacheMode(CacheMode.Normal)
            .Future(); // schedule query for execution

        var result = roles.ToList(); // both queries executed here in one roundtrip

        Assert.AreEqual(5, result.Count);
    }
}

[Test]
public void UsingHqlToFutureWithCacheAndTransformerDoesntThrow()
{
    //NH-3596
    //store values in cache
    UsingHqlToFutureWithCacheAndTransformerDoesntThrowCore();
    //get values from cache
    UsingHqlToFutureWithCacheAndTransformerDoesntThrowCore();
}

private void UsingHqlToFutureWithCacheAndTransformerDoesntThrowCore()
{
    using (var session = this.OpenSession())
    using (session.BeginTransaction())
    {
        session
            .CreateQuery("select r from Role r left join r.Children left join r.Parent where r.Parent is null")
            .SetResultTransformer(Transformers.DistinctRootEntity)
            .SetCacheable(true)
            .SetCacheMode(CacheMode.Normal)
            .Future<Role>(); // schedule query for execution

        var roles = session
            .CreateQuery("select r from Role r left join r.Children left join r.Parent")
            .SetResultTransformer(Transformers.DistinctRootEntity)
            .SetCacheable(true)
            .SetCacheMode(CacheMode.Normal)
            .Future<Role>(); // schedule query for execution

        var result = roles.ToList(); // both queries executed here in one roundtrip

        Assert.AreEqual(5, result.Count);
    }
}

rjperes · 2014-11-27T12:28:26Z

OK, but that doesn't affect my fix, and also the "bug" I spotted. My fix makes the unit test work, otherwise, it would fail.

hazzik · 2014-11-27T12:30:04Z

Your fix fixes only half of the problem, only that part when you put results to cache, but does not fix the same problem when you retrieve from cache.

rjperes · 2014-11-27T12:35:14Z

Great! So you'll only have half the trouble! :-)
BTW, do look at that problem I reported...

hazzik · 2014-11-27T12:40:41Z

This is in SessionImpl, line 606

This is inside Find method which is private and used only from SessionImpl.Delete

hazzik · 2014-11-27T12:44:15Z

Ok, back to our sheeps... I managed to fix QueryOver by application of exactly same changes to MultipleQueriesCacheAssembler.Assemble, and it promoted error in hql to different part...

hazzik · 2014-11-27T12:52:05Z

I think that the best solution is split MultipleQueriesCacheAssembler to MultipleQueriesCacheAssemblerForMultiQuery and MultipleQueriesCacheAssemblerForCriteria

hazzik · 2014-11-27T12:56:48Z

Also, please add tests with Order-Details-Product domain, because else branch is not tested..

(This one)

else
{
    singleQueryCached.Add(TypeHelper.Disassemble(valuesToCache, assemblersToCache, null, session, null));
}

rjperes · 2014-11-27T13:10:43Z

"This is inside Find method which is private and used only from SessionImpl.Delete"
No, it isn't! While debugging the unit test, I land there!

rjperes · 2014-11-27T14:48:49Z

"This is inside Find method which is private and used only from SessionImpl.Delete"
You are right, my fault! Forget it.

fredericDelaporte · 2018-07-09T15:10:29Z

src/NHibernate/Impl/MultipleQueriesCacheAssembler.cs

-						singleQueryCached.Add(assemblers[0].Disassemble(objToCache, session, owner));
-					}
-					else
+					if (objToCache != null)


If #1783 is merged, this fix will be fixing an obsoleted class.

The future case is maybe already no more failing or failing in another way, since it does no more use multi-query/multi-criteria. (See #1718.)

fredericDelaporte · 2018-07-13T15:50:15Z

I have rebased this and amended tests for them to actually test caching then retrieving from cache. I have also rewritten them for using multi-query and multi-criteria, since Future does no more use it.

They fail, as expected by @hazzik.

I cannot push the rebase to show this, it seems "Allow edits from maintainers." is not enabled.

fredericDelaporte · 2018-07-17T09:19:43Z

The affected APIs are now obsoleted, and the replacing ones do not suffer of the bug.

rjperes added 2 commits November 25, 2014 18:09

Unit test for NH-3596

0bed7d9

Fix and unit test from NH-3737

a545396

rjperes closed this Nov 26, 2014

Improvements

0e201b2

Added HQL unit test

rjperes reopened this Nov 26, 2014

hazzik reviewed Nov 27, 2014
View reviewed changes

hazzik added the to review label Dec 2, 2014

hazzik force-pushed the master branch from e20411b to 9b7a5a3 Compare October 26, 2016 01:26

Merge branch 'master' into NH-3596

50cbfcc

hazzik assigned hazzik and unassigned hazzik Jul 25, 2017

hazzik mentioned this pull request Jun 14, 2018

[WIP] Cacheable Multicriteria/Future'd query with aliased join throw exception #1733

Closed

fredericDelaporte reviewed Jul 9, 2018

View reviewed changes

fredericDelaporte mentioned this pull request Jul 10, 2018

Combine Future queries for ICriteria and IQuery #1718

Merged

fredericDelaporte closed this Jul 17, 2018

fredericDelaporte added the r: Obsolete label Jul 17, 2018

NH-3596 - DistinctRootEntityResultTransformer breaks Cache #383

NH-3596 - DistinctRootEntityResultTransformer breaks Cache #383

Uh oh!

Conversation

rjperes commented Nov 26, 2014 • edited by fredericDelaporte Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hazzik commented Nov 27, 2014

Uh oh!

rjperes commented Nov 27, 2014

Uh oh!

hazzik commented Nov 27, 2014

Uh oh!

rjperes commented Nov 27, 2014

Uh oh!

hazzik commented Nov 27, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented Nov 27, 2014

Uh oh!

rjperes commented Nov 27, 2014

Uh oh!

hazzik commented Nov 27, 2014

Uh oh!

rjperes commented Nov 27, 2014

Uh oh!

rjperes commented Nov 27, 2014

Uh oh!

hazzik commented Nov 27, 2014

Uh oh!

rjperes commented Nov 27, 2014

Uh oh!

hazzik commented Nov 27, 2014

Uh oh!

hazzik commented Nov 27, 2014

Uh oh!

rjperes commented Nov 27, 2014

Uh oh!

hazzik commented Nov 27, 2014

Uh oh!

rjperes commented Nov 27, 2014

Uh oh!

hazzik commented Nov 27, 2014

Uh oh!

rjperes commented Nov 27, 2014

Uh oh!

hazzik commented Nov 27, 2014

Uh oh!

rjperes commented Nov 27, 2014

Uh oh!

hazzik commented Nov 27, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjperes commented Nov 27, 2014

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!

hazzik commented Nov 27, 2014

Uh oh!

hazzik commented Nov 27, 2014

Uh oh!

rjperes commented Nov 27, 2014

Uh oh!

hazzik commented Nov 27, 2014

Uh oh!

hazzik commented Nov 27, 2014

rjperes commented Nov 26, 2014 •

edited by fredericDelaporte

Loading

fredericDelaporte commented Jul 13, 2018 •

edited

Loading