Skip to content

Bugfix for culture invariant decimal types #423

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
Dec 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,7 @@ We'd love your contributions! If you want to contribute please read our [Contrib
* [@AndersenGans](https://github.com/AndersenGans)
* [@mdrakib](https://github.com/mdrakib)
* [@jrpavoncello](https://github.com/jrpavoncello)
* [@axnetg](https://github.com/axnetg)

<!-- Logo -->
[Logo]: images/logo.svg
Expand Down
36 changes: 18 additions & 18 deletions src/Redis.OM/Aggregation/AggregationPredicates/QueryPredicate.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,32 +57,32 @@ protected override void ValidateAndPushOperand(Expression expression, Stack<stri

if (binaryExpression.Right is ConstantExpression constantExpression)
{
stack.Push(BuildQueryPredicate(binaryExpression.NodeType, memberExpression, constantExpression));
var constVal = ExpressionParserUtilities.GetOperandString(constantExpression);
stack.Push(BuildQueryPredicate(binaryExpression.NodeType, memberExpression, constVal));
}
else if (binaryExpression.Right is UnaryExpression uni)
{
switch (uni.Operand)
{
case ConstantExpression c:
stack.Push(BuildQueryPredicate(binaryExpression.NodeType, memberExpression, c));
var constVal = ExpressionParserUtilities.GetOperandString(c);
stack.Push(BuildQueryPredicate(binaryExpression.NodeType, memberExpression, constVal));
break;
case MemberExpression mem:
{
var val = ExpressionParserUtilities.GetOperandString(mem);
stack.Push(BuildQueryPredicate(binaryExpression.NodeType, memberExpression, System.Linq.Expressions.Expression.Constant(val)));
stack.Push(BuildQueryPredicate(binaryExpression.NodeType, memberExpression, val));
break;
}
}
}
else if (binaryExpression.Right is MemberExpression mem)
{
var val = ExpressionParserUtilities.GetOperandString(mem);
stack.Push(BuildQueryPredicate(binaryExpression.NodeType, memberExpression, System.Linq.Expressions.Expression.Constant(val)));
stack.Push(BuildQueryPredicate(binaryExpression.NodeType, memberExpression, val));
}
else
{
var val = ExpressionParserUtilities.GetOperandStringForQueryArgs(binaryExpression.Right, new List<object>()); // hack - will need to revisit when integrating vectors into aggregations.
stack.Push(BuildQueryPredicate(binaryExpression.NodeType, memberExpression, System.Linq.Expressions.Expression.Constant(val)));
stack.Push(BuildQueryPredicate(binaryExpression.NodeType, memberExpression, val));
}
}
else if (expression is ConstantExpression c
Expand Down Expand Up @@ -169,7 +169,7 @@ protected override void SplitBinaryExpression(BinaryExpression expression, Stack
}
}

private static string BuildEqualityPredicate(MemberInfo member, ConstantExpression expression, string memberStr, bool negated = false)
private static string BuildEqualityPredicate(MemberInfo member, string queryValue, string memberStr, bool negated = false)
{
var sb = new StringBuilder();
var fieldAttribute = member.GetCustomAttribute<SearchFieldAttribute>();
Expand All @@ -190,13 +190,13 @@ private static string BuildEqualityPredicate(MemberInfo member, ConstantExpressi
switch (searchFieldType)
{
case SearchFieldType.TAG:
sb.Append($"{{{ExpressionParserUtilities.EscapeTagField(expression.Value.ToString())}}}");
sb.Append($"{{{ExpressionParserUtilities.EscapeTagField(queryValue)}}}");
break;
case SearchFieldType.TEXT:
sb.Append(expression.Value);
sb.Append(queryValue);
break;
case SearchFieldType.NUMERIC:
sb.Append($"[{expression.Value} {expression.Value}]");
sb.Append($"[{queryValue} {queryValue}]");
break;
default:
throw new InvalidOperationException("Could not translate query equality searches only supported for Tag, text, and numeric fields");
Expand All @@ -205,17 +205,17 @@ private static string BuildEqualityPredicate(MemberInfo member, ConstantExpressi
return sb.ToString();
}

private string BuildQueryPredicate(ExpressionType expType, MemberExpression member, ConstantExpression constExpression)
private string BuildQueryPredicate(ExpressionType expType, MemberExpression member, string queryValue)
{
var memberStr = ExpressionParserUtilities.GetOperandString(member);
var queryPredicate = expType switch
{
ExpressionType.GreaterThan => $"{memberStr}:[({constExpression.Value} inf]",
ExpressionType.LessThan => $"{memberStr}:[-inf ({constExpression.Value}]",
ExpressionType.GreaterThanOrEqual => $"{memberStr}:[{constExpression.Value} inf]",
ExpressionType.LessThanOrEqual => $"{memberStr}:[-inf {constExpression.Value}]",
ExpressionType.Equal => BuildEqualityPredicate(member.Member, constExpression, memberStr),
ExpressionType.NotEqual => BuildEqualityPredicate(member.Member, constExpression, memberStr, true),
ExpressionType.GreaterThan => $"{memberStr}:[({queryValue} inf]",
ExpressionType.LessThan => $"{memberStr}:[-inf ({queryValue}]",
ExpressionType.GreaterThanOrEqual => $"{memberStr}:[{queryValue} inf]",
ExpressionType.LessThanOrEqual => $"{memberStr}:[-inf {queryValue}]",
ExpressionType.Equal => BuildEqualityPredicate(member.Member, queryValue, memberStr),
ExpressionType.NotEqual => BuildEqualityPredicate(member.Member, queryValue, memberStr, true),
_ => string.Empty
};
return queryPredicate;
Expand Down
12 changes: 11 additions & 1 deletion src/Redis.OM/Common/ExpressionParserUtilities.cs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ internal static string GetOperandStringForQueryArgs(Expression exp, List<object>
{
var res = exp switch
{
ConstantExpression constExp => $"{constExp.Value}",
ConstantExpression constExp => ValueToString(constExp.Value),
MemberExpression member => GetOperandStringForMember(member, treatEnumsAsInt),
MethodCallExpression method => TranslateMethodStandardQuerySyntax(method, parameters),
UnaryExpression unary => GetOperandStringForQueryArgs(unary.Operand, parameters, treatEnumsAsInt, unary.NodeType == ExpressionType.Not),
Expand Down Expand Up @@ -960,6 +960,16 @@ private static string ValueToString(object value)
return ((double)value).ToString(CultureInfo.InvariantCulture);
}

if (valueType == typeof(float) || Nullable.GetUnderlyingType(valueType) == typeof(float))
{
return ((float)value).ToString(CultureInfo.InvariantCulture);
}

if (valueType == typeof(decimal) || Nullable.GetUnderlyingType(valueType) == typeof(decimal))
{
return ((decimal)value).ToString(CultureInfo.InvariantCulture);
}

if (value is DateTimeOffset dto)
{
return dto.ToUnixTimeMilliseconds().ToString(CultureInfo.InvariantCulture);
Expand Down
3 changes: 2 additions & 1 deletion src/Redis.OM/Modeling/JsonDiff.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
using System;
using System.Linq;
using System.Globalization;
using System.Text.Json;
using System.Web;
using Newtonsoft.Json.Linq;
Expand Down Expand Up @@ -37,6 +37,7 @@ public string[] SerializeScriptArgs()
return _value.Type switch
{
JTokenType.String => new[] { _operation, _path, $"\"{HttpUtility.JavaScriptStringEncode(_value.ToString())}\"" },
JTokenType.Float => new[] { _operation, _path, ((JValue)_value).ToString(CultureInfo.InvariantCulture) },
JTokenType.Boolean => new[] { _operation, _path, _value.ToString().ToLower() },
JTokenType.Date => SerializeAsDateTime(),
_ => new[] { _operation, _path, _value.ToString() }
Expand Down
2 changes: 1 addition & 1 deletion src/Redis.OM/RedisObjectHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ internal static IDictionary<string, object> BuildHashSet(this object obj)
var val = property.GetValue(obj);
if (val != null)
{
hash.Add(propertyName, val.ToString());
hash.Add(propertyName, Convert.ToString(val, CultureInfo.InvariantCulture));
}
}
else if (type.IsEnum)
Expand Down
30 changes: 30 additions & 0 deletions test/Redis.OM.Unit.Tests/RediSearchTests/AggregationSetTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -587,5 +587,35 @@ public void DateTimeQuery()
_ = collection.Where(query).ToList();
_substitute.Received().Execute("FT.AGGREGATE", "objectwithdatetime-idx", $"@TimestampOffset:[({dtMs} inf]", "WITHCURSOR", "COUNT", "10000");
}

[Fact]
public void TestDecimalQuery()
{
var collection = new RedisAggregationSet<Person>(_substitute, true, chunkSize: 10000);
_substitute.Execute("FT.AGGREGATE", Arg.Any<object[]>()).Returns(MockedResult);
_substitute.Execute("FT.CURSOR", Arg.Any<object[]>()).Returns(MockedResultCursorEnd);

var y = 30.55M;
Expression<Func<AggregationResult<Person>, bool>> query = a => a.RecordShell!.Salary > y;
_ = collection.Where(query).ToList();
_substitute.Received().Execute("FT.AGGREGATE", "person-idx", "@Salary:[(30.55 inf]", "WITHCURSOR", "COUNT", "10000");

query = a => a.RecordShell!.Salary > 85.99M;
_ = collection.Where(query).ToList();
_substitute.Received().Execute("FT.AGGREGATE", "person-idx", "@Salary:[(85.99 inf]", "WITHCURSOR", "COUNT", "10000");

query = a => a.RecordShell!.Salary == 70.5M;
_ = collection.Where(query).ToList();
_substitute.Received().Execute("FT.AGGREGATE", "person-idx", "@Salary:[70.5 70.5]", "WITHCURSOR", "COUNT", "10000");
}

[Theory]
[InlineData("en-DE")]
[InlineData("it-IT")]
[InlineData("es-ES")]
public void TestDecimalQueryTestingInvariantCultureCompliance(string lcid)
{
Helper.RunTestUnderDifferentCulture(lcid, _ => TestDecimalQuery());
}
}
}
3 changes: 3 additions & 0 deletions test/Redis.OM.Unit.Tests/RediSearchTests/Person.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ public partial class Person
[Indexed(Sortable = true)]
public double? Height { get; set; }

[Indexed(Sortable = true)]
public decimal? Salary { get; set; }

[ListType]
[Indexed]
public string[] NickNames { get; set; }
Expand Down
49 changes: 48 additions & 1 deletion test/Redis.OM.Unit.Tests/RediSearchTests/SearchTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ public void TestFirstOrDefaultWithMixedLocals()
}

[Fact]
public void TestBasicQueryWithExactNumericMatch()
public void TestBasicQueryWithExactIntegerMatch()
{
_substitute.ClearSubstitute();
_substitute.Execute(Arg.Any<string>(), Arg.Any<object[]>()).Returns(_mockReply);
Expand All @@ -135,6 +135,32 @@ public void TestBasicQueryWithExactNumericMatch()
"100");
}

[Fact]
public void TestBasicQueryWithExactDecimalMatch()
{
_substitute.ClearSubstitute();
_substitute.Execute(Arg.Any<string>(), Arg.Any<object[]>()).Returns(_mockReply);
var y = 90.5M;
var collection = new RedisCollection<Person>(_substitute);
_ = collection.Where(x => x.Salary == y).ToList();
_substitute.Received().Execute(
"FT.SEARCH",
"person-idx",
"(@Salary:[90.5 90.5])",
"LIMIT",
"0",
"100");
}

[Theory]
[InlineData("en-DE")]
[InlineData("it-IT")]
[InlineData("es-ES")]
public void TestBasicQueryWithExactDecimalMatchTestingInvariantCultureCompliance(string lcid)
{
Helper.RunTestUnderDifferentCulture(lcid, _ => TestBasicQueryWithExactDecimalMatch());
}

[Fact]
public void TestBasicFirstOrDefaultQuery()
{
Expand Down Expand Up @@ -1057,6 +1083,15 @@ public async Task TestUpdateJsonWithDouble()
Scripts.ShaCollection.Clear();
}

[Theory]
[InlineData("en-DE")]
[InlineData("it-IT")]
[InlineData("es-ES")]
public void TestUpdateJsonWithDoubleTestingInvariantCultureCompliance(string lcid)
{
Helper.RunTestUnderDifferentCulture(lcid, async _ => await TestUpdateJsonWithDouble());
}

[Fact]
public async Task TestDeleteAsync()
{
Expand Down Expand Up @@ -3346,6 +3381,7 @@ public void NonNullableNumericFieldContains()
var doubles = new [] { 22.5, 23, 24 };
var floats = new [] { 25.5F, 26, 27 };
var ushorts = new ushort[] { 28, 29, 30 };
var decimals = new decimal[] { 31.5M, 32, 33 };
_substitute.ClearSubstitute();
_substitute.Execute(Arg.Any<string>(), Arg.Any<object[]>()).Returns(_mockReply);
var collection = new RedisCollection<ObjectWithNumerics>(_substitute).Where(x => ints.Contains(x.Integer));
Expand Down Expand Up @@ -3457,6 +3493,17 @@ public void NonNullableNumericFieldContains()
"LIMIT",
"0",
"100");

collection = new RedisCollection<ObjectWithNumerics>(_substitute).Where(x => decimals.Contains(x.Decimal));
_ = collection.ToList();
expected = $"@{nameof(ObjectWithNumerics.Decimal)}:[31.5 31.5]|@{nameof(ObjectWithNumerics.Decimal)}:[32 32]|@{nameof(ObjectWithNumerics.Decimal)}:[33 33]";
_substitute.Received().Execute(
"FT.SEARCH",
"objectwithnumerics-idx",
expected,
"LIMIT",
"0",
"100");
}

[Fact]
Expand Down
2 changes: 2 additions & 0 deletions test/Redis.OM.Unit.Tests/Serialization/ObjectWithNumerics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,6 @@ public class ObjectWithNumerics
public double Double { get; set; }
[Indexed]
public float Float { get; set; }
[Indexed]
public decimal Decimal { get; set; }
}