Skip to content

Conversation

@Havret
Copy link
Owner

@Havret Havret commented Nov 6, 2025

No description provided.

@Havret Havret requested a review from Copilot November 6, 2025 16:55
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for the JsonNumberHandling.AllowReadingFromString option when deserializing protobuf messages with System.Text.Json. This allows numeric fields to be deserialized from JSON string values when the appropriate option is enabled.

  • Added logic in FieldConverter<T> to handle string-to-number conversions when JsonNumberHandling.AllowReadingFromString is set
  • Added comprehensive test coverage for both successful and failure scenarios

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/Protobuf.System.Text.Json/InternalConverters/FieldConverter.cs Implements number-from-string deserialization support with appropriate type checking and conversion logic
test/Protobuf.System.Text.Json.Tests/JsonNumberHandlingTests.cs Adds test cases to verify number-from-string conversion works correctly when enabled and fails when disabled

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1 to +2
using System.Buffers;
using System.Diagnostics;
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.

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.

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 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.
@Havret Havret merged commit c248b75 into main Nov 7, 2025
8 checks passed
@Havret Havret deleted the Add-support-for-deserializing-numbers-from-strings branch November 7, 2025 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants