Bug 62669 - ResponseIncludeWrapper.getContentType() never returns NULL and sets the field
Summary: ResponseIncludeWrapper.getContentType() never returns NULL and sets the field
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.5.x-trunk
Hardware: All All
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-09-01 13:51 UTC by Sven
Modified: 2018-09-03 19:22 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sven 2018-09-01 13:51:49 UTC
Hello,

according to the documentation org.apache.catalina.ssi.ResponseIncludeWrapper.getContentType() should return NULL if the content type is not known. However a fallback to "application/x-octet-stream" is implemented and it actually sets the content type field of the object.

I had a quick look into the SVN repository and it seems this code was introduced in or before 2006. Because this was 12 years ago and because I could not find any report regarding this issue, I guess everyone else can work with that and only the documentation needs to be adapted.

However I actually prefer to get NULL. First of all for me it is bad practice to set a value in a getter (if it is not called "lazy..."). But let me try to explain the real problem.

The control flow of SSIFilter is:
* ...
* Wrap the actual response with ResponseIncludeWrapper
* Continue and complete the filter chain
* Check the content type
* ...

The problem is when the filter chain is continued and completed, other filters that get the content type from the response (ResponseIncludeWrapper) actually change the content type of the response even though this is most likely NOT what the other filters want to do. It happens by accident, just by calling getContentType.

Also some filters rely on the fact that the getContentType returns NULL if it is not known, in order to check if they should set it on their own. They never set the content type because with the current implementation it always return a value other than NULL.

Best regards,
Sven.
Comment 1 Mark Thomas 2018-09-01 18:43:13 UTC
Going back this far in svn requires a little knowledge of the project history and how the repos have been structured over time.

There was a big re-organisation between 5.5.x and 6.0.x. The files were copied without history so if you want to go back further than 6.0.x you need to look into 5.5.x (then 5.0.x and then 4.1.x).

Looking in 5.5.x shows that this change was part of a patch submitted under bug 33106. Next step is to check that bug, review the patch and see if there is a reason for this change.
Comment 2 Mark Thomas 2018-09-03 15:39:29 UTC
I'm currently struggling to see why ResposneIncludeWrapper goes to the trouble it does to capture the content type and the last modified date.

I worry I am missing something so I want to look at this some more but I am currently leaning towards removing all of that code and simply obtaining the content type and last modified date from the response object. That will mean handling a potential NPE but that is one line of code vs rather more in the current implementation.
Comment 3 Sven 2018-09-03 17:46:29 UTC
I am not a maintainer or contributor to this project yet but I also suggestion to remove the wrapper.

Currently I simply add another filter after SSIFilter that wraps the ResponseIncludeWrapper again and return the original response's content type as a work-around for this issue. So far it works well but to be honest I did no in-depth testing.
Comment 4 Mark Thomas 2018-09-03 19:22:52 UTC
Fixed in:
- trunk for 9.0.12 onwards
- 8.5.x for 8.5.34 onwards
- 7.0.x for 7.0.91 onwards

I've removed the Content-Type processing from the wrapper.

I left the last modified processing as it did serve a purpose (it saved looking for and parsing the header to pass the value to the SSIProcessor).