Fixed Unnecessary Computation Issue Caused by String.format()
#3444
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
dukbong
changed the title
Assert methods.
dukbong
changed the title
dukbong
changed the title
String.format()
There was a problem hiding this comment.
Excessive switching from if statements that throw an exception into Assert utility usage is counterintuitive in many places. There are also occurrences that change the thrown exception type.
It makes sense to revisit Assert usages in combination with String.format to defer message creation. However, in places that explicitly throw exceptions, we would like to keep the code as-is.
Once an exception is being thrown, we need to construct the message and that is a fine arrangement already.
| } | ||
|
|
||
| throw new IllegalStateException(String.format("No annotated query found for query method %s", getName())); | ||
| Assert.notNull(query, () -> String.format("No annotated query found for query method %s", getName())); |
There was a problem hiding this comment.
This change changes behavior into throwing IllegalArgumentException instead of IllegalStateException. At that point, throwing an explicit IllegalStateException is fine so we should not change this method.
|
|
||
| throw new IllegalArgumentException( | ||
| String.format("%s managed by more than one EntityManagers: %s", type.getName(), candidates)); | ||
| Assert.isTrue(candidates.size() == 1, () -> String.format("%s managed by more than one EntityManagers: %s", type.getName(), candidates)); |
There was a problem hiding this comment.
One thought for here: The control flow is being replaced with calls to Assert. While assertions are fine for method-entry checks, usage here is kind-of unexpected. Also, we're always creating a capturing lambda leading to a bit more GC pressure.
| if (this.idType == null) { | ||
| throw new IllegalStateException("Cannot resolve Id type from " + type); | ||
| } | ||
| Assert.notNull(this.idType, () -> String.format("Cannot resolve Id type from %s", type)); |
There was a problem hiding this comment.
This change changes IllegalStateException to IllegalArgumentException
| if (mi == null) | ||
| throw new IllegalStateException( | ||
| "No MethodInvocation found: Check that an AOP invocation is in progress, and that the " | ||
| Assert.notNull(mi, "No MethodInvocation found: Check that an AOP invocation is in progress, and that the " |
There was a problem hiding this comment.
This change changes behavior into throwing IllegalArgumentException instead of IllegalStateException.
I apologize for negatively impacting the open-source project management. Before sending the PR, I will ensure that it is thoroughly checked multiple times. I'm sorry again for the inconvenience caused. |
|
|
First of all, do not worry, and there's no need to apologize. We're happy to provide guidance. Please also note that you can force-push into a pull request until the final merge so that you can reuse already open pull requests. |
|
Oh, I wasn't aware of that. I have now created a separate PR for the parts that urgently need to be fixed. I'll keep this in mind and use it more effectively going forward. |
This PR improved the code by using Assert methods instead of throwing IllegalArgumentException in if-else statements.
33eb67a : fixed an issue where the computation was processed immediately due to instant evaluation, regardless of the Assert outcome.
You have read the Spring Data contribution guidelines.
You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
You submit test cases (unit or integration tests) that back your changes.
You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).