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

ResponseStatusExceptionResolver ignores ResponseStatusException headers #24944

Closed
michael-o opened this issue Apr 20, 2020 · 6 comments
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@michael-o
Copy link

michael-o commented Apr 20, 2020

Affects: Spring Framework 5.2.5.RELEASE


When throwing a derived exception like:

	public class NotModifiedStatusException extends ResponseStatusException {

		private static final long serialVersionUID = -4380761922076567306L;

		private String etag;

		public NotModifiedStatusException(String etag) {
			super(HttpStatus.NOT_MODIFIED);
			this.etag = etag;
		}

		@Override
		public Map<String, String> getHeaders() {
			return getResponseHeaders().toSingleValueMap();
		}

		@Override
		public HttpHeaders getResponseHeaders() {
			HttpHeaders headers = new HttpHeaders();
			headers.setETag(etag);
			return headers;
		}

	}

the headers are completely ignored:

17:56:01,681 [http-nio-127.0.0.1-8081-exec-5] DEBUG o.s.w.s.m.a.ResponseStatusExceptionResolver: Resolved [...NotModifiedStatusException: 304 NOT_MODIFIED]
17:56:01,681 [http-nio-127.0.0.1-8081-exec-5] TRACE o.s.web.servlet.DispatcherServlet: No view rendering, null ModelAndView returned.
17:56:01,681 [http-nio-127.0.0.1-8081-exec-5] DEBUG o.s.web.servlet.DispatcherServlet: Completed 304 NOT_MODIFIED, headers={}

It happily ignores the headers:

protected ModelAndView resolveResponseStatusException(ResponseStatusException ex,
HttpServletRequest request, HttpServletResponse response, @Nullable Object handler) throws Exception {
int statusCode = ex.getStatus().value();
String reason = ex.getReason();
return applyStatusAndReason(statusCode, reason, response);

My naive solution seems to work:

@Component
public class HeaderAwareResponseStatusExceptionResolver extends ResponseStatusExceptionResolver {

	@Override
	public int getOrder() {
		return super.getOrder() + 1;
	}

	@Override
	protected ModelAndView resolveResponseStatusException(ResponseStatusException ex,
			HttpServletRequest request, HttpServletResponse response, Object handler)
			throws Exception {
		int statusCode = ex.getStatus().value();
		String reason = ex.getReason();
		HttpHeaders headers = ex.getResponseHeaders();

		headers.forEach((name, values) -> values.forEach(value -> response.addHeader(name, value)));

		if (!StringUtils.hasLength(reason)) {
			response.sendError(statusCode);
		} else {
			response.sendError(statusCode, reason);
		}
		return new ModelAndView();
	}

}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 20, 2020
@sbrannen sbrannen added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Apr 20, 2020
@rstoyanchev rstoyanchev self-assigned this Apr 22, 2020
@rstoyanchev
Copy link
Contributor

Support for headers was added recently in #24261 but only on the WebFlux side it seems.

@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 22, 2020
@rstoyanchev rstoyanchev added this to the 5.2.6 milestone Apr 22, 2020
@rstoyanchev rstoyanchev changed the title ResponseStatusExceptionResolver ignores ResponseStatusException.getResponseHeaders() ResponseStatusExceptionResolver ignores ResponseStatusException headers Apr 22, 2020
@michael-o
Copy link
Author

Correct, this is my observation. I assume this is simply an oversight.

@michael-o
Copy link
Author

The provided solution is in consistent with the Flux version:

if (ex instanceof ResponseStatusException) {
((ResponseStatusException) ex).getResponseHeaders()
.forEach((name, values) ->
values.forEach(value -> response.getHeaders().add(name, value)));

It does not contain a null check.

@rstoyanchev
Copy link
Contributor

Yes it is a little more defensive. Spring MVC has been around longer and is used more widely. Do you foresee an issue?

@michael-o
Copy link
Author

Not right now, but this deserves to be documented.

rstoyanchev added a commit that referenced this issue Apr 25, 2020
This caused random failures in a newly added test. Also remove defensive
check in ResponseStatusExceptionResolver.

See gh-24944
@rstoyanchev
Copy link
Contributor

I've updated it to match WebMvc.

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

No branches or pull requests

4 participants