Skip to content

Alignment of MimeMappings with Tomcat's defaults #32101

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

Closed
wants to merge 1 commit into from

Conversation

terminux
Copy link
Contributor

Hi, this PR is used to solve #31171 .

  • Migrate default mime mappings maintained in MimeMappings into properties file

  • Load org/apache/catalina/startup/MimeTypeMappings.properties and assert that each entry has an override.

Closes gh-31171

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 17, 2022
@philwebb philwebb added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 19, 2022
@philwebb philwebb added this to the 3.0.x milestone Aug 19, 2022
@snicoll snicoll added the for: merge-with-amendments Needs some changes when we merge label Aug 19, 2022
@snicoll
Copy link
Member

snicoll commented Aug 19, 2022

We need to double check that mime-mappings.properties is covered by a resource hint or we won't be able to load the file. I wonder if it would be better to update what we have (list in code) and then have the loading of the file only in the test.

@terminux
Copy link
Contributor Author

Thanks for your feedback @snicoll . I've also considered updating the list directly in the code, but since there are so many mappings (1000+), maybe it's easier to maintain using a properties file.

@bclozel
Copy link
Member

bclozel commented Aug 19, 2022

Also did we consider Andy's comment in the linked issue?

There may be a more memory efficient way to do it then simply updating the static map in MimeMappings.

@philwebb
Copy link
Member

There may be a more memory efficient way to do it then simply updating the static map in MimeMappings.

I think that may have referred to the idea that loading properties from a file might be better than having a large static block in the class. One advantage of the file is we could load it on demand then let it get garbage collected.

Another, more complex idea would be to override findMimeMapping and findMimeMappings in org.apache.catalina.Context. We could keep the most common mappings in memory and only load the full list if a more exotic extension is requested.

Either way, I generally think having the mappings in a file opens up more options to us.

@philwebb philwebb self-assigned this Aug 20, 2022
philwebb pushed a commit that referenced this pull request Aug 23, 2022
Update `MimeMappings` to align with the default values used in
Tomcat. The mime mapping are now loaded from a properties file
and a test has been added to ensure that they remain in sync
with Tomcat's defaults.

See gh-32101
philwebb added a commit that referenced this pull request Aug 23, 2022
Refine `MimeMappings` so that common default mappings are included
by default and the complete set is only loaded when needed.

The `TomcatServletWebServerFactory` has been updated so that if
`TomcatEmbeddedContext` is in use the mime mapping are used
directly rather than being copied to another Map.

The `AbstractServletWebServerFactory` class has also been changed
to use a lazy copy of the mappings. This should mean that the
complete set of properties is only loaded if the user mutates
the mappings.

See gh-32101
@philwebb philwebb closed this in 90b68d8 Aug 23, 2022
@philwebb
Copy link
Member

Thanks @terminux, this is now in main along with some refinements that should hopefully reduce memory usage for Tomcat users that aren't doing anything exotic.

@philwebb philwebb modified the milestones: 3.0.x, 3.0.0-M5 Aug 23, 2022
philwebb added a commit that referenced this pull request Aug 23, 2022
Update COMMON_MAPPINGS to align with the properties file and change
the test so that it consistently works.

See gh-32101
philwebb added a commit that referenced this pull request Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider alignment of MimeMappings with Tomcat's defaults
5 participants