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

fix: avoid runtime panic when the errors field is nil #236

Merged
merged 1 commit into from
Jan 7, 2025
Merged

Conversation

pyrooka
Copy link
Member

@pyrooka pyrooka commented Jan 7, 2025

This PR adds an extra step to the error message processing chain,
so that the length of the errors slice is checked before trying to
get the first error from it.

@pyrooka pyrooka requested a review from padamstx January 7, 2025 16:41
@pyrooka
Copy link
Member Author

pyrooka commented Jan 7, 2025

This PR will be held back until Travis starts functioning again.

@pyrooka pyrooka self-assigned this Jan 7, 2025
@pyrooka
Copy link
Member Author

pyrooka commented Jan 7, 2025

@padamstx This is a trust game. There is no Travis to help you out. Only you and me. 😂

Copy link
Member

@padamstx padamstx left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -627,7 +627,11 @@ func getErrorMessage(responseMap map[string]interface{}, statusCode int) string
var errors Errors
responseBuffer, _ := json.Marshal(responseMap)
if err := json.Unmarshal(responseBuffer, &errors); err == nil {
return errors.Errors[0].Message
if len(errors.Errors) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Wow, how did we exist for so long with this bug in place? LOL
This must be the first time that a service returned an error response with an explicit null value for "errors".

Please check the other cores to make sure we don't have a similar bug there.

…nil`

Signed-off-by: Norbert Biczo <pyrooka@users.noreply.github.com>
@padamstx
Copy link
Member

padamstx commented Jan 7, 2025

@pyrooka After I got GH actions working on the main branch (and published a release), I rebased your PR branch on top of the latest main branch, then force-pushed it. Since everything is now green, I'll merge it.

@padamstx padamstx merged commit 9c104b3 into main Jan 7, 2025
12 checks passed
@padamstx padamstx deleted the nb/fix-panic branch January 7, 2025 21:14
@ibm-devx-sdk
Copy link

🎉 This PR is included in version 5.18.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants