Bugfix: util: Correctly handle Number value types in GetArg/GetBoolArg #24479
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
3766c66 to
bd32cd9
Compare
|
Rebased due to merge of partial fix in #24498 |
|
The unit tests are failing: test/getarg_tests.cpp(103): error: in "getarg_tests/setting_args": exception std::runtime_error expected but not raised
test/getarg_tests.cpp(104): error: in "getarg_tests/setting_args": exception std::runtime_error expected but not raised
test/getarg_tests.cpp(110): error: in "getarg_tests/setting_args": exception std::runtime_error expected but not raised
test/getarg_tests.cpp(111): error: in "getarg_tests/setting_args": exception std::runtime_error expected but not raised
test/getarg_tests.cpp(117): error: in "getarg_tests/setting_args": exception std::runtime_error expected but not raised
test/getarg_tests.cpp(118): error: in "getarg_tests/setting_args": exception std::runtime_error expected but not raised
test/getarg_tests.cpp(53): Leaving test case "setting_args"; testing time: 63184us |
|
Fixed tests. |
bd32cd9 to
3372227
Compare
There was a problem hiding this comment.
I could see people having different opinions about this change. Personally I wouldn't object to it, but I also don't think it is an improvement overall.
The PR has changed since it was opened, but now it is pretty simple. Right now, if a user edits their settings.json file, or if the settings.json file gets corrupted some other way so it contains a numeric int or float value where a boolean value is expected, the current GetBoolArg method will throw an exception. This PR changes GetBoolArg behavior in that case to return false if the number is zero and true if the number is nonzero instead of throwing an exception.
Since this PR does not change behavior in other cases, GetBoolArg still throws exceptions if the settings value is an array or object, and GetBoolArg still has strange behavior for string values of returning true for the empty string "", true for strings which are non-zero decimal numbers, and false for all other strings.
Since this PR isn't removing every case where GetBoolArg throws exceptions, and since specifying a number where a boolean value is expected is a real indication of corruption or misconfiguration, I think throwing an exception is probably the best thing method can do right now to deal with whatever bad value is causing the problem. In longer run, if a change like #16545 goes through, settings could be typed, and errors could be triggered earlier on startup before an exception could happen here.
Anyway, this is not a very strong opinion, and I'm fine with other approaches.
Code review ACK 3372227
3372227 to
8a5324a
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
The PR didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open PRs. Closing due to lack of interest. |
Currently, Number values can cause
GetArg/GetBoolArg to throw an exception, which most of the code isn't expecting (eg #24457).AFAIK it isn't possible to normally get Numbers in settings right now, but
that's expected to change with #15936 (which could create downgrading issues without this fixed), and in any caseit isn't a great idea to randomly crash because of a logically-value settings.json value anyway.This doesn't fix Get*Arg from throwing in the case it encounters an Array or Object. It's not obvious how to handle those scenarios. However, the function
stouched are refactored to explicitly clarify that those scenarios will throw.NOTE: Based on 0.20 branch-point, so a clean merge to everything newer. For testing, you will probably want to merge onto 23.x or master.