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

Check ResponseStatusException reason as MessageSource code for ProblemDetail #30300

Closed
wants to merge 1 commit into from

Conversation

quaff
Copy link
Contributor

@quaff quaff commented Apr 7, 2023

detail in ProblemDetail will be localized
Closes GH-30222

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 7, 2023
@quaff quaff marked this pull request as draft April 7, 2023 02:00
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

The problem is that reason may or may not be a message code, and this change suppresses the default message code determination. Is there a reason you can't use the default message code, i.e. problemDetail.org.springframework.web.ErrorResponseException, or otherwise creating a dedicated sub-class also allows you to set the message code and arguments?

@rstoyanchev rstoyanchev self-assigned this Apr 11, 2023
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) status: waiting-for-feedback We need additional information before we can continue labels Apr 11, 2023
@quaff
Copy link
Contributor Author

quaff commented Apr 12, 2023

GH-30222

I'm trying to fix the inconsistency in boot spring-projects/spring-boot#34791

@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 Apr 12, 2023
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Apr 12, 2023

The referenced Boot issue was closed because the issue can't be addressed on the Boot side, so I don't know what you mean. Here I'm saying that the PR in its present form can't go forward because it assumes the reason is a message code, without any option to change that assumption, also suppressing the ability to use a consistent message code as for all other exceptions.

Whatever the solution, it would need to be much more explicit about passing a message code, but again I'm wondering whether switching to the default message code would work for you.

@quaff quaff changed the title Improve ResponseStatusException to use reason as detailMessageCode if present ResponseStatusException reason as problem detail should be updated via MessageSource Apr 13, 2023
@quaff
Copy link
Contributor Author

quaff commented Apr 13, 2023

@rstoyanchev I updated the commit that will break nothing just enhancement, please review it.

@quaff
Copy link
Contributor Author

quaff commented May 5, 2023

@rstoyanchev Could you take a look?

@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Jan 4, 2024
@rstoyanchev rstoyanchev added this to the 6.1.3 milestone Jan 4, 2024
@rstoyanchev
Copy link
Contributor

Apologies for the delay. Yes, if used as a fallback it should work.

@rstoyanchev rstoyanchev changed the title ResponseStatusException reason as problem detail should be updated via MessageSource ErrorResponse should try the ResponseStatusException reason as a MessageSource code Jan 4, 2024
@rstoyanchev rstoyanchev changed the title ErrorResponse should try the ResponseStatusException reason as a MessageSource code Check ResponseStatusException reason as MessageSource code for ProblemDetail Jan 5, 2024
@rstoyanchev
Copy link
Contributor

I have moved the change down to ResponseStatusException from ErrorResponse since it is something specific to it and doesn't need to apply to any exception.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider using reason as detailMessageCode for ResponseStatusException
3 participants