Skip to content

Commit

Permalink
Raise ParseError for non-numeric strings in numeric fields in Ruby an…
Browse files Browse the repository at this point in the history
…d PHP JSON parsing.

This fixes Ruby and PHP to be conformant with the Protobuf's JSON spec. Note this fix is not accompanied by a major version bump for Ruby or PHP, but was pre-announced in https://engdoc.corp.google.com/eng/doc/devguide/proto/news/2024-11-07.md#ruby-and-php-errors-in-json-parsing and landed as a warning in 29.x.

PiperOrigin-RevId: 704855202
  • Loading branch information
zhangskz authored and copybara-github committed Dec 10, 2024
1 parent 828716e commit abb197c
Show file tree
Hide file tree
Showing 10 changed files with 6 additions and 68 deletions.
10 changes: 0 additions & 10 deletions conformance/failure_list_jruby_ffi.txt
Original file line number Diff line number Diff line change
@@ -1,11 +1 @@
Required.*.JsonInput.DoubleFieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.FloatFieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.Int32FieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.Int64FieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.Uint32FieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.Uint64FieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.DoubleFieldStringValueNonNumeric # Should have failed to parse, but didn't.
Required.*.JsonInput.DoubleFieldStringValuePartiallyNumeric # Should have failed to parse, but didn't.
Required.*.JsonInput.FloatFieldStringValueNonNumeric # Should have failed to parse, but didn't.
Required.*.JsonInput.FloatFieldStringValuePartiallyNumeric # Should have failed to parse, but didn't.
Required.*.JsonInput.Int32FieldQuotedExponentialValue.* # Failed to parse input or produce output.
10 changes: 0 additions & 10 deletions conformance/failure_list_php_c.txt
Original file line number Diff line number Diff line change
@@ -1,14 +1,4 @@
Recommended.Proto2.JsonInput.FieldNameExtension.Validator
Required.*.JsonInput.DoubleFieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.FloatFieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.Int32FieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.Int64FieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.Uint32FieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.Uint64FieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.DoubleFieldStringValueNonNumeric # Should have failed to parse, but didn't.
Required.*.JsonInput.DoubleFieldStringValuePartiallyNumeric # Should have failed to parse, but didn't.
Required.*.JsonInput.FloatFieldStringValueNonNumeric # Should have failed to parse, but didn't.
Required.*.JsonInput.FloatFieldStringValuePartiallyNumeric # Should have failed to parse, but didn't.
Required.*.JsonInput.Int32FieldQuotedExponentialValue.* # Failed to parse input or produce output.
Required.Proto2.JsonInput.BoolFieldFalse.JsonOutput
Required.Proto2.JsonInput.BoolFieldFalse.ProtobufOutput
Expand Down
10 changes: 0 additions & 10 deletions conformance/failure_list_ruby.txt
Original file line number Diff line number Diff line change
@@ -1,11 +1 @@
Required.*.JsonInput.DoubleFieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.FloatFieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.Int32FieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.Int64FieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.Uint32FieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.Uint64FieldEmptyString # Should have failed to parse, but didn't.
Required.*.JsonInput.DoubleFieldStringValueNonNumeric # Should have failed to parse, but didn't.
Required.*.JsonInput.DoubleFieldStringValuePartiallyNumeric # Should have failed to parse, but didn't.
Required.*.JsonInput.FloatFieldStringValueNonNumeric # Should have failed to parse, but didn't.
Required.*.JsonInput.FloatFieldStringValuePartiallyNumeric # Should have failed to parse, but didn't.
Required.*.JsonInput.Int32FieldQuotedExponentialValue.* # Failed to parse input or produce output.
3 changes: 0 additions & 3 deletions php/ext/google/protobuf/message.c
Original file line number Diff line number Diff line change
Expand Up @@ -735,9 +735,6 @@ PHP_METHOD(Message, mergeFromJsonString) {
switch (result) {
case kUpb_JsonDecodeResult_Ok:
break;
case kUpb_JsonDecodeResult_OkWithEmptyStringNumerics:
zend_error(E_USER_WARNING, "%s", upb_Status_ErrorMessage(&status));
return;
case kUpb_JsonDecodeResult_Error:
zend_throw_exception_ex(NULL, 0, "Error occurred during parsing: %s",
upb_Status_ErrorMessage(&status));
Expand Down
3 changes: 0 additions & 3 deletions ruby/ext/google/protobuf_c/message.c
Original file line number Diff line number Diff line change
Expand Up @@ -1046,9 +1046,6 @@ static VALUE Message_decode_json(int argc, VALUE* argv, VALUE klass) {
switch (result) {
case kUpb_JsonDecodeResult_Ok:
break;
case kUpb_JsonDecodeResult_OkWithEmptyStringNumerics:
rb_warn("%s", upb_Status_ErrorMessage(&status));
break;
case kUpb_JsonDecodeResult_Error:
rb_raise(cParseError, "Error occurred during parsing: %s",
upb_Status_ErrorMessage(&status));
Expand Down
1 change: 0 additions & 1 deletion ruby/lib/google/protobuf/ffi/ffi.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ class FFI

## JSON Decoding results
Upb_JsonDecodeResult_Ok = 0
Upb_JsonDecodeResult_OkWithEmptyStringNumerics = 1
Upb_JsonDecodeResult_Error = 2

typedef :pointer, :Array
Expand Down
2 changes: 0 additions & 2 deletions ruby/lib/google/protobuf/ffi/message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -272,8 +272,6 @@ def self.decode_json(data, options = {})
status = Google::Protobuf::FFI::Status.new
result = Google::Protobuf::FFI.json_decode_message_detecting_nonconformance(data, data.bytesize, message.instance_variable_get(:@msg), message.class.descriptor, pool_def, decoding_options, message.instance_variable_get(:@arena), status)
case result
when Google::Protobuf::FFI::Upb_JsonDecodeResult_OkWithEmptyStringNumerics
warn Google::Protobuf::FFI.error_message(status)
when Google::Protobuf::FFI::Upb_JsonDecodeResult_Error
raise ParseError.new "Error occurred during parsing: #{Google::Protobuf::FFI.error_message(status)}"
end
Expand Down
30 changes: 4 additions & 26 deletions ruby/tests/encode_decode_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,36 +38,14 @@ def hex2bin(s)

class NonConformantNumericsTest < Test::Unit::TestCase
def test_empty_json_numerics
if defined? JRUBY_VERSION and Google::Protobuf::IMPLEMENTATION != :FFI
# In a future version, CRuby and JRuby FFI will also have this behavior.
assert_raises Google::Protobuf::ParseError do
msg = ::BasicTestProto2::TestMessage.decode_json('{"optionalInt32":""}')
end
else
warnings = CaptureWarnings.capture {
msg = ::BasicTestProto2::TestMessage.decode_json('{"optionalInt32":""}')
assert_equal 0, msg.optional_int32
assert msg.has_optional_int32?
}
assert_equal 1, warnings.size
assert_match "Empty string is not a valid number (field: basic_test_proto2.TestMessage.optional_int32)", warnings[0]
assert_raises Google::Protobuf::ParseError do
msg = ::BasicTestProto2::TestMessage.decode_json('{"optionalInt32":""}')
end
end

def test_trailing_non_numeric_characters
if defined? JRUBY_VERSION and Google::Protobuf::IMPLEMENTATION != :FFI
# In a future version, CRuby and JRuby FFI will also have this behavior.
assert_raises Google::Protobuf::ParseError do
msg = ::BasicTestProto2::TestMessage.decode_json('{"optionalDouble":"123abc"}')
end
else
warnings = CaptureWarnings.capture {
msg = ::BasicTestProto2::TestMessage.decode_json('{"optionalDouble":"123abc"}')
assert_equal 123, msg.optional_double
assert msg.has_optional_double?
}
assert_equal 1, warnings.size
assert_match "Non-number characters in quoted number (field: basic_test_proto2.TestMessage.optional_double)", warnings[0]
assert_raises Google::Protobuf::ParseError do
msg = ::BasicTestProto2::TestMessage.decode_json('{"optionalDouble":"123abc"}')
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions upb/json/decode.c
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,7 @@ static int64_t jsondec_strtoint64(jsondec* d, upb_StringView str) {
static void jsondec_checkempty(jsondec* d, upb_StringView str,
const upb_FieldDef* f) {
if (str.size != 0) return;
d->result = kUpb_JsonDecodeResult_OkWithEmptyStringNumerics;
d->result = kUpb_JsonDecodeResult_Error;
upb_Status_SetErrorFormat(d->status,
"Empty string is not a valid number (field: %s). "
"This will be an error in a future version.",
Expand Down Expand Up @@ -782,7 +782,7 @@ static upb_MessageValue jsondec_double(jsondec* d, const upb_FieldDef* f) {
char* end;
val.double_val = strtod(str.data, &end);
if (end != str.data + str.size) {
d->result = kUpb_JsonDecodeResult_OkWithEmptyStringNumerics;
d->result = kUpb_JsonDecodeResult_Error;
upb_Status_SetErrorFormat(
d->status,
"Non-number characters in quoted number (field: %s). "
Expand Down
1 change: 0 additions & 1 deletion upb/json/decode.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ enum { upb_JsonDecode_IgnoreUnknown = 1 };

enum {
kUpb_JsonDecodeResult_Ok = 0,
kUpb_JsonDecodeResult_OkWithEmptyStringNumerics = 1,
kUpb_JsonDecodeResult_Error = 2,
};

Expand Down

0 comments on commit abb197c

Please sign in to comment.