In a reactive web app, SslBundle can no longer open store file locations without using a 'file:' prefix #44535
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
nosan
force-pushed
the
43953
branch
2 times, most recently
from
02498ec to
60548aa
Compare
…tResource Prior to this update, FilteredReactiveWebContextResource was not considered when preferFileResolution was set to true. This commit updates the ApplicationResourceLoader to include support for FilteredReactiveWebContextResource. Signed-off-by: Dmytro Nosan <[email protected]>
| @@ -132,8 +132,8 @@ public static ResourceLoader get(ResourceLoader resourceLoader) { | |||
| * class loader at the time this call is made. | |||
| * @param resourceLoader the delegate resource loader | |||
| * @param preferFileResolution if file based resolution is preferred over | |||
| * {@code ServletContextResource} or {@link ClassPathResource} when no resource prefix | |||
| * is provided. | |||
| * {@code ServletContextResource}, {@code FilteredReactiveWebContextResource} or | |||
There was a problem hiding this comment.
I had a look to this and, unfortunately, I don't think that we can use that option as I realize now that it's an implementation detail (package private on top of it). This seems a bit fragile that we rely on this, compared to the standard ServletContextResource. I am still digging what our options are.
There was a problem hiding this comment.
Resource implementation that replaces the {@link org.springframework.web.context.support.ServletContextResource} in a reactive web application.
org.springframework.boot.web.reactive.context.FilteredReactiveWebContextResource is a
part of Spring Boot codebase, so its ServletContextResource but for Reactive.
This seems a bit fragile that we rely on this, compared to the standard ServletContextResource. I am still digging what our options are.
You're right. Unfortunately, it is. With the introduction of ResourceLoader delegation in #42835, followed by the regression fix in #43274, the ApplicationResourceLoader has become a bit complex and fragile overall.
snicoll
added
the
for: team-attention
philwebb
added
type: bug
Update `ApplicationResourceLoader` so that the `preferFileResolution` flag now also supports `FilteredReactiveWebContextResource`. See gh-44535 Signed-off-by: Dmytro Nosan <[email protected]>
Refactor `ApplicationResourceLoader` so that `preferFileResolution` is supported using a new `ResourceFilePathResolver` strategy interface. This update allows us to locate `ResourceFilePathResolver` implementations in more suitable packages that are closer to the code that they support. See gh-44535
|
Thanks @nosan! Since |
|
Thanks @philwebb I really like the idea of adding a dedicated What do you think about changing the return type to |
|
I also feel that |
|
@nosan Thanks for the suggestions. How do you feel about commit 3531071? I tried changing the return type as well but it made all the implementations a little more awkward since they all needed to implement this logic. |
|
Thanks @philwebb!
I think the nested interface is a great idea. I really like it.
Oh, I completely overlooked One potential option could be to change the return type to either public interface ApplicationResourceResolver {
ApplicationResource resolve(String location, Resource resource);
}However, in such a case, |
|
I went though a similar thought process, but I agree that an extra public class is not ideal. I also considered returning a Given that this stuff is somewhat internal, I think what we have is probably the least bad option. Thanks for the extra review! |
See #43953