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

Use JsonIOException for unchecked exceptions instead of RuntimeException in ConstructorConstructor #2771

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

panic08
Copy link
Contributor

@panic08 panic08 commented Oct 29, 2024

In the newDefaultConstructor method in ConstructorConstructor we throw RuntimeException when Failed to invoke constructor. However, as noted in the TODO, we should throw something specific like JsonIOException when an unchecked exception occurs. In this method we already throw JsonIOException on similar exceptions. I believe this change will give more readability, but more importantly, more semantic precision

Copy link
Collaborator

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

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

but more importantly, more semantic precision

But only due to using a Gson-specific exception instead of a generic RuntimeException. JsonIOException is still kind of 'wrong' for this because these issues are not IO related.

So I am a bit indecisive on this change. The underlying issue is that Gson has no proper exception hierarchy (#2359), and I am not sure if that can even be solved without breaking backward compatibility.

@eamonnmcmanus, what is your opinion?


Could you please adjust this test though to expect a JsonIOException now and remove the TODO comment there (I hope that is the only case):

RuntimeException.class, () -> gson.fromJson("{}", ClassWithThrowingConstructor.class));

@panic08 panic08 force-pushed the use-JsonParseException-in-ConstructorConstructor branch from cde8922 to e4471d0 Compare October 30, 2024 08:32
@panic08
Copy link
Contributor Author

panic08 commented Oct 30, 2024

I agree that JsonIOException doesn't quite fit since it's typically used for IO related issues, but in my opinion it provides a bit more context than RuntimeException while we expect a more refined exception hierarchy in Gson (as discussed in #2359)

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.

2 participants