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

Minor 'invalid code' error output suggestion #7174

Closed
curly-brace-butler opened this issue May 1, 2024 · 0 comments
Closed

Minor 'invalid code' error output suggestion #7174

curly-brace-butler opened this issue May 1, 2024 · 0 comments
Assignees

Comments

@curly-brace-butler
Copy link

curly-brace-butler commented May 1, 2024

What version of gRPC are you using?

google.golang.org/grpc v1.63.2

What version of Go are you using (go version)?

go1.20.10 darwin/arm64

What operating system (Linux, Windows, …) and version?

macOS

What did you do?

Attempted to call standard json.Unmarshal on a string {"body":"redactedBase64","statusCode":200,"delay":0} to unmarshall it into a struct defined:

type ExampleRequest struct {
	Body       []byte     `json:"body"`
	StatusCode codes.Code `json:"statusCode"`
	Delay      int        `json:"delay"` // response delay in ms
}

What did you expect to see?

invalid code: 200

What did you see instead?

invalid code: 'È'

Suggestion / Question

In the implementation (https://github.com/grpc/grpc-go/blob/b433b9467d87d70de277ee7e1139ef2ad900bfa4/codes/codes.go):

	if ci, err := strconv.ParseUint(string(b), 10, 32); err == nil {
		if ci >= _maxCode {
			return fmt.Errorf("invalid code: %q", ci)
		}

		*c = Code(ci)
		return nil
	}

After strconv.ParseUint is successful, we've decided to treat "ci" as an unsigned base-10 integer. So, I think we should output in the error message using %d. That way, if someone tries to unmarshal 200, 17, etc., it will show up more clearly in the error logs.

In the branch reached if ParseUint err != nil, maybe it still makes sense to use %q, since we've not made a decision on how to interpret the byte[] value.

Maybe it is a standard best practice to use %q in this manner and the appearance of a random Unicode rune in error output suggests just generically "bad bytes" without committing to a particular datatype/interpretation. But, I did feel this error message was a little misleading. The phrasing "invalid code" didn't indicate the issue had originated from "gRPC codes" and when working in the context of JSON unmarshalling the generic term "code" could relate to any number of issues in converting data to and from byte-level and textual representations. It doesn't really point one to gRPC codes as a specific starting place for debugging.

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

No branches or pull requests

2 participants