Skip to content

Add special case for constants contained within Convert nodes (#2642) #2643

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 5 commits into from
Feb 7, 2021
Merged

Conversation

nkreipke
Copy link
Contributor

Test case and fix for issue #2642

  • Extracted AddConstantExpressionParameter method, added an extra parameter to "override" the value instead of always using expression.Value
  • Added a special case for a constant contained within a "Convert" node. Instead of using the constant value, evaluate the node so the conversion is performed correctly.

Tested with SQL Server and PostgreSQL.

@maca88
Copy link
Contributor

maca88 commented Feb 3, 2021

@nkreipke thanks for the PR, I added a small optimization and some additional tests.

// because it might be necessary if the types are incompatible with each other, which might happen if
// the expression uses an implicitly or explicitly defined cast operator.
if (node.NodeType == ExpressionType.Convert &&
node.Method != null && // The implicit/explicit operator method
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid compiling and evaluating the value for .NET built-in conversions.

@fredericDelaporte fredericDelaporte added this to the 5.3.6 milestone Feb 6, 2021
@fredericDelaporte fredericDelaporte changed the base branch from master to 5.3.x February 6, 2021 17:57
@fredericDelaporte fredericDelaporte merged commit d6a4859 into nhibernate:5.3.x Feb 7, 2021
bahusoid pushed a commit to bahusoid/nhibernate-core that referenced this pull request Aug 10, 2021
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.

3 participants