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

ContentType is null for respone that handles server errors (for example NoHandlerFoundException) and that breaks SiteMesh decorating #34736

Closed
MarekPribulaBOL opened this issue Apr 10, 2025 · 6 comments
Assignees
Labels
for: external-project Needs a fix in external project in: web Issues in web modules (web, webmvc, webflux, websocket) status: invalid An issue that we don't feel is valid

Comments

@MarekPribulaBOL
Copy link

MarekPribulaBOL commented Apr 10, 2025

Our setup:

  • SpringBoot 3.4.4 (recently updated from 3.4.3) with spring-boot-starter-web
  • we are using jsp as view files
  • we are using spring-boot-starter-sitemesh in version 3.2.1

After update to 3.4.4 we are experiencing problem during rendering of our error page or even the pages after the error is process by DispatcherServlet.

Problem is that because of the ticket #34366, DispatcherServlet has been modified that it sets up ContentType to null. At this point HttpServletResponseBuffer is called, which tries to determine if buffering (decorating) should be applied or not. Since ContentType is now null, it decides against decorating, which ends with blank pages with text.

Modification in DispatcherServlet>

@Nullable
protected ModelAndView processHandlerException(HttpServletRequest request, HttpServletResponse response,
		@Nullable Object handler, Exception ex) throws Exception {

	// Success and error responses may use different content types
	request.removeAttribute(HandlerMapping.PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE);
	// Reset the response content-type header and body buffer if the response is not committed already,
	// leaving the other response headers in place.
	try {
		response.setHeader(HttpHeaders.CONTENT_TYPE, null);
		response.resetBuffer();
	}
	catch (IllegalStateException illegalStateException) {
		// the response is already committed, leave it to exception handlers anyway
	}
...

If this is a desired feature for exception handling, please how would I get some ContentType into my response during such case?

If this is desired just for async behaviour, maybe add some swich before whole try-catch.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 10, 2025
@jhoeller jhoeller added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Apr 10, 2025
@bclozel bclozel self-assigned this Apr 10, 2025
@bclozel
Copy link
Member

bclozel commented Apr 10, 2025

This was an intentional change, but looking at your description it's not clear why the error handling part in your case is not setting the content type. Can you share a minimal sample application that reproduces the problem?

@bclozel bclozel added the status: feedback-provided Feedback has been provided label Apr 10, 2025
@MarekPribulaBOL
Copy link
Author

I am adding simple app.

Currently it is using Spring Boot 3.4.4. When calling localhost:8083/demo/test view will be rendered also with header and footer provided by sitemesh. When trying something else e.g. localhost:8083/demo/form, error page is shown, but without this header and footer.

When the Spring Boot version is changed to 3.4.3, also error page contains header and footer.

I see in browser that response for error page has ContenType in both 3.4.4 and 3.4.3, but this comes from View. SiteMesh however creates a wrapper around HttpResponse and when setHeader is called, it listens specifically for ContentType header. I tried to debug even more, but I did not see any other call setsHeader.

demo.zip

@bclozel
Copy link
Member

bclozel commented Apr 11, 2025

Thanks for the sample @MarekPribulaBOL , this helped me better understand the situation.

In #34366, we remove the Content-Type response header by using response.setHeader("Content-Type", (String)null); as this is the standard way in Servlet to remove a header. At this stage, when an error happens, we need to reset this header because the error handling phase might use a different Content-Type for the error response. In this case, I still think this change makes sense.

I debugged your sample and noticed the following:

  • the "text/html" Content-Type is always set by the JSP page
  • after being removed by org.springframework.web.servlet.DispatcherServlet#processHandlerException, the Content-Type is set again to "text/html" by the error.jsp page and buffering is activated.

At this point, I'm leaning towards this being a bug in sitemesh where the error handling part should be adapted. I don't know much about the inner workings of sitemesh. Could you maybe create an issue on the Sitemesh side? I'd be happy to assist there. I'll close this issue for now because I don't think this use case shows a problem with the way we handle errors in Spring.

Thanks!

@bclozel bclozel closed this as not planned Won't fix, can't repro, duplicate, stale Apr 11, 2025
@bclozel bclozel added status: invalid An issue that we don't feel is valid for: external-project Needs a fix in external project and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Apr 11, 2025
@MarekPribulaBOL
Copy link
Author

Thanks for looking into it @bclozel, I have opened a ticket for SiteMesh as recommended.

@bclozel
Copy link
Member

bclozel commented Apr 11, 2025

Thanks @MarekPribulaBOL

For what it's worth, I suspect that Sitemesh doesn't expect the content-type to be set several times for a single request/response exchange and that setting the content-type header to null (in this case, to remove it) disables buffering. It seems that re-enabling it later doesn't work, maybe this is not designed with this use case in mind.

I have locally patched org.sitemesh.webapp.contentfilter.BasicSelector with the following and the SiteMesh header/footer is displayed for 404s. Hope that helps

	public boolean shouldBufferForContentType(String contentType, String mimeType, String encoding) {
		if (mimeType == null) {
			// workaround to avoid disabling buffering when the content-type header is removed
			return true;
		} else {
			for(String mimeTypeToBuffer : this.mimeTypesToBuffer) {
				if (mimeTypeToBuffer.equalsIgnoreCase(mimeType)) {
					return true;
				}
			}

			return false;
		}
	}

@MarekPribulaBOL
Copy link
Author

Thanks @bclozel,

I have actually went with different approach and temporarily patched DispatcherServlet, since we are using embeddedTomcat and we have not faced so far any problem with duplicated ContentType in case of an error.

But yes, this is also an possibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: external-project Needs a fix in external project in: web Issues in web modules (web, webmvc, webflux, websocket) status: invalid An issue that we don't feel is valid
Projects
None yet
Development

No branches or pull requests

4 participants