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

RpcException API improvements #1992

Merged
merged 4 commits into from
Dec 20, 2022

Conversation

jtattermusch
Copy link
Contributor

Addresses grpc/grpc#27066 and grpc/grpc#25740 (in a backward compatible way).

Two options for addressing the problem in Grpc.Core as well:

  • if users pull the latest versions of Grpc.Core.Api (which now lives in the grpc-dotnet repo), they'll get the fix as well. It is fine to use Grpc.Core at e.g. 2.46.5 (currently the latest patch) and Grpc.Core.Api 2.47.x, 2.48.x, ... together.
  • we could potentially backport into grpc/grpc's 1.46.x branch (which seems unnecessary due to option 1 though).

@jtattermusch
Copy link
Contributor Author

CC @mgravell @tonydnewell

@@ -88,7 +88,8 @@ public override string ToString()
{
if (DebugException != null)
{
return $"Status(StatusCode=\"{StatusCode}\", Detail=\"{Detail}\", DebugException=\"{DebugException}\")";
return $"Status(StatusCode=\"{StatusCode}\", Detail=\"{Detail}\"," +
$" DebugException=\"{DebugException.GetType().Name}: {DebugException.Message}\")";
Copy link
Contributor

Choose a reason for hiding this comment

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

a slightly odd choice to show the type name - especially as concrete exception types are often internal, but: not invalid - just me adding a "are you 100% sure you want this?" check

Copy link
Member

Choose a reason for hiding this comment

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

Showing the type name is normal in Exception.ToString()

Copy link
Contributor

@mgravell mgravell left a comment

Choose a reason for hiding this comment

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

minor thought added re text; but: I wholeheartedly support the change (and populating the inner exception), 👍

@@ -88,7 +88,8 @@ public override string ToString()
{
if (DebugException != null)
{
return $"Status(StatusCode=\"{StatusCode}\", Detail=\"{Detail}\", DebugException=\"{DebugException}\")";
return $"Status(StatusCode=\"{StatusCode}\", Detail=\"{Detail}\"," +
$" DebugException=\"{DebugException.GetType().Name}: {DebugException.Message}\")";
Copy link
Member

Choose a reason for hiding this comment

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

Showing the type name is normal in Exception.ToString()

src/Grpc.Core.Api/Status.cs Outdated Show resolved Hide resolved
test/Grpc.Core.Api.Tests/RpcExceptionTest.cs Outdated Show resolved Hide resolved
test/Grpc.Core.Api.Tests/RpcExceptionTest.cs Outdated Show resolved Hide resolved
Co-authored-by: James Newton-King <james@newtonking.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants