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

Add trailing slash to static resource location if missing #33817

Conversation

cagliostro92
Copy link

As already stressed by @philwebb in #33815 and by my colleague @milazzo-g, the brand new assertion about the required trailing slash can lead to important breaking changes in both custom project and libraries. As mentioned by the author in #33712:

A location that does not end on "/" does not actually work as intended, which is probably why it hasn't been reported as an issue. We should append the slash where we can, e.g. String location values, and assert Resource locations.

It seems to be clear that this constraint was intended to avoid unexpected behaviour, but the framework can still be accountable for adding the trailing slash without breaking other projects.

closes: #33815

@pivotal-cla
Copy link

@cagliostro92 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@cagliostro92 Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 30, 2024
@cagliostro92 cagliostro92 force-pushed the 33815-resource-handler-trailing-slash branch from dc3ea16 to 0b7ebb8 Compare October 30, 2024 10:51
@bclozel bclozel self-assigned this Oct 31, 2024
@cagliostro92 cagliostro92 force-pushed the 33815-resource-handler-trailing-slash branch from 0b7ebb8 to becc24c Compare October 31, 2024 07:36
@cagliostro92
Copy link
Author

Hi @bclozel,
I would like to bring to your attention that the offending assertion is still present and invoked in ResourceHttpRequestHandler and PathResourceLookupFunction.
Let me know how you prefer handling such cases.
PS: I've rebased my branch onto your main a few minutes ago, so if you have already downloaded it, please drop it.

@bclozel
Copy link
Member

bclozel commented Oct 31, 2024

Thanks for the proposal, but I'm declining this one in favor of #33815 (comment)

@bclozel bclozel closed this Oct 31, 2024
@bclozel bclozel added in: web Issues in web modules (web, webmvc, webflux, websocket) status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Oct 31, 2024
@rstoyanchev
Copy link
Contributor

Re-opening as per discussion under #33826.

@rstoyanchev rstoyanchev reopened this Oct 31, 2024
@rstoyanchev rstoyanchev self-assigned this Oct 31, 2024
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: declined A suggestion or change that we don't feel we should currently apply labels Oct 31, 2024
@rstoyanchev rstoyanchev added this to the 6.2.0 milestone Oct 31, 2024
@rstoyanchev rstoyanchev changed the title Fixed the trailing slash handling during resource location Add trailing slash to static resource location if missing Oct 31, 2024
@cagliostro92 cagliostro92 force-pushed the 33815-resource-handler-trailing-slash branch from becc24c to 7cb8a4f Compare November 4, 2024 10:14
@rstoyanchev
Copy link
Contributor

Thanks for the PR, and I will apply equivalent changes, but I need to start in 6.1.x where the code is different, and then forward merge to main.

@rstoyanchev rstoyanchev closed this Nov 6, 2024
@rstoyanchev rstoyanchev removed this from the 6.2.0 milestone Nov 6, 2024
@rstoyanchev rstoyanchev added the status: superseded An issue that has been superseded by another label Nov 6, 2024
@cagliostro92 cagliostro92 deleted the 33815-resource-handler-trailing-slash branch November 6, 2024 09:09
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) status: superseded An issue that has been superseded by another type: enhancement A general enhancement
Projects
None yet
5 participants