Skip to content

Improve assertion message in PersistenceExceptionTranslationInterceptor#24484

Merged
sbrannen merged 2 commits into
spring-projects:masterfrom
michael-simons:patch-1
Feb 6, 2020
Merged

Improve assertion message in PersistenceExceptionTranslationInterceptor#24484
sbrannen merged 2 commits into
spring-projects:masterfrom
michael-simons:patch-1

Conversation

@michael-simons

Copy link
Copy Markdown
Contributor

It's a bit odd asserting the bean factory with != null and the given message.

It's a bit odd asserting the bean factory with != null and the given message.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 6, 2020
@sbrannen

sbrannen commented Feb 6, 2020

Copy link
Copy Markdown
Member

Assert.state throws an IllegalStateException which is appropriate here.

Assert.notNull throws an IllegalArgumentException which is inappropriate here, since the thing missing is not provided as an argument to the method in question.

Spring is rather consistent in that regard, when multiple properties could have been set and Spring later determines that the current state is illegal.

As for the exception message, "No XXX set" is also pretty standard across core Spring for missing properties; however, your proposal would provide more detailed information to the user and would align with the message in setBeanFactory.

So if you'd like to revert back to Assert.state, we'll consider the PR.

Cheers

@sbrannen sbrannen added status: waiting-for-feedback We need additional information before we can continue in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 6, 2020
@sbrannen sbrannen changed the title Improve assertion message. Improve assertion message in PersistenceExceptionTranslationInterceptor Feb 6, 2020
@michael-simons

Copy link
Copy Markdown
Contributor Author

Thanks for letting me know, I understand and the argument makes perfect sense. As I applied the suggested change, you see that I still like the more detailed exception message better.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Feb 6, 2020
@sbrannen sbrannen self-assigned this Feb 6, 2020
@sbrannen sbrannen added this to the 5.2.4 milestone Feb 6, 2020
@sbrannen sbrannen merged commit 711fafc into spring-projects:master Feb 6, 2020
@sbrannen

sbrannen commented Feb 6, 2020

Copy link
Copy Markdown
Member

This has been merged into master.

Thanks

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) status: feedback-provided Feedback has been provided type: enhancement A general enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants