Skip to content
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

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

Closed

Conversation

igitur
Copy link
Contributor

@igitur igitur commented Jun 5, 2018

Original Jira post at https://nhibernate.jira.com/browse/NH-3864
Subsequent issue filed at #1344

I'm adding the unit tests that I found first. I don't have a fix yet, but I'm willing to have a bash at it.

@igitur igitur changed the title Cacheable Multicriteria/Future'd query with aliased join throw exception [WIP] Cacheable Multicriteria/Future'd query with aliased join throw exception Jun 5, 2018
@fredericDelaporte
Copy link
Member

Better take the habit of generating async code within each commit.

@igitur
Copy link
Contributor Author

igitur commented Jun 5, 2018

@fredericDelaporte I did and it failed. Parking that part until I finalise the tests.

@bahusoid
Copy link
Member

bahusoid commented Jun 5, 2018

If description is correct:

The bug actually lies is in MultipleQueriesCacheAssembler Disassemble method.

I believe with new batcher #1718 Future is not affected by this issue (as MultipleQueriesCacheAssembler is not used - cache is created per each query)

@fredericDelaporte
Copy link
Member

Maybe the regen fails due to maca88/AsyncGenerator#70. You would have to try with methods instead of delegate parameters.

@bahusoid, interesting indeed. But of course better checking that with appropriate tests, so having them thanks to this PR ist still a good thing. Maybe you could then cherry-pick them in #1718 for ascertaining it does also fix #1344.

@igitur
Copy link
Contributor Author

igitur commented Jun 5, 2018

@bahusoid That's good news. I ran the tests against your branch and 3 of the 5 tests still fail. I can push a PR to your repo to show you what I mean?

There still a problem with QueryLoader.GetResultList at the var row = (Object[]) results[i]; line. System.InvalidCastException : Unable to cast object of type 'NHibernate.Test.NHSpecificTest.NH3864.Person' to type 'System.Object[]'.

And the CacheableMulticriteria_QueryOverWithAliasedJoinQueryOver test still uses MultipleQueriesCacheAssembler (even on your branch).

@bahusoid
Copy link
Member

bahusoid commented Jun 5, 2018

New batcher is used only with Future methods.

MultiCriteria and MultiQuery are separate batchers. If you need MulitCriteria or MultiQuery - you still need to fix it.

@hazzik
Copy link
Member

hazzik commented Jun 14, 2018

Is #383 the same?

@igitur
Copy link
Contributor Author

igitur commented Jun 18, 2018

Is #383 the same?

Not sure if this question is directed at me. I'm not comfortable enough to answer it convincingly.

@hazzik
Copy link
Member

hazzik commented Jun 18, 2018

@igitur its to whomever capable to answer this question.

@igitur igitur force-pushed the NH3864-Cacheable-Multicriteria branch from 85a4cef to 0de7af4 Compare June 25, 2018 14:15
@igitur
Copy link
Contributor Author

igitur commented Jul 11, 2018

@maca88 I see you've been commenting on related PRs. Just, FYI, my goal for this PR was to enable caching when using .Include() methods from your NHibernate.Extensions project.

@fredericDelaporte
Copy link
Member

There still a problem with QueryLoader.GetResultList at the var row = (Object[]) results[i]; line. System.InvalidCastException : Unable to cast object of type 'NHibernate.Test.NHSpecificTest.NH3864.Person' to type 'System.Object[]'.

This is a bug in query batch implementation indeed. It should be fixed with #1789.

And the CacheableMulticriteria_QueryOverWithAliasedJoinQueryOver test still uses MultipleQueriesCacheAssembler (even on your branch).

This would change with #1783, by obsoleting Multi-Criteria/Query APIs in favor of using the query batcher. Those tests should be converted for using the query batcher.

Is #383 the same?

They could have the same root cause.

owerkop and others added 2 commits July 12, 2018 10:41
# Conflicts:
#	src/NHibernate.Test/NHibernate.Test.csproj
@igitur igitur force-pushed the NH3864-Cacheable-Multicriteria branch from 0de7af4 to 5f3efb7 Compare July 12, 2018 08:41
@fredericDelaporte
Copy link
Member

With #1783, the flawed class is now obsoleted.

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