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

Query: Read Avg/Min/Max as nullable from database to throw exception … #18971

Merged
merged 2 commits into from
Nov 26, 2019

Conversation

smitpatel
Copy link
Contributor

…only when it is empty

Resolves #18955

@smitpatel
Copy link
Contributor Author

Description
Since 3.0, we read value from database according to the type. In Avg/Min/Max database returns null if there are no elements. We read this as default value of non-nullable type and throw exception if we hit default. If the actual result of above methods ends up being same as default value, we incorrect identify it is empty sequence.

Customer Impact
Query failure when result of Avg/Min/Max in database is 0.

How found
Customer-reported (#18955)

Test coverage
We did not have tests when Avg/Min/Max returned 0 result.

Regression?
Yes, from 2.2.

Risk
Risk is very small - the fix is reading database value as nullable for those functions and disambiguate null from 0 in result to throw exception only in correct case.

@ajcvickers ajcvickers added this to the 3.1.x milestone Nov 19, 2019
@ajcvickers
Copy link
Contributor

@Pilchie FYI, we would like to patch this in 3.1.1.

@Pilchie
Copy link
Member

Pilchie commented Nov 19, 2019

What workaround is available to the customer, if any?

@ajcvickers
Copy link
Contributor

ajcvickers commented Nov 19, 2019

@Pilchie Having a workaround may not help much in this case. The issue is that if you are calculating the max of a set of values, then your application may work fine until the result happens to be zero at which point the app will crash. To use a workaround you would have to know that the issue exists, which is very easily missed here, even with reasonable testing.

@@ -55,6 +56,10 @@ protected override RelationalTypeMapping Clone(RelationalTypeMappingParameters p
/// The generated string.
/// </returns>
protected override string GenerateNonNullSqlLiteral(object value)
=> ((float)value).ToString("R", CultureInfo.InvariantCulture);
{
var floatValue = Convert.ToSingle(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

keep as lambda body

@ajcvickers
Copy link
Contributor

@Pilchie @wtgodbe @dougbu Can this now be merged for 3.1.1?

@wtgodbe wtgodbe merged commit 2da12e7 into release/3.1 Nov 26, 2019
@wtgodbe wtgodbe deleted the smit/issue18955 branch November 26, 2019 19:12
@ajcvickers ajcvickers removed this from the 3.1.1 milestone Nov 26, 2019
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.

6 participants