Fix bug in IsLong implementation (#4655)

Relates: #4649

This commit fixes a bug in the IsLong implementation, and prefers
reading long values over double values in PrimitiveObjectFormatter
when the bytes represent a valid long value, to avoid floating
point rounding errors that may happen when parsing a double from
a long value.

The bug in IsLong was that the loop over the array segment bytes was
exiting early when the count of bytes was the same as long.Min/MaxValue
length, and the current loop byte is less than the byte at index in
long.Min/MaxValue. In this scenario, it should not exit early, but
not continue to check following loop bytes against byte at index in
long.Min/MaxValue. Following loop iterations still need to check
whether remaining bytes are numbers.

Author: russcam

src/Elasticsearch.Net/Extensions/ArraySegmentBytesExtensions.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Globalization;
 using System.Runtime.CompilerServices;
 using Elasticsearch.Net.Utf8Json;
 using Elasticsearch.Net.Utf8Json.Formatters;
@@ -29,9 +30,9 @@ public static bool IsDouble(this ref ArraySegment<byte> arraySegment)
 			return false;
 		}
 
-		private static readonly byte[] LongMaxValue = StringEncoding.UTF8.GetBytes(long.MaxValue.ToString());
+		private static readonly byte[] LongMaxValue = StringEncoding.UTF8.GetBytes(long.MaxValue.ToString(CultureInfo.InvariantCulture));
 
-		private static readonly byte[] LongMinValue = StringEncoding.UTF8.GetBytes(long.MinValue.ToString());
+		private static readonly byte[] LongMinValue = StringEncoding.UTF8.GetBytes(long.MinValue.ToString(CultureInfo.InvariantCulture));
 
 		[MethodImpl(MethodImplOptions.AggressiveInlining)]
 		public static bool IsLong(this ref ArraySegment<byte> arraySegment)
@@ -45,22 +46,27 @@ public static bool IsLong(this ref ArraySegment<byte> arraySegment)
 				return false;
 
 			var longBytes = isNegative ? LongMinValue : LongMaxValue;
-			var i = isNegative ? 1 : 0;
 
+			// this doesn't handle positive values that are prefixed with + symbol.
+			// Elasticsearch does not return values with this prefix.
+			var i = isNegative ? 1 : 0;
+			var check = arraySegment.Count == longBytes.Length;
 			while (i < arraySegment.Count)
 			{
 				var b = arraySegment.Array[arraySegment.Offset + i];
 				if (!NumberConverter.IsNumber(b))
 					return false;
 
 				// only compare to long.MinValue or long.MaxValue if we're dealing with same number of bytes
-				if (arraySegment.Count == longBytes.Length)
+				if (check)
 				{
+					// larger than long.MinValue or long.MaxValue, bail early.
 					if (b > longBytes[i])
 						return false;
 
+					// only continue to check bytes if current byte is equal to longBytes[i]
 					if (b < longBytes[i])
-						return true;
+						check = false;
 				}
 
 				i++;

src/Elasticsearch.Net/Utf8Json/Formatters/PrimitiveObjectFormatter.cs

@@ -90,7 +90,7 @@ public void Serialize(ref JsonWriter writer, object value, IJsonFormatterResolve
 
             if (t.IsEnum)
             {
-                writer.WriteString(t.ToString()); // enum as stringq
+                writer.WriteString(t.ToString()); // enum as string
                 return;
             }
 
@@ -158,13 +158,10 @@ public object Deserialize(ref JsonReader reader, IJsonFormatterResolver formatte
                 case JsonToken.Number:
 					var numberSegment = reader.ReadNumberSegment();
 					// conditional operator here would cast both to double, so don't use.
-					// Check for IsDouble first, IsDouble && IsLong can both return true, prefer precision
-					if (numberSegment.IsDouble())
-						return NumberConverter.ReadDouble(numberSegment.Array, numberSegment.Offset, out _);
+					// Check for IsLong first, avoid floating point rounding
 					if (numberSegment.IsLong())
 						return NumberConverter.ReadInt64(numberSegment.Array, numberSegment.Offset, out _);
 
-
 					return NumberConverter.ReadDouble(numberSegment.Array, numberSegment.Offset, out _);
 				case JsonToken.String:
                     return reader.ReadString();

tests/Tests/CodeStandards/Serialization/PrimitiveObjectFormatterTests.cs

@@ -0,0 +1,84 @@
+using System.Text;
+using Elastic.Xunit.XunitPlumbing;
+using Elasticsearch.Net;
+using FluentAssertions;
+using Tests.Core.Client;
+
+namespace Tests.CodeStandards.Serialization
+{
+	public class PrimitiveObjectFormatterTests
+	{
+		private static DynamicValue GetValue(string json)
+		{
+			var bytes = Encoding.UTF8.GetBytes(json);
+			var client = TestClient.FixedInMemoryClient(bytes);
+			var response = client.LowLevel.Search<DynamicResponse>(PostData.Empty);
+			return response.Body["value"];
+		}
+
+		[U]
+		public void ReadDoubleWhenContainsDecimalPoint()
+		{
+			var value = GetValue(@"{""value"":0.43066659210472646}");
+			value.Value.GetType().Should().Be<double>();
+			value.Value.Should().Be(0.43066659210472646);
+		}
+
+		[U]
+		public void ReadDoubleWhenContainsENotation()
+		{
+			var value = GetValue(@"{""value"":1.7976931348623157E+308}");
+			value.Value.GetType().Should().Be<double>();
+			value.Value.Should().Be(double.MaxValue);
+		}
+
+		[U]
+		public void ReadDoubleWhenGreaterThanLongMaxValue()
+		{
+			var value = GetValue($"{{\"value\":{((double)long.MaxValue) + 1}}}");
+			value.Value.GetType().Should().Be<double>();
+			value.Value.Should().Be(((double)long.MaxValue) + 1);
+		}
+
+		[U]
+		public void ReadDoubleWhenLessThanLongMinValue()
+		{
+			var value = GetValue($"{{\"value\":{((double)long.MinValue) - 1}}}");
+			value.Value.GetType().Should().Be<double>();
+			value.Value.Should().Be(((double)long.MinValue) - 1);
+		}
+
+		[U]
+		public void ReadLongWhenLongMaxValue()
+		{
+			var value = GetValue($"{{\"value\":{long.MaxValue}}}");
+			value.Value.GetType().Should().Be<long>();
+			value.Value.Should().Be(long.MaxValue);
+		}
+
+		[U]
+		public void ReadLongWhenLessThanLongMaxValue()
+		{
+			var value = GetValue($"{{\"value\":{long.MaxValue - 1}}}");
+			value.Value.GetType().Should().Be<long>();
+			value.Value.Should().Be(long.MaxValue - 1);
+		}
+
+		[U]
+		public void ReadLongWhenGreaterThanLongMinValue()
+		{
+			var value = GetValue($"{{\"value\":{long.MinValue + 1}}}");
+			value.Value.GetType().Should().Be<long>();
+			value.Value.Should().Be(long.MinValue + 1);
+		}
+
+		[U]
+		public void ReadLongWhenLongMinValue()
+		{
+			var value = GetValue($"{{\"value\":{long.MinValue}}}");
+			value.Value.GetType().Should().Be<long>();
+			value.Value.Should().Be(long.MinValue);
+		}
+	}
+}
+