Skip to content
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.

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

Improve json decode error messages #9484

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dgud
Copy link
Contributor

@dgud dgud commented Feb 24, 2025

When printing errors include the position that the error occured at if included in the error.

@dgud dgud added the team:PS Assigned to OTP team PS label Feb 24, 2025
@dgud dgud requested a review from bjorng February 24, 2025 13:06
@dgud dgud self-assigned this Feb 24, 2025
Copy link
Contributor

github-actions bot commented Feb 24, 2025

CT Test Results

    2 files     97 suites   1h 8m 39s ⏱️
2 190 tests 2 143 ✅ 47 💤 0 ❌
2 557 runs  2 508 ✅ 49 💤 0 ❌

Results for commit acf88b7.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@@ -633,6 +635,11 @@ check_io_arguments([Type|TypeT], [Arg|ArgT], No) ->
check_io_arguments(TypeT, ArgT, No+1)]
end.

format_json_error(_F, _As, #{position := Position}) ->
[{general, io_lib:format("Occured at position ~w", [Position])}];
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the position a character? or a byte?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is a character in unicode?
I wrote position for a reason :-)
@michalmuskala

Copy link
Contributor

Choose a reason for hiding this comment

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

erlang:char/0 is a character in Erlang :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a byte offset

@bjorng
Copy link
Contributor

bjorng commented Feb 24, 2025

Test case?

@dgud
Copy link
Contributor Author

dgud commented Feb 24, 2025

Test case?

How do I write one, can point me to any examples?

@bjorng
Copy link
Contributor

bjorng commented Feb 24, 2025

How do I write one, can point me to any examples?

error_info(_Config) ->
BadIterator = [-1|#{}],
BadIterator2 = {x, y, z},
GoodIterator = maps:iterator(#{}),
BadOrder = fun(_) -> true end,
GoodOrder = fun(A, B) -> A =< B end,
L = [
{filter, [fun(_, _) -> true end, abc]},
{filter, [fun(_, _) -> true end, BadIterator]},
{filter, [fun(_, _) -> true end, BadIterator2]},
{filter, [bad_fun, BadIterator],[{1,".*"},{2,".*"}]},
{filter, [bad_fun, BadIterator2],[{1,".*"},{2,".*"}]},
{filter, [bad_fun, GoodIterator]},
{filtermap, [fun(_, _) -> true end, abc]},
{filtermap, [fun(_, _) -> true end, BadIterator]},
{filtermap, [fun(_, _) -> true end, BadIterator2]},
{filtermap, [fun(_) -> true end, GoodIterator]},
{filtermap, [fun(_) -> ok end, #{}]},
{find, [key, no_map]},
{fold, [fun(_, _, _) -> true end, init, abc]},
{fold, [fun(_, _, _) -> true end, init, BadIterator]},
{fold, [fun(_, _, _) -> true end, init, BadIterator2]},
{fold, [fun(_) -> true end, init, GoodIterator]},
{fold, [fun(_) -> ok end, init, #{}]},
{foreach, [fun(_, _) -> ok end, no_map]},
{foreach, [fun(_, _) -> ok end, BadIterator]},
{foreach, [fun(_, _) -> ok end, BadIterator2]},
{foreach, [fun(_) -> ok end, GoodIterator]},
{foreach, [fun(_) -> ok end, #{}]},
{from_keys, [#{a => b}, whatever]},
{from_keys, [[a|b], whatever]},
{from_list, [#{a => b}]},
{from_list, [[a|b]]},
{get, [key, #{}]},
{get, [key, {no,map}]},
{get, [key, {no,map}, default]},
{groups_from_list, [not_a_fun, []]},
{groups_from_list, [fun hd/1, not_a_list]},
{groups_from_list, [not_a_fun, fun(_) -> ok end, []]},
{groups_from_list, [fun(_) -> ok end, not_a_fun, []]},
{groups_from_list, [fun(_) -> ok end, fun(_) -> ok end, not_a_list]},
{intersect, [#{a => b}, y]},
{intersect, [x, #{a => b}]},
{intersect, [x, y],[{1,".*"},{2,".*"}]},
{intersect_with, [fun(_, _, _) -> ok end, #{a => b}, y]},
{intersect_with, [fun(_, _, _) -> ok end, x, #{a => b}]},
{intersect_with, [fun(_, _, _) -> ok end, x, y],[{2,".*"},{3,".*"}]},
{intersect_with, [fun(_, _) -> ok end, #{}, #{}]},
{is_iterator_valid, [GoodIterator], [no_fail]},
{is_iterator_valid, [BadIterator], [no_fail]},
{is_iterator_valid, [BadIterator2], [no_fail]},
{is_key,[key, no_map]},
{iterator,[{no,map}]},
{iterator, [{no,map}, undefined], [{1, ".*"}]},
{iterator, [{no,map}, ordered], [{1, ".*"}]},
{iterator, [{no,map}, reversed], [{1, ".*"}]},
{iterator, [{no,map}, GoodOrder], [{1, ".*"}]},
{iterator, [#{a => b}, BadOrder], [{2, ".*"}]},
{iterator, [{no,map}, BadOrder], [{1, ".*"}, {2, ".*"}]},
{keys, [{no,map}]},
{map, [fun(_, _) -> true end, abc]},
{map, [fun(_, _) -> true end, BadIterator]},
{map, [fun(_, _) -> true end, BadIterator2]},
{map, [fun(_) -> true end, GoodIterator]},
{map, [fun(_) -> ok end, #{}]},
{merge,[#{a => b}, {a,b}]},
{merge,[{x,y}, #{a => b}]},
{merge,[{x,y}, {a,b}],[{1,".*"},{2,".*"}]},
{merge_with, [fun(_, _) -> ok end, #{}, #{}]},
{merge_with, [a, b, c],[{1,".*"},{2,".*"},{3,".*"}]},
{next,[no_iterator]},
{next,[BadIterator]},
{put, [key, value, {no,map}]},
{remove,[key, {no,map}]},
{size, [no_map]},
{take, [key, no_map]},
{to_list,[xyz]},
{to_list,[BadIterator]},
{to_list,[BadIterator2]},
{update,[key, value, no_map]},
{update_with, [key, fun(_) -> ok end, {no,map}]},
{update_with, [key, fun() -> ok end, #{}]},
{update_with, [key, fun(_) -> ok end, init, {no,map}]},
{update_with, [key, fun() -> ok end, init, #{}]},
{values,[no_map]},
{with, [not_list, #{}]},
{with, [[1,2,3], {no,map}]},
{without, [not_list, #{}]},
{without, [[1,2,3], {no,map}]}
],
error_info_lib:test_error_info(maps, L).

When printing errors include the position that the error occured at
if included in the error.
@dgud dgud force-pushed the dgud/stdlib/json-error-msg/OTP-19508 branch from dcb1a9a to acf88b7 Compare February 24, 2025 15:33
@dgud dgud added the testing currently being tested, tag is used by OTP internal CI label Feb 24, 2025
Comment on lines +638 to +639
format_json_error(_F, _As, #{position := Position}) ->
[{general, io_lib:format("Occured at position ~w", [Position])}];
Copy link
Contributor

Choose a reason for hiding this comment

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

"Occurred" is misspelled.

Instead of simply fixing the misspelling, I suggest that you generate a clearer message:

Suggested change
format_json_error(_F, _As, #{position := Position}) ->
[{general, io_lib:format("Occured at position ~w", [Position])}];
format_json_error({invalid_byte, Byte}, #{position := Position}) ->
S = io_lib:format("invalid byte ~w at position ~w", [Byte, Position]),
[{general, S}];

(You will have to update the call to format_json_error, too.)

This will result in some redundancy, but overall I think it's clearer:

1> json:decode(~'["valid string", not_valid]').
** exception error: {invalid_byte,111}
     in function  json:invalid_byte/2 (json.erl, line 541)
        *** invalid byte 111 at position 18
     in call from json:decode/1 (json.erl, line 879)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants