[Bug] Fix internal server error when unexpected json inputs are sent to Bookkeeper HTTP Services #3914
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Issue: |
|
Root cause: |
|
Post fix: |
|
Post Fix: |
|
Forked Repo PR where test results passed: Apurva007#1 |
|
@hangc0276 / @BewareMyPower Please can you help review the PR. |
hangc0276
reviewed
Apr 13, 2023
| Map<String, Object> configMap = JsonUtil.fromJson(requestBody, HashMap.class); | ||
| Boolean forceMajor = (Boolean) configMap.getOrDefault("forceMajor", null); | ||
| Boolean forceMinor = (Boolean) configMap.getOrDefault("forceMinor", null); | ||
| Map<String, String> configMap = JsonUtil.fromJson(requestBody, HashMap.class); |
There was a problem hiding this comment.
If the requestBody doesn't pass a String, the JsonUtil.fromJson will throw an exception.
Would you please add a test for this change?
There was a problem hiding this comment.
@hangc0276 Thanks for the review. I will add the respective tests.
Apurva007
force-pushed
the
gc_internal_server_error_fix
branch
6 times, most recently
from
bede630
to
c95cc34
Compare
Apurva007
changed the title
Apurva007
changed the title
Apurva007
force-pushed
the
gc_internal_server_error_fix
branch
from
c95cc34
to
3ff05b8
Compare
… invalid values in json input
Apurva007
force-pushed
the
gc_internal_server_error_fix
branch
from
3ff05b8
to
c034c22
Compare
|
@hangc0276 I have made the suggested changes. Please can you review again when you get a chance. |
|
@hangc0276 Please can you help re-review this PR when you get a chance. |
eolivelli
requested changes
May 8, 2023
| Object forceMinorObj = configMap.getOrDefault("forceMinor", null); | ||
| boolean forceMajor = false, forceMinor = false; | ||
| if (forceMajorObj instanceof Boolean) { | ||
| forceMajor = (Boolean) forceMajorObj; |
There was a problem hiding this comment.
We should throw an error if it is not a boolean, otherwise the result is unpredictable
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Descriptions of the changes in this PR:
This PR fixes the internal server error when the following are executed with unexpected json inputs via the REST API:
The reason this error occurs is because the code is trying to force cast inputs it receives as Json to Boolean without checking the type.
The code change is to make this conversion safer.
Motivation
Fixes #3845
Changes
Modified the code to perform a safer boolean conversion when trying to find the values in the JSON payload.
Master Issue: #3845