Skip to content

Add a missing short circuit in query parameter expansion. #1548

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

Conversation

fredericDelaporte
Copy link
Member

  • This should have been the fix for NH-3050.

As explained in #1547 first commit rationals, queries having a list parameter need some additional processing once its value is known. This parameter (having a list value) may need to be removed in case of empty list, or replaced by a list of parameters representing one value of the list each.

But when the list has only one value, the parameter is not replaced but just retyped to the list element type, with its value changed from the list to the single value. In such case the query is not changed.

This was already rightfully short-circuited for AbstractQueryImpl, by yielding back the same query string. But for ExpressionQueryImpl (used by LINQ queries), a new IQueryExpression was computed anyway, while being in fine identical to the original one. (Its type excepted, leading to NH-3050: you can check its test cases, they use on purpose a single value list parameter, otherwise they could not reproduce the trouble.)

So here an additional short-circuit has been added to just yield the original query in this case. (NH-3050 test cases do indeed test this change by the way.)

NH-3050 original solution could be reverted with this change, but on its principles its solution stays also valid (it does include query expression type in the plan key). So better leave it in place.

 * This should have been the fix for NH-3050.
@fredericDelaporte fredericDelaporte merged commit 89034e6 into nhibernate:master Jan 22, 2018
@fredericDelaporte fredericDelaporte deleted the ParameterExpansion branch January 22, 2018 09:12
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