dotnet / dotnet/aspnetcore

Grpc JsonTranscoding - use JsonException instead of InvalidOperationException

Open
#52,552 0 comments 0 reactions 0 assignees View on GitHub
area-grpc
Dominant language
C#
Stars
38.4k
Forks
10.9k
Avg merge
2d 6h
Merged PRs (30d)
290

Description

### Is there an existing issue for this?

- [X] I have searched the existing issues

### Is your feature request related to a problem? Please describe the problem.

At this moment when json request is not valid or some field is bool instead of string or anything else, output returned by transcoder is very weak. for example when we send
```json
{
"Option": true
}
```
but out protobuf message expect some `enum` value, we will get something like this
```json
{
"code": 3,
"message": "Unexpected JSON token: True.",
"details": []
}
```
When message contains more fields or even some array or some sub message it's getting very difficult to spot where is the issue for the user of the API

### Describe the solution you'd like

So it's my propsal and question at the same time(I didn't found an answer).
Why `InvalidOperationException` is used instead of `JsonException`?
In the example we can look into [this line](https://github.com/dotnet/aspnetcore/blob/61c585890c09e8a30f511d19c6c9c45e5e9f63b1/src/Grpc/JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/Json/EnumConverter.cs#L43) `throw new InvalidOperationException($"Unexpected JSON token: {reader.TokenType}.");`

When I changed exception type to `JsonException` then I could go to [exception handler](https://github.com/dotnet/aspnetcore/blob/61c585890c09e8a30f511d19c6c9c45e5e9f63b1/src/Grpc/JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/JsonRequestHelpers.cs#L306-L315) and change it to something like
```cs
catch (JsonException ex) when (ex.Message?.Contains("Path: ") ?? false)
{
GrpcServerLog.ErrorReadingMessage(serverCallContext.Logger, ex);
throw new RpcException(new Status(StatusCode.InvalidArgument, $"{ex.Message} | Path: {ex.Path} | LineNumber: {ex.LineNumber} | BytePositionInLine: {ex.BytePositionInLine}.", ex));
}
catch (JsonException ex)
{
GrpcServerLog.ErrorReadingMessage(serverCallContext.Logger, ex);
throw new RpcException(new Status(StatusCode.InvalidArgument, ex.Message, ex));
}
```
I also changed `"Request JSON payload is not correctly formatted."` to `ex.Message`.

I know about ugly why of checking for existence of `Path` in the message I created [issue .NET runtime to improve it](https://github.com/dotnet/runtime/issues/95205).

Reading [doc](https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/converters-how-to?pivots=dotnet-8-0) it says to me it's normal to throw this exception from converter

### Additional context

_No response_

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.