Should include the name of enum in error message when unmarshaling an invalid enum value

[nvatuan]

Currently, when passing an invalid enum value, protobuf returns error saying invalid value for enum type: \"<value>\": Request:

{
    "enumField": "invalid_value"
}

Response:

{
    "code": 3,
    "message": "proto: (line X:Y): invalid value for enum type: \"invalid_value\"",
    "details": []
}

I expect it to return the name of the enum instead of the value. I check the source code of the decoder: https://github.com/protocolbuffers/protobuf-go/blob/f0e23c7a8f8d55910f877091fdad71d3abb26d10/encoding/protojson/decode.go#L331-L340

And I think it should have the commented line:

case protoreflect.EnumKind:
    if v, ok := unmarshalEnum(tok, fd); ok {
        return v, nil
    }
    // return protoreflect.Value{}, d.newError(tok.Pos(), "invalid value %v for enum type: %v", tok.RawString(), fd.Enum().Name())
default:
    panic(fmt.Sprintf("unmarshalScalar: invalid scalar kind %v", kind))
}
return protoreflect.Value{}, d.newError(tok.Pos(), "invalid value for %v type: %v", kind, tok.RawString())

so the error message would look like this:

"message": "proto: (line X:Y): invalid value \"invalid_value\" for enum type: enum_name"`

[cybrcodr]

I'm assuming you're asking for the field name. In which case, it may be argued that providing the full path to the field will be better. While that seems nice, the initial implementation avoided that due to the extra cost that it will incur. We opted to provide the position info instead and I think that should mostly suffice for debugging purposes.

[puellanivis]

I kind of like to avoid injecting user supplied values into the middle of an error message. As this complicates searching Google or other such resource for the issue.

d.newError(tok.Pos(), "invalid value for enum type: %v not in %v", kind, tok.RawString(), fd.Enum().Name()) groups a header of generic error details, followed by variable specifics.

Yielding the following text which is more easily isolatable to the causing code:

"message": "proto: (line X:Y): invalid value for enum type: \"invalid_value\" not in enum_name"

I’m curious about what the actual extra cost would be? Is it really going to be that significant?

[nvatuan]

@cybrcodr: I'm assuming you're asking for the field name.

No, I only want the error message to include the name of the Enum that was defined in the proto file. We have this proto file that defined a bunch of enums and we were testing our generated HTTP gateway. We tried sending a full body request and it failed at one of the enum fields. Although we did find out which enum it failed by looking at the position, but personally I think it would be better if the error message could straight up telling us which enum our request was failling.

@puellanivis: I kind of like to avoid injecting user supplied values into the middle of an error message. As this complicates searching Google or other such resource for the issue.

I agree.

@puellanivis: I’m curious about what the actual extra cost would be? Is it really going to be that significant?

Me too. As I propose returning an error with a custom message, I figure it wouldn't cost much because the Enum name is already there to use. Although I have some concern that it might break something if there is someone out there wrapping the error handling by checking for the "enum" token in error message.

[cybrcodr]

Yes, the enum name is there and the message can include the enum name easily. I had thought you were asking about the field name/path, hence I had to clarify.

[aktau]

Improving the error message to mention the enum name seems like a good idea and we'd welcome a PR.

[cybrcodr]

I don't have the bandwidth right now. Earliest I can do this is at the end of the year. If someone else can pick this up sooner, that will be great.

[igorbrites]

Hey, it's 2024 already and I'm still facing this issue. Is it going to be worked on at some time?