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

jsonpb: error on scalar enum provided for repeated enums instead of panic #627

Merged
merged 2 commits into from
Jun 6, 2018

Conversation

eleniums
Copy link
Contributor

@eleniums eleniums commented Jun 5, 2018

Ran into an issue where attempting to unmarshal valid json that contains a field with a single enum into a message that has a repeated enum field causes a panic. For example, try to unmarshal this:

{
    "rFunny":"PUNS"
}

into a message that looks like this:

type Message struct {
    RFunny []Message_Humour
}

This will cause a panic. I know the json is an invalid representation of the Message struct, but it shouldn't panic. I'm attempting to fix this issue by validating that the targetType is correct before setting the enum.

jsonpb/jsonpb.go Outdated
target.SetInt(int64(n))
if targetType.Kind() == reflect.Int32 {
target.SetInt(int64(n))
}
Copy link
Member

Choose a reason for hiding this comment

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

The conditional is correct, but the behavior is still off. This simply ignores the invalid input instead of returning an error.

@eleniums
Copy link
Contributor Author

eleniums commented Jun 6, 2018

I wasn't sure whether jsonpb typically ignored errors like that or not. I just pushed a commit to instead return an error in this scenario.

@dsnet dsnet changed the title jsonpb: add fix for single enum to repeated enum field jsonpb: error on scalar enum provided for repeated enums instead of panic Jun 6, 2018
@dsnet dsnet merged commit 64db29d into golang:master Jun 6, 2018
@eleniums eleniums deleted the fix/marshal-panic branch June 6, 2018 03:25
@golang golang locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants