From d43d4b260d5ec0eb58e4c34f4659cb891dce9cb4 Mon Sep 17 00:00:00 2001 From: Alex <57764940+axnetg@users.noreply.github.com> Date: Wed, 6 Dec 2023 20:13:21 +0100 Subject: [PATCH] Bugfix for culture invariant decimal types (#423) --- README.md | 1 + .../AggregationPredicates/QueryPredicate.cs | 36 +++++++------- .../Common/ExpressionParserUtilities.cs | 12 ++++- src/Redis.OM/Modeling/JsonDiff.cs | 3 +- src/Redis.OM/RedisObjectHandler.cs | 2 +- .../RediSearchTests/AggregationSetTests.cs | 30 ++++++++++++ .../RediSearchTests/Person.cs | 3 ++ .../RediSearchTests/SearchTests.cs | 49 ++++++++++++++++++- .../Serialization/ObjectWithNumerics.cs | 2 + 9 files changed, 116 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 97ff9dc3..6be266c6 100644 --- a/README.md +++ b/README.md @@ -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]: images/logo.svg diff --git a/src/Redis.OM/Aggregation/AggregationPredicates/QueryPredicate.cs b/src/Redis.OM/Aggregation/AggregationPredicates/QueryPredicate.cs index 87263bf8..c6f30f02 100644 --- a/src/Redis.OM/Aggregation/AggregationPredicates/QueryPredicate.cs +++ b/src/Redis.OM/Aggregation/AggregationPredicates/QueryPredicate.cs @@ -57,32 +57,32 @@ protected override void ValidateAndPushOperand(Expression expression, Stack()); // 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 @@ -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(); @@ -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"); @@ -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; diff --git a/src/Redis.OM/Common/ExpressionParserUtilities.cs b/src/Redis.OM/Common/ExpressionParserUtilities.cs index c2515611..a56e5b43 100644 --- a/src/Redis.OM/Common/ExpressionParserUtilities.cs +++ b/src/Redis.OM/Common/ExpressionParserUtilities.cs @@ -83,7 +83,7 @@ internal static string GetOperandStringForQueryArgs(Expression exp, List { 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), @@ -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); diff --git a/src/Redis.OM/Modeling/JsonDiff.cs b/src/Redis.OM/Modeling/JsonDiff.cs index 39949241..2dbb10a6 100644 --- a/src/Redis.OM/Modeling/JsonDiff.cs +++ b/src/Redis.OM/Modeling/JsonDiff.cs @@ -1,5 +1,5 @@ using System; -using System.Linq; +using System.Globalization; using System.Text.Json; using System.Web; using Newtonsoft.Json.Linq; @@ -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() } diff --git a/src/Redis.OM/RedisObjectHandler.cs b/src/Redis.OM/RedisObjectHandler.cs index a60aa023..ac22c3af 100644 --- a/src/Redis.OM/RedisObjectHandler.cs +++ b/src/Redis.OM/RedisObjectHandler.cs @@ -350,7 +350,7 @@ internal static IDictionary 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) diff --git a/test/Redis.OM.Unit.Tests/RediSearchTests/AggregationSetTests.cs b/test/Redis.OM.Unit.Tests/RediSearchTests/AggregationSetTests.cs index 26fe2379..d32ba7e4 100644 --- a/test/Redis.OM.Unit.Tests/RediSearchTests/AggregationSetTests.cs +++ b/test/Redis.OM.Unit.Tests/RediSearchTests/AggregationSetTests.cs @@ -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(_substitute, true, chunkSize: 10000); + _substitute.Execute("FT.AGGREGATE", Arg.Any()).Returns(MockedResult); + _substitute.Execute("FT.CURSOR", Arg.Any()).Returns(MockedResultCursorEnd); + + var y = 30.55M; + Expression, 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()); + } } } diff --git a/test/Redis.OM.Unit.Tests/RediSearchTests/Person.cs b/test/Redis.OM.Unit.Tests/RediSearchTests/Person.cs index 78ff0326..79f3efe5 100644 --- a/test/Redis.OM.Unit.Tests/RediSearchTests/Person.cs +++ b/test/Redis.OM.Unit.Tests/RediSearchTests/Person.cs @@ -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; } diff --git a/test/Redis.OM.Unit.Tests/RediSearchTests/SearchTests.cs b/test/Redis.OM.Unit.Tests/RediSearchTests/SearchTests.cs index d3d263f1..b78780f9 100644 --- a/test/Redis.OM.Unit.Tests/RediSearchTests/SearchTests.cs +++ b/test/Redis.OM.Unit.Tests/RediSearchTests/SearchTests.cs @@ -119,7 +119,7 @@ public void TestFirstOrDefaultWithMixedLocals() } [Fact] - public void TestBasicQueryWithExactNumericMatch() + public void TestBasicQueryWithExactIntegerMatch() { _substitute.ClearSubstitute(); _substitute.Execute(Arg.Any(), Arg.Any()).Returns(_mockReply); @@ -135,6 +135,32 @@ public void TestBasicQueryWithExactNumericMatch() "100"); } + [Fact] + public void TestBasicQueryWithExactDecimalMatch() + { + _substitute.ClearSubstitute(); + _substitute.Execute(Arg.Any(), Arg.Any()).Returns(_mockReply); + var y = 90.5M; + var collection = new RedisCollection(_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() { @@ -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() { @@ -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(), Arg.Any()).Returns(_mockReply); var collection = new RedisCollection(_substitute).Where(x => ints.Contains(x.Integer)); @@ -3457,6 +3493,17 @@ public void NonNullableNumericFieldContains() "LIMIT", "0", "100"); + + collection = new RedisCollection(_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] diff --git a/test/Redis.OM.Unit.Tests/Serialization/ObjectWithNumerics.cs b/test/Redis.OM.Unit.Tests/Serialization/ObjectWithNumerics.cs index 1671e9e1..981cd44d 100644 --- a/test/Redis.OM.Unit.Tests/Serialization/ObjectWithNumerics.cs +++ b/test/Redis.OM.Unit.Tests/Serialization/ObjectWithNumerics.cs @@ -25,4 +25,6 @@ public class ObjectWithNumerics public double Double { get; set; } [Indexed] public float Float { get; set; } + [Indexed] + public decimal Decimal { get; set; } } \ No newline at end of file