Skip to content

Add support for deserializing numbers from strings #68

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

Merged
Havret merged 1 commit into from
Nov 7, 2025
Merged

Add support for deserializing numbers from strings #68

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
27 changes: 23 additions & 4 deletions src/Protobuf.System.Text.Json/InternalConverters/FieldConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
using System.Buffers;
using System.Diagnostics;
Comment on lines +1 to +2
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

The System.Buffers and System.Diagnostics namespaces are imported but not used in this file. Remove these unused imports.

Suggested change
using System.Buffers;
using System.Diagnostics;

Copilot uses AI. Check for mistakes.
using System.Text.Json;
using System.Text.Json.Serialization;
using Google.Protobuf;
Expand All @@ -8,6 +10,15 @@ namespace Protobuf.System.Text.Json.InternalConverters;
internal class FieldConverter<T> : InternalConverter
{
private JsonConverter<T>? _converter;
private readonly bool _isConverterForNumberType;

public FieldConverter()
{
var type = typeof(T);
type = Nullable.GetUnderlyingType(type) ?? type;
var typeCode = Type.GetTypeCode(type);
_isConverterForNumberType = typeCode is >= TypeCode.SByte and <= TypeCode.Decimal;
}

public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options)
{
Expand All @@ -23,14 +34,22 @@ public override void Read(ref Utf8JsonReader reader, IMessage obj, Type typeToCo
{
return;
}

var read = _converter.Read(ref reader, typeToConvert, options);
if (read is { } value)

if (_isConverterForNumberType && reader.TokenType == JsonTokenType.String && (JsonNumberHandling.AllowReadingFromString & options.NumberHandling) != 0)
{
var value = Convert.ChangeType(reader.GetString(), typeToConvert);
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

Using Convert.ChangeType without specifying CultureInfo.InvariantCulture can lead to culture-dependent parsing issues. JSON numbers should always use invariant culture (decimal point, not comma). Additionally, Convert.ChangeType can throw FormatException or InvalidCastException which are not wrapped in JsonException, making error handling inconsistent with the rest of the deserializer. Consider using type-specific parsing methods (e.g., double.Parse, int.Parse) with NumberStyles and CultureInfo.InvariantCulture, and wrap exceptions in JsonException.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

Using Convert.ChangeType with reflection-based type conversion is significantly slower than type-specific parsing. For numeric types, consider using a switch statement on Type.GetTypeCode(typeToConvert) with direct calls to Parse methods for each type (e.g., int.Parse, double.Parse, etc.), which will be more performant.

Suggested change
var value = Convert.ChangeType(reader.GetString(), typeToConvert);
object value;
var str = reader.GetString();
switch (Type.GetTypeCode(typeToConvert))
{
case TypeCode.Byte:
value = byte.Parse(str);
break;
case TypeCode.SByte:
value = sbyte.Parse(str);
break;
case TypeCode.Int16:
value = short.Parse(str);
break;
case TypeCode.UInt16:
value = ushort.Parse(str);
break;
case TypeCode.Int32:
value = int.Parse(str);
break;
case TypeCode.UInt32:
value = uint.Parse(str);
break;
case TypeCode.Int64:
value = long.Parse(str);
break;
case TypeCode.UInt64:
value = ulong.Parse(str);
break;
case TypeCode.Single:
value = float.Parse(str);
break;
case TypeCode.Double:
value = double.Parse(str);
break;
case TypeCode.Decimal:
value = decimal.Parse(str);
break;
default:
throw new JsonException($"Unsupported numeric type: {typeToConvert}");
}

Copilot uses AI. Check for mistakes.
fieldAccessor.SetValue(obj, value);
}
else
{
var read = _converter.Read(ref reader, typeToConvert, options);
if (read is { } value)
{
fieldAccessor.SetValue(obj, value);
}
}
}

private static JsonConverter<T> GetConverter(ref JsonSerializerOptions options)
{
var converter = options.GetConverter(typeof(T));
Expand Down
84 changes: 84 additions & 0 deletions test/Protobuf.System.Text.Json.Tests/JsonNumberHandlingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
using System.Text.Json;
using System.Text.Json.Protobuf.Tests;
using System.Text.Json.Serialization;
using Protobuf.System.Text.Json.Tests.Utils;
using Xunit;

namespace Protobuf.System.Text.Json.Tests;

public class JsonNumberHandlingTests
{
[Fact]
public void Should_deserialize_numbers_as_strings_when_NumberHandling_set_to_AllowReadingFromString()
{
// Arrange
var json =
"""
{
"doubleProperty" : "0.12",
"floatProperty" : "7.89",
"int32Property" : "123",
"int64Property" : "456",
"uint32Property" : "789",
"uint64Property" : "101112",
"sint32Property" : "1314",
"sint64Property" : "151617",
"fixed32Property" : "1819",
"fixed64Property" : "202122",
"sfixed32Property" : "2324",
"sfixed64Property" : "252627"
}
""";

var options = TestHelper.CreateJsonSerializerOptions();
options.NumberHandling = JsonNumberHandling.AllowReadingFromString;

// Act
var deserialized = JsonSerializer.Deserialize<SimpleMessage>(json, options);

// Assert
Assert.NotNull(deserialized);
Assert.Equal(0.12d, deserialized.DoubleProperty);
Assert.Equal(7.89f, deserialized.FloatProperty);
Assert.Equal(123, deserialized.Int32Property);
Assert.Equal(456L, deserialized.Int64Property);
Assert.Equal(789u, deserialized.Uint32Property);
Assert.Equal(101112ul, deserialized.Uint64Property);
Assert.Equal(1314, deserialized.Sint32Property);
Assert.Equal(151617L, deserialized.Sint64Property);
Assert.Equal(1819u, deserialized.Fixed32Property);
Assert.Equal(202122ul, deserialized.Fixed64Property);
Assert.Equal(2324, deserialized.Sfixed32Property);
Assert.Equal(252627L, deserialized.Sfixed64Property);
}

[Fact]
public void Should_fail_deserializing_numbers_as_strings_when_NumberHandling_not_set_to_AllowReadingFromString()
{
// Arrange
var json =
"""
{
"doubleProperty" : "0.12",
"floatProperty" : "7.89",
"int32Property" : "123",
"int64Property" : "456",
"uint32Property" : "789",
"uint64Property" : "101112",
"sint32Property" : "1314",
"sint64Property" : "151617",
"fixed32Property" : "1819",
"fixed64Property" : "202122",
"sfixed32Property" : "2324",
"sfixed64Property" : "252627"
}
""";

var options = TestHelper.CreateJsonSerializerOptions();
// Explicitly disable reading numbers from strings
options.NumberHandling = JsonNumberHandling.Strict;

// Act & Assert
Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<SimpleMessage>(json, options));
}
}