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

Allow custom query loader #3209

Merged

Conversation

micmerchant
Copy link
Contributor

Hello,

I'm currently working on a NHibernate-Linq plugin to support MSSQL Table-Valued-Functions, which I plan to release as open source software.

My entry points are the following ones:
<property name="query.linq_provider_class"/>
<property name="query.factory_class"/>

The plugin propagates the Table-Value parameters down to a custom query loader via the following steps

  1. inherited DefaultQueryProvider implementation to override the PrepareQuery method
  2. decorated NhLinqExpression to keep track of
  • ParameterDescriptors
  • ParameterValuesByName
  • NamedParameters
    and to pass the query/parameter verification checks.
  1. custom query translator factory to create a special query translator
  2. expand the ParameterMetaData by Table-Valued parameters via decorated BuildParameterMetadata method of query translator
  3. inherited QueryLoader implementation to expand parameters by Table-Valued parameters via overridden CreateSqlCommand method. Expanding the parameters works quite similar like the session filter parameter implementation.

But to get this working and not to use reflection, I need some changes to support Dependency Inversion in the following areas:

  1. A custom query loader must be injectable into the query translator. This requires the following changes:
  • change some access modifiers from internal to public
  • extracted ILoader and IQueryLoader interfaces
  • QueryTranslatorImpl constructor extended by a factory method to create an IQueryLoader on demand
  1. To support query plan caching and proper copying of query plan/expression also for Table-Valued Function queries, the following changes are necessary:
  • NhLinqExpression implements ILinqQueryExpression, which extends IQueryExpression, ICacheableQueryExpression
  • checks changed from NhLinqExpression to ILinqQueryExpression in QueryPlanCache::PreparePlanToCache, PreparePlanToCache::CopyIfRequired
  • CopyExpressionTranslation public accessible via ILinqQueryExpression now
  • NhLinqExpressionCache depends on ILinqQueryExpression now instead of NhLinqExpression
  • due to interface extraction the access modifier of some other classes/interfaces had to be changed to public

Basically, the functionality of NHibernate has not changed at all. Some classes depend on abstractions now instead on concrete implementations.

What do you think about these changes?

BR,
Michael

Verified

This commit was signed with the committer’s verified signature. The key has expired.
antonpirker Anton Pirker
- IQueryLoader
- ILoader
- ILinqQueryExpression
Copy link
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.

Tests for the change should be added: check that providing a custom query loader factory works as expected, for all querying interface NHibernate has. I suspect it may break where I have asked about a possible missing type change.

Also, the possibility of supplying a custom query loader factory should likely be surfaced to the NHibernate configuration, in a similar way to what we do for most other factories (see linqtohql.generatorsregistry, collectiontype.factory_class, query.factory_class, ...).

Michael Kaufmann and others added 4 commits December 11, 2022 23:51

Verified

This commit was signed with the committer’s verified signature. The key has expired.
antonpirker Anton Pirker
…erted

Verified

This commit was signed with the committer’s verified signature. The key has expired.
antonpirker Anton Pirker

Verified

This commit was signed with the committer’s verified signature. The key has expired.
antonpirker Anton Pirker
@micmerchant
Copy link
Contributor Author

Tests for the change should be added: check that providing a custom query loader factory works as expected, for all querying interface NHibernate has. I suspect it may break where I have asked about a possible missing type change.

Also, the possibility of supplying a custom query loader factory should likely be surfaced to the NHibernate configuration, in a similar way to what we do for most other factories (see linqtohql.generatorsregistry, collectiontype.factory_class, query.factory_class, ...).

  1. I'll add some tests.
  2. Yes, you are right, the query loader could be made configurable. But this would also require a "major" documentation update.

Michael Kaufmann added 3 commits December 16, 2022 09:43
- QueryLoaderFactory added
- IQueryTranslator only publishes the Loader property of type Loader.Loader again
- Type of Loader property of ITranslator is of type Loader.Loader again
- ICacheableQueryExpression is only internal again
- additional constructors added to QueryTranslatorImpl, QueryInfo to ensure backwards compatibility
- QueryCacheResultBuilder method restored to ensure backwards compatibility
@micmerchant
Copy link
Contributor Author

I've added tests that should use a custom QueryTranslatorFactory in combination with a custom QueryLoader for Criteria, HQL, Linq and QueryOver queries. I've not added a NHibernate config option for custom QueryLoaders.

But, it seems that Criteria and QueryOver queries don't depend on a QueryTranslatorFactory, so these two TestCases are kinda useless. Only Linq and HQL queries instantiate a QueryTranslatorFactory.

@fredericDelaporte
Copy link
Member

fredericDelaporte commented Jan 1, 2023

I have pushed the commit I had written about.

I got by the way a better view of what was actually needed. So, about some of my previous comments:

Also, the possibility of supplying a custom query loader factory should likely be surfaced to the NHibernate configuration, in a similar way to what we do for most other factories (see linqtohql.generatorsregistry, collectiontype.factory_class, query.factory_class, ...).

It is indeed not needed, the current translator factory is enough.

For all possible querying interface

Indeed a bit broad since that change does apply neither to Criteria nor QueryOver (nor native queries).

@micmerchant
Copy link
Contributor Author

I have pushed the commit I had written about.

I got by the way a better view of what was actually needed. So, about some of my previous comments:

Also, the possibility of supplying a custom query loader factory should likely be surfaced to the NHibernate configuration, in a similar way to what we do for most other factories (see linqtohql.generatorsregistry, collectiontype.factory_class, query.factory_class, ...).

It is indeed not needed, the current translator factory is enough.

For all possible querying interface

Indeed a bit broad since that change does apply neither to Criteria nor QueryOver (nor native queries).

I'm glad that you got a better view on it and I thankful for your commit. I'll still add the FutureQuery-Test to ensure that there it is no breaking change.

micmerchant and others added 4 commits January 3, 2023 10:19
Co-authored-by: Frédéric Delaporte <12201973+fredericDelaporte@users.noreply.github.com>
Co-authored-by: Frédéric Delaporte <12201973+fredericDelaporte@users.noreply.github.com>
- HqlTranslatorWrapper for future queries implements the ITranslatorWithCustomizableLoader now
- documentation added for test classes
@micmerchant
Copy link
Contributor Author

I think HqlTranslatorWrapper needs to implement ITranslatorWithCustomizableLoader to use the QueryLoader property. Now the new future query test runs also successfully.

	internal class HqlTranslatorWrapper : ITranslatorWithCustomizableLoader
	{
		/// <inheritdoc />
		public ILoader QueryLoader => innerTranslator.GetQueryLoader();

Copy link
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.

I am going to adjust these additional points.

@fredericDelaporte fredericDelaporte added this to the 5.5 milestone Jan 8, 2023
@micmerchant
Copy link
Contributor Author

micmerchant commented Jan 13, 2023

I am going to adjust these additional points.

👍

I can recommend this NuGet package for more verbose assertions FluentAssertions.

@micmerchant
Copy link
Contributor Author

Just for information, you can find the plugin to support MSSQL Table-Valued Functions with Linq here:
MSSQLTableValuedFunctions.NHibernate.Linq

@fredericDelaporte fredericDelaporte changed the title Dependency Inversion Principle - QueryLoader, NhLinqExpression Allow custom query loader Jan 28, 2023
@fredericDelaporte fredericDelaporte merged commit ab5eb9d into nhibernate:master Jan 28, 2023
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.

None yet

2 participants