From e3de4cc59c4c2ad7a44895a2206094bc54b17826 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Sun, 27 Jul 2025 13:13:11 +0200 Subject: [PATCH] Improve duplicate key warning and errors to include the key name Followup: https://github.com/ruby/json/pull/818 --- CHANGES.md | 2 ++ ext/json/ext/parser/parser.c | 50 +++++++++++++++++++++++++++-- java/src/json/ext/ParserConfig.java | 4 +-- java/src/json/ext/ParserConfig.rl | 4 +-- test/json/json_parser_test.rb | 21 +++++++++++- 5 files changed, 73 insertions(+), 8 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 58e3f60d2..53a18b900 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -2,6 +2,8 @@ ### Unreleased +* Improve duplicate key warning and errors to include the key name. + ### 2025-07-24 (2.13.1) * Fix support for older compilers without `__builtin_cpu_supports`. diff --git a/ext/json/ext/parser/parser.c b/ext/json/ext/parser/parser.c index 01b6e6293..8d72c18ac 100644 --- a/ext/json/ext/parser/parser.c +++ b/ext/json/ext/parser/parser.c @@ -426,6 +426,7 @@ static void emit_parse_warning(const char *message, JSON_ParserState *state) } #define PARSE_ERROR_FRAGMENT_LEN 32 + #ifdef RBIMPL_ATTR_NORETURN RBIMPL_ATTR_NORETURN() #endif @@ -830,21 +831,64 @@ static inline VALUE json_decode_array(JSON_ParserState *state, JSON_ParserConfig return array; } +static VALUE json_find_duplicated_key(size_t count, const VALUE *pairs) +{ + VALUE set = rb_hash_new_capa(count / 2); + for (size_t index = 0; index < count; index += 2) { + size_t before = RHASH_SIZE(set); + VALUE key = pairs[index]; + rb_hash_aset(set, key, Qtrue); + if (RHASH_SIZE(set) == before) { + if (RB_SYMBOL_P(key)) { + return rb_sym2str(key); + } + return key; + } + } + return Qfalse; +} + +static void emit_duplicate_key_warning(JSON_ParserState *state, VALUE duplicate_key) +{ + VALUE message = rb_sprintf( + "detected duplicate key %"PRIsVALUE" in JSON object. This will raise an error in json 3.0 unless enabled via `allow_duplicate_key: true`", + rb_inspect(duplicate_key) + ); + + emit_parse_warning(RSTRING_PTR(message), state); + RB_GC_GUARD(message); +} + +#ifdef RBIMPL_ATTR_NORETURN +RBIMPL_ATTR_NORETURN() +#endif +static void raise_duplicate_key_error(JSON_ParserState *state, VALUE duplicate_key) +{ + VALUE message = rb_sprintf( + "duplicate key %"PRIsVALUE, + rb_inspect(duplicate_key) + ); + + raise_parse_error(RSTRING_PTR(message), state); + RB_GC_GUARD(message); +} + static inline VALUE json_decode_object(JSON_ParserState *state, JSON_ParserConfig *config, size_t count) { size_t entries_count = count / 2; VALUE object = rb_hash_new_capa(entries_count); - rb_hash_bulk_insert(count, rvalue_stack_peek(state->stack, count), object); + const VALUE *pairs = rvalue_stack_peek(state->stack, count); + rb_hash_bulk_insert(count, pairs, object); if (RB_UNLIKELY(RHASH_SIZE(object) < entries_count)) { switch (config->on_duplicate_key) { case JSON_IGNORE: break; case JSON_DEPRECATED: - emit_parse_warning("detected duplicate keys in JSON object. This will raise an error in json 3.0 unless enabled via `allow_duplicate_key: true`", state); + emit_duplicate_key_warning(state, json_find_duplicated_key(count, pairs)); break; case JSON_RAISE: - raise_parse_error("duplicate key", state); + raise_duplicate_key_error(state, json_find_duplicated_key(count, pairs)); break; } } diff --git a/java/src/json/ext/ParserConfig.java b/java/src/json/ext/ParserConfig.java index ccfc558e5..2bd03babc 100644 --- a/java/src/json/ext/ParserConfig.java +++ b/java/src/json/ext/ParserConfig.java @@ -2125,10 +2125,10 @@ else if ( _widec > _JSON_object_trans_keys[_mid+1] ) if (((RubyHash)result).hasKey(lastName)) { if (config.deprecateDuplicateKey) { context.runtime.getWarnings().warning( - "detected duplicate keys in JSON object. This will raise an error in json 3.0 unless enabled via `allow_duplicate_key: true`" + "detected duplicate key " + name.inspect() + " in JSON object. This will raise an error in json 3.0 unless enabled via `allow_duplicate_key: true`" ); } else { - throw parsingError(context, "duplicate key", p, pe); + throw parsingError(context, "duplicate key" + name.inspect(), p, pe); } } } diff --git a/java/src/json/ext/ParserConfig.rl b/java/src/json/ext/ParserConfig.rl index 4bc5d93bd..533b13227 100644 --- a/java/src/json/ext/ParserConfig.rl +++ b/java/src/json/ext/ParserConfig.rl @@ -692,10 +692,10 @@ public class ParserConfig extends RubyObject { if (((RubyHash)result).hasKey(lastName)) { if (config.deprecateDuplicateKey) { context.runtime.getWarnings().warning( - "detected duplicate keys in JSON object. This will raise an error in json 3.0 unless enabled via `allow_duplicate_key: true`" + "detected duplicate key " + name.inspect() + " in JSON object. This will raise an error in json 3.0 unless enabled via `allow_duplicate_key: true`" ); } else { - throw parsingError(context, "duplicate key", p, pe); + throw parsingError(context, "duplicate key" + name.inspect(), p, pe); } } } diff --git a/test/json/json_parser_test.rb b/test/json/json_parser_test.rb index 106492e1c..be13c5ed3 100644 --- a/test/json/json_parser_test.rb +++ b/test/json/json_parser_test.rb @@ -333,11 +333,30 @@ def test_parse_big_integers def test_parse_duplicate_key expected = {"a" => 2} + expected_sym = {a: 2} + assert_equal expected, parse('{"a": 1, "a": 2}', allow_duplicate_key: true) assert_raise(ParserError) { parse('{"a": 1, "a": 2}', allow_duplicate_key: false) } - assert_deprecated_warning(/duplicate keys/) do + assert_raise(ParserError) { parse('{"a": 1, "a": 2}', allow_duplicate_key: false, symbolize_names: true) } + + assert_deprecated_warning(/duplicate key "a"/) do assert_equal expected, parse('{"a": 1, "a": 2}') end + assert_deprecated_warning(/duplicate key "a"/) do + assert_equal expected_sym, parse('{"a": 1, "a": 2}', symbolize_names: true) + end + + unless RUBY_ENGINE == 'jruby' + assert_raise(ParserError) do + fake_key = Object.new + JSON.load('{"a": 1, "a": 2}', -> (obj) { obj == "a" ? fake_key : obj }, allow_duplicate_key: false) + end + + assert_deprecated_warning(/duplicate key # (obj) { obj == "a" ? fake_key : obj }) + end + end end def test_some_wrong_inputs