Skip to content

NH-3848 - Tests and fix for HQL#463

Merged
fredericDelaporte merged 19 commits into
nhibernate:masterfrom
itmagination:NH-3848_HQL
Feb 6, 2019
Merged

NH-3848 - Tests and fix for HQL#463
fredericDelaporte merged 19 commits into
nhibernate:masterfrom
itmagination:NH-3848_HQL

Conversation

@zalewskil
Copy link
Copy Markdown

@zalewskil zalewskil commented Feb 24, 2016

This pull should be treated as a hint for fix or tests rather than actual fix (but maybe I'm wrong).
It can harm performance (traversing Where clause tree or using String.StartsWith).
Actually we aren't using HQL in our application so this fix had to be done quick.

NH-3848.

Fixes #1341

@hazzik
Copy link
Copy Markdown
Member

hazzik commented Nov 23, 2018

Rebased to resolve conflicts

@hazzik

This comment has been minimized.

}
}

public bool CanAddFetchedCollectionToCache
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This probably needs to be moved to a loader, however, I'm not entirely sure.

Copy link
Copy Markdown
Member

@fredericDelaporte fredericDelaporte Nov 24, 2018

Choose a reason for hiding this comment

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

Shouldn't it be CanAddFetchedCollectionsToCache? (Collection is switched to plural.)

If I understand well this PR, contrary to #460, it would disable collection caching for the whole query, not just for the collection having restrictions.

This probably needs to be moved to a loader, however, I'm not entirely sure.

I am not sure either. It may require to expose some internal state of the translator for the loader to analyse it, and then, better keep it in the translator.

@hazzik hazzik changed the title NH-3848 - Tests and fix for HQL WIP - NH-3848 - Tests and fix for HQL Nov 23, 2018
}
}

public bool CanAddFetchedCollectionToCache
Copy link
Copy Markdown
Member

@fredericDelaporte fredericDelaporte Nov 24, 2018

Choose a reason for hiding this comment

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

Shouldn't it be CanAddFetchedCollectionsToCache? (Collection is switched to plural.)

If I understand well this PR, contrary to #460, it would disable collection caching for the whole query, not just for the collection having restrictions.

This probably needs to be moved to a loader, however, I'm not entirely sure.

I am not sure either. It may require to expose some internal state of the translator for the loader to analyse it, and then, better keep it in the translator.

Comment thread src/NHibernate.Test/NHSpecificTest/NH3848/HqlTestFixture.cs Outdated
Comment thread src/NHibernate.Test/NHSpecificTest/NH3848/TestFixture.cs Outdated
Comment thread src/NHibernate/Hql/Ast/ANTLR/QueryTranslatorImpl.cs Outdated
Comment thread src/NHibernate.Test/NHSpecificTest/NH3848/TestFixture.cs Outdated
Comment thread src/NHibernate/Hql/Ast/ANTLR/QueryTranslatorImpl.cs Outdated
fredericDelaporte and others added 3 commits November 24, 2018 17:07
Also add transactions since using second level cache without
transactions is a bad practice.
Copy link
Copy Markdown
Member

@fredericDelaporte fredericDelaporte left a comment

Choose a reason for hiding this comment

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

@bahusoid change is a good improvement in my opinion.

Comment thread src/NHibernate/Hql/Ast/ANTLR/QueryTranslatorImpl.cs Outdated
Comment thread src/NHibernate/Hql/Ast/ANTLR/QueryTranslatorImpl.cs Outdated
Comment thread src/NHibernate/Hql/Ast/ANTLR/Util/ASTUtil.cs
bahusoid
bahusoid previously approved these changes Dec 29, 2018
Copy link
Copy Markdown
Member

@bahusoid bahusoid left a comment

Choose a reason for hiding this comment

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

Why WIP?

@hazzik hazzik changed the title WIP - NH-3848 - Tests and fix for HQL NH-3848 - Tests and fix for HQL Jan 14, 2019
@bahusoid
Copy link
Copy Markdown
Member

bahusoid commented Feb 4, 2019

But it seems to fully fix NH-3848 filter case (when collection is filtered by session.EnableFilter) is also needs to be handled (both in criteria and hql)

@fredericDelaporte
Copy link
Copy Markdown
Member

But it seems to fully fix NH-3848 filter case (when collection is filtered by session.EnableFilter) is also needs to be handled (both in criteria and hql).

Let's call it another bug, if confirmed.

@fredericDelaporte fredericDelaporte merged commit ac55892 into nhibernate:master Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

5 participants