Skip to content

NH-3961 - Linq auto-parameterization failures #570

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 4 commits into from
Mar 19, 2017

Conversation

fredericDelaporte
Copy link
Member

Tests and fix for NH-3961: in some edge circumstances, the parameters parsed from an expression are not actually parameterized in the final resulting query.
This causes bugs as described in NH-3961, including queries ending up executed with parameter values of previous queries.

@@ -26,9 +27,11 @@ static NhRelinqQueryParser()
transformerRegistry.Register(new RemoveRedundantCast());
transformerRegistry.Register(new SimplifyCompareTransformer());

var processor = ExpressionTreeParser.CreateDefaultProcessor(transformerRegistry);
Copy link
Member Author

@fredericDelaporte fredericDelaporte Mar 17, 2017

Choose a reason for hiding this comment

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

This (current source) was creating a CompoundExpressionTreeProcessor containing a PartialEvaluatingExpressionTreeProcessor then a TransformingExpressionTreeProcessor.

But this parser is currently only used by NhLinqExpression, which has already processed the expression with a PartialEvaluatingExpressionVisitor (which the corresponding processor use itself without additional processing as of current sources).

This second evaluation was sometimes able of evaluating further some sub-trees, altering constants which were awaiting conversion to query parameters, and causing them to no more be converted to parameters. This was occurring with MappedAs on a non nullable constant compared to a nullable entity property. See NH-3961 comments for more details.

(Unfortunately, this change may be a possible breaking change for some external library, since NhRelinqQueryParser is public and could have other usages outside of NHibernate.)

@fredericDelaporte
Copy link
Member Author

fredericDelaporte commented Mar 17, 2017

I just realize this change could have an interesting "side effect": allowing non-runtime Linq extension to be used without referencing an entity. And I have tested it: yes it works!

With this PR, we could additionaly define and use something like:

public static class SqlServerFunction
{
	[LinqExtensionMethod]
	public static Guid NewID()
	{
		throw new InvalidOperationException("To be translated to SQL only");
	}
}

And use it:

var result = session.Query<Entity>()
	.OrderBy(e => SqlServerFunction.NewID())
	.ToList();

And it get executed as following SQL:

select entity0_.Id as Id1_0_, entity0_.Name as Name2_0_ from Entity entity0_ order by NewID() asc

That was not anticipated, but eliminating this second PartialEvaluatingExpressionTreeProcessor seems to add support for SQL side only function even without the function using an entity in its parameters.

But then I have to update the new Linq provider documentation ^^.

@hazzik
Copy link
Member

hazzik commented Mar 18, 2017

@fredericDelaporte it's described here: NH-3386. Can you also add test cases for it?

@fredericDelaporte
Copy link
Member Author

fredericDelaporte commented Mar 18, 2017

Test case for NH-3386 added.

Now I have seen we may filter out some evaluations by adding a filter deriving from EvaluatableExpressionFilterBase. This could allow supporting this feature without relying on exception being thrown, and silently swallowed by NH partial evaluation as it is currently.
I think I would add a parameter on LinqExtensionMethod such as DBSideOnly, defaulting to false. The filter would check it and also explicitly filter the MappedAs case. Partial evaluation exceptions would no more be swallowed.

But changing that in this PR is maybe not adequate. If done separately, it will then have to wait for this PR.

@hazzik
Copy link
Member

hazzik commented Mar 18, 2017 via email

@fredericDelaporte
Copy link
Member Author

fredericDelaporte commented Mar 18, 2017

You are right, I have seen it in 2.1 reference documentation and not checked with IlSpy it was there in 1.13.

Implementing it that way would have to wait for your PR for upgrading re-linq.

Otherwise we may define a specific exception to throw (such as NotImplementedException) and only swallow this one, to avoid swallowing unexpected troubles. That will still be better than current, but not as good than using filter in my opinion. I rather avoid having features implemented by throwing exception for their nominal, non error case.

As NH-3386 was never supported, it would be preferable to freeze its interface before release. If it gets released with this "any exception causes it to be silently evaluated db-side" behavior, changing that afterward for what I suggest would be a breaking change.

@hazzik
Copy link
Member

hazzik commented Mar 18, 2017

I did not check your request, but I suppose that removing a [LinqExtension] attribute from (or a custom generator for) a method will make the method throwing the exception at runtime, so, really it does not matter which exception to throw.

@hazzik
Copy link
Member

hazzik commented Mar 18, 2017

With the filter (in 2.1) we would be able to make a generator for Guid.NewGuid() (or any method which has an equivalent SQL form) which will be translated to proper SQL.

.OrderBy(e => SqlServerFunction.NewID());

Assert.DoesNotThrow(() => { result.ToList(); });
Assert.AreEqual(true,
Copy link
Member

Choose a reason for hiding this comment

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

Please update to NUnit fluent syntax: Assert.That(sqlInterceptor.Sql.ToString(), Does.Contain(nameof(SqlServerFunction.NewID)).IgnoreCase)

@fredericDelaporte
Copy link
Member Author

Done and squashed.

@fredericDelaporte
Copy link
Member Author

fredericDelaporte commented Mar 18, 2017

With the filter (in 2.1) we would be able to make a generator for Guid.NewGuid() (or any method which has an equivalent SQL form) which will be translated to proper SQL.

As EF 6 is doing, but I am not fond of that. DateTime.Now would have to choose for SQL Server between getdate, sysdatetime, current_timestamp. And having such expressions evaluated on db side while it was previously evaluated on app side could be a breaking change.
I would rather let NHibernate users choose explicitly which function to call on DB side.

I think something missing about functions is a static class like HqlFunctions supplying a way to call with Linq built-in HQL functions supported by dialects.

@fredericDelaporte
Copy link
Member Author

IgnoreCase forgotten, added and re-squashed. Test was still succeeding but I would rather avoid letting the test case being unnecessarily fragile.

@hazzik hazzik merged commit aba2943 into nhibernate:master Mar 19, 2017
@fredericDelaporte fredericDelaporte deleted the NH-3961 branch March 19, 2017 09:36
@hazzik hazzik modified the milestone: 5.0.0 Mar 24, 2017
@hazzik hazzik added the r: Fixed label Aug 3, 2017
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