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

Include actual cause's message in various parsing exception messages #32636

Closed
wants to merge 3 commits into from

Conversation

remeio
Copy link
Contributor

@remeio remeio commented Apr 14, 2024

When timeoutString is "-2", DefaultTransactionDefinition#setTimeout method will throw IllegalArgumentException(it extends RuntimeException), we will get the error message: Invalid timeoutString value "-2" - cannot parse into int.

But actually "-2" can parse into int.

In other words, the error message cannot parse into int is just right for Integer#parseInt, so I change RuntimeException to NumberFormatException. So:

  • When timeoutString is "1", just ok.
  • When timeoutString is "-2", we will get: Timeout must be a positive integer or TIMEOUT_DEFAULT.
  • When timeoutString is "foo", we will get: Invalid timeoutString value "foo" - cannot parse into int.
try {
	setTimeout(Integer.parseInt(timeoutString));
}
catch (RuntimeException ex) {
	throw new IllegalArgumentException(
			"Invalid timeoutString value \"" + timeoutString + "\" - cannot parse into int");
}

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 14, 2024
@simonbasle simonbasle self-assigned this Apr 16, 2024
@sbrannen sbrannen added the in: data Issues in data modules (jdbc, orm, oxm, tx) label Apr 16, 2024
Copy link
Contributor

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

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

@remeio thanks for the PR. There is indeed a bit of an inconsistency here, but we don't think the fix you proposed is the best approach. With the current approach, we intentionally present the erroneous timeoutString in the message. It's just that the second half of the message that becomes misleading in the -2 case.

I would suggest that instead of changing the type of exception caught, you change the exception message to replace \" - cannot parse into int with \"; " + ex. That way the message will consistently reflect the actual underlying reason.

@remeio
Copy link
Contributor Author

remeio commented Apr 16, 2024

Thanks, I agree with you.

@remeio remeio requested a review from simonbasle April 16, 2024 13:17
@simonbasle simonbasle added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 16, 2024
@simonbasle simonbasle added this to the 6.1.x milestone Apr 16, 2024
Copy link
Contributor

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

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

yeah, looking better 👍
I was made aware that there are 3 similar cases in ScheduledAnnotationBeanPostProcessor as well (look for "- cannot parse into"). Would you like to also fix these with a similar change as part of this PR?

@@ -206,7 +206,7 @@ public void resolveAttributeStrings(@Nullable StringValueResolver resolver) {
}
catch (RuntimeException ex) {
throw new IllegalArgumentException(
"Invalid timeoutString value \"" + timeoutString + "\" - cannot parse into int");
"Invalid timeoutString value \"" + timeoutString + "\": " + ex);
Copy link
Contributor

Choose a reason for hiding this comment

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

let's use ; as a separator since : is already part of Throwable::toString

Suggested change
"Invalid timeoutString value \"" + timeoutString + "\": " + ex);
"Invalid timeoutString value \"" + timeoutString + "\"; " + ex);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will fix these cases together

@simonbasle simonbasle added type: bug A general bug and removed type: enhancement A general enhancement labels Apr 16, 2024
@simonbasle
Copy link
Contributor

(marking as type: bug on @jhoeller suggestion, albeit a mild one: the original exception should percolate through to the user one way or another but here it is entirely swallowed / masked)

@simonbasle simonbasle changed the title fix: Modify the exception type to NumberFormatException. Include actual cause's message in various parsing exception messages Apr 16, 2024
@simonbasle simonbasle added the in: core Issues in core modules (aop, beans, core, context, expression) label Apr 16, 2024
Copy link
Contributor

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

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

thanks for the PR @remeio 👍

@simonbasle simonbasle modified the milestones: 6.1.x, 6.1.7 Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) in: data Issues in data modules (jdbc, orm, oxm, tx) type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants