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

Improve assertion message in PersistenceExceptionTranslationInterceptor #24484

Merged
merged 2 commits into from
Feb 6, 2020

Conversation

michael-simons
Copy link
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
Copy link
Member

sbrannen commented Feb 6, 2020

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
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
Copy link
Member

sbrannen commented Feb 6, 2020

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