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.

Sign up for GitHub

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

fix: FIR-33174: parse boolean in old (0/1) and new (false/true) formats #86

Merged
merged 1 commit into from
May 22, 2024
Merged

fix: FIR-33174: parse boolean in old (0/1) and new (false/true) formats #86

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion FireboltDotNetSdk.Tests/Unit/FireboltDataReaderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,10 +156,11 @@ public void Numbers()
new Meta() { Name = "real_number", Type = "text" },
new Meta() { Name = "int_number", Type = "text" },
new Meta() { Name = "bool_string", Type = "text" },
new Meta() { Name = "num_bool_string", Type = "text" },
},
Data = new List<List<object?>>()
{
new List<object?>() { byteValue, shortValue, intValue, longValue, floatValue, 3.1415926, true, "2.71828", "123", "true" }
new List<object?>() { byteValue, shortValue, intValue, longValue, floatValue, 3.1415926, true, "2.71828", "123", "true", "0" }
}
};

Expand Down Expand Up @@ -244,6 +245,13 @@ public void Numbers()
Assert.That(reader.GetBoolean(9), Is.EqualTo(true));
Assert.That(reader.GetChar(9), Is.EqualTo('t'));

Assert.That(reader.GetBoolean(10), Is.EqualTo(false));
Assert.That(reader.GetChar(10), Is.EqualTo('0'));
Assert.That(reader.GetByte(10), Is.EqualTo(0));
Assert.That(reader.GetInt16(10), Is.EqualTo(0));
Assert.That(reader.GetInt32(10), Is.EqualTo(0));
Assert.That(reader.GetInt64(10), Is.EqualTo(0));

Assert.That(reader.Read(), Is.EqualTo(false));

Assert.That(reader.IsClosed, Is.EqualTo(false));
Expand Down
31 changes: 23 additions & 8 deletions FireboltDotNetSdk.Tests/Unit/TypesConverterTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ namespace FireboltDotNetSdk.Tests;

public class TypesConverterTest
{
private const string WRONG_NUMERIC_FORMAT_ERROR_MESSAGE = "Input string was not in a correct format.";

[TestCase("string", "Hello", "Hello")]
[TestCase("string", null, null)]
[TestCase("text", "Hello", "Hello")]
Expand Down Expand Up @@ -46,25 +48,38 @@ public class TypesConverterTest
[TestCase("double", "-inf", double.NegativeInfinity)]
[TestCase("double", "nan", double.NaN)]
[TestCase("double", "-nan", double.NaN)]
[TestCase("boolean", "true", true)]
[TestCase("boolean", "false", false)]
[TestCase("boolean", "1", true)]
[TestCase("boolean", "0", false)]
public void ConvertProvidedValues(string columnTypeName, string value, object expectedValue)
{
ColumnType columnType = ColumnType.Of(columnTypeName);
object? result = TypesConverter.ConvertToCSharpVal(value, columnType);
Assert.That(expectedValue, Is.EqualTo(result));
}

[TestCase("int", "+inf")]
[TestCase("int", "-inf")]
[TestCase("int", "inf")]
[TestCase("long", "+inf")]
[TestCase("long", "-inf")]
[TestCase("long", "inf")]
public void FailingConvertProvidedValues(string columnTypeName, string value)
[TestCase("int", "+inf", WRONG_NUMERIC_FORMAT_ERROR_MESSAGE)]
[TestCase("int", "-inf", WRONG_NUMERIC_FORMAT_ERROR_MESSAGE)]
[TestCase("int", "inf", WRONG_NUMERIC_FORMAT_ERROR_MESSAGE)]
[TestCase("long", "+inf", WRONG_NUMERIC_FORMAT_ERROR_MESSAGE)]
[TestCase("long", "-inf", WRONG_NUMERIC_FORMAT_ERROR_MESSAGE)]
[TestCase("long", "inf", WRONG_NUMERIC_FORMAT_ERROR_MESSAGE)]
[TestCase("int", "hello", WRONG_NUMERIC_FORMAT_ERROR_MESSAGE)]
[TestCase("int", "bye", WRONG_NUMERIC_FORMAT_ERROR_MESSAGE)]
[TestCase("int", "not a number", WRONG_NUMERIC_FORMAT_ERROR_MESSAGE)]
[TestCase("long", "some text", WRONG_NUMERIC_FORMAT_ERROR_MESSAGE)]
[TestCase("long", "", WRONG_NUMERIC_FORMAT_ERROR_MESSAGE)]
[TestCase("long", "not empty text", WRONG_NUMERIC_FORMAT_ERROR_MESSAGE)]
[TestCase("boolean", "2", "String '2' was not recognized as a valid Boolean.")]
[TestCase("boolean", "t", "String 't' was not recognized as a valid Boolean.")]
[TestCase("boolean", "yes", "String 'yes' was not recognized as a valid Boolean.")]
public void FailingConvertProvidedValues(string columnTypeName, string value, string expectedErrorMessage)
{
ColumnType columnType = ColumnType.Of(columnTypeName);
FormatException? e = Assert.Throws<FormatException>(() => TypesConverter.ConvertToCSharpVal(value, columnType));
string visualTypeName = char.ToUpper(columnTypeName[0]) + columnTypeName.Substring(1);
Assert.That(e?.Message, Is.EqualTo($"Input string was not in a correct format."));
Assert.That(e?.Message, Is.EqualTo(expectedErrorMessage));
}

[Test]
Expand Down
2 changes: 1 addition & 1 deletion FireboltNETSDK/Client/FireboltDataReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ public override bool GetBoolean(int ordinal)
case short s: return s != 0;
case int i: return i != 0;
case long l: return l != 0;
case string s: return bool.Parse(s);
case string s: return TypesConverter.ParseBoolean(s);
default: throw new InvalidCastException($"Cannot cast ({value.GetType()}){value} to boolean");
}
}
Expand Down
19 changes: 18 additions & 1 deletion FireboltNETSDK/Utils/Types.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,17 @@ public static class TypesConverter
{
double.PositiveInfinity, double.NegativeInfinity, float.PositiveInfinity, float.NegativeInfinity
};
// when ParseBoolean is called from ConvertToCSharpVal that in turn invoked from FireboltDataReader.GetValueSafely() that calls ToString()
// to create string value representation. In case of boolean ToString() performs capitalization,
// so true becomes string True and false becomes False. However when ParseBoolean is invoked directly from GetBoolean()
// the value may be true (lower case). This is the reason to use case insensitive comparison.
private static IDictionary<string, bool> booleanValues = new Dictionary<string, bool>(StringComparer.OrdinalIgnoreCase)
{
{ "true", true },
{ "false", false },
{ "1", true },
{ "0", false },
};

public static object? ConvertToCSharpVal(string? val, ColumnType columnType)
{
Expand Down Expand Up @@ -68,7 +79,7 @@ public static class TypesConverter
case FireboltDataType.Float:
return floatInfinity.ContainsKey(str) ? floatInfinity[str] : Convert.ToSingle(str.Trim('"'), CultureInfo.InvariantCulture);
case FireboltDataType.Boolean:
return bool.Parse(str);
return ParseBoolean(str);
case FireboltDataType.Array:
return ArrayHelper.TransformToSqlArray(str, columnType);
case FireboltDataType.ByteA:
Expand All @@ -80,6 +91,12 @@ public static class TypesConverter
}
}

public static bool ParseBoolean(string str)
{
bool b;
return booleanValues.TryGetValue(str, out b) ? b : throw new FormatException($"String '{str}' was not recognized as a valid Boolean.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be a case-insensitive comparison? For cases like True and TRUE?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In fact it should support both true and True because string value arrives here after calling ToString() that turns true into string True and false into string False.

I agree that this is not obvious, so I can either add this text as a comment or add entries for True and False to the dictionary. What do you prefer?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. Eh, just a comment here is fine so we don't forget.

}

public static DateTime ParseDateTime(string str)
{
try
Expand Down
Loading