jsonpb: Unmarshal returns generic error on unknown fields

[atombender]

When AllowUnknownFields is false, Unmarshal will produce an error string such as:

unknown field "glorp" in proto.Mutation

This leaks the type proto.Mutation to clients, which should not need to see such internals when they are consuming a pure-JSON HTTP API. The only way for an app to prevent this is to use a regexp to detect this specific unmarshaling error, which is not acceptable.

There are other instances of fmt.Errorf being used, but this is the main one that leaks.

Fixed in this PR, which was rejected for not creating an issue first.

[dsnet]

I understand that the type mentioned is specific to Go, but I don't understand why new API has to be exported to support this. Why can't we just change the error message to use the canonical message name of the proto type?

[atombender]

For a JSON-based API, there is arguably no "canonical message name". Unless Protobuf magically starts supporting a mapping to JSON Schema, OpenAPI or similar.

We have a specific use case where gRPC is available internally, but the JSON marshaler is used for a consumer-facing API. These error messages, referring to internal types inside the app, are confusing. (Adding to the confusion is that the consumer of this API cannot get a position inside the JSON.)

[dsnet]

This project is fundamentally about protocol buffers. The canonical proto message name should be sufficient to describe which message is problematic. I believe it is reasonable for a user to be aware of the proto definitions since the JSON schema is derived from the JSON<->Protobuf specification.

Exposing API for a more structured error is a dangerous slippery slope to go down. There are many possible failure modes for protos, and this should not be a precedent that we add API for every possible failure scenario.

[atombender]

This project is also about a mapping between Protobuf and JSON. Plenty of people use the grpc-gateway, which relies on the JSON marshaling. A JSON client shouldn't need to know that there's a gRPC system underneath.

When designing layered systems, you want to be able to prevent underlying layers from leaking through. For example, consider, hypothetically, if protobuf read files as part of its normal operation. You would not want a ENOENT: no such file or directory /foo/bar to bubble up above the API layer so a client could see it.

I don't see where the "slippery slope" is. Go has error values for a reason. They should be used to provide enough information for a caller to intercept a specific use case. If that wasn't the case, then all Go errors would be strings.

[dsnet]

Plenty of people use the grpc-gateway, which relies on the JSON marshaling.

The documentation for grpc-gateway says that it uses jsonpb. How are users suppose to know what the public API is? Well, they read the API as specified in the proto files somewhere. By it's very nature grpc-gateway bridges protos and JSON. You cannot reasonably expect protos to be hidden away completely.

Secondly, the documentation says it uses jsonpb with the OrigName set to true, which blends the two layers together all the more.

For example, consider, hypothetically, if protobuf read files as part of its normal operation. You would not want a ENOENT: no such file or directory /foo/bar to bubble up above the API layer so a client could see it.

That's not a strong argument since protos don't do anything of the sort. The error returned is distinctly related to protos, so its okay that it returns proto-specific information. The reasonable changes to possibly make are either:

• to use the proto type name, rather than the Go type name.
• print the full JSON path leading up to the error

I don't see where the "slippery slope" is.

The slippery slope is after adding UnknownFieldError, we start adding UnknownMessageError, OverflowError, InvalidWellKnownTypeError, and so on.

Go has error values for a reason.

That's not a strong argument either. Go also has complex numbers; doesn't mean we should use them in protos. Also, we do use errors, we have just intentionally chosen not to provide structured errors to avoid bloating the API. The only form of structured errors we may consider are those that have semantic meaning in the realm of protos.

[dsnet]

Closing as duplicate of #266. I agree that the errors messages can be improve, but I don't see a compelling reason to provide structured errors.