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

Provide RequestPredicate with no side effects #25814

Open
tt4g opened this issue Sep 25, 2020 · 5 comments
Open

Provide RequestPredicate with no side effects #25814

tt4g opened this issue Sep 25, 2020 · 5 comments
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: feedback-provided Feedback has been provided status: pending-design-work Needs design work before any code can be developed type: enhancement A general enhancement

Comments

@tt4g
Copy link

tt4g commented Sep 25, 2020

I'm developing a Spring WebFlux application that serve Single Page Application front-end resources.
I have registered front-end resources in the ResourceHandlerRegistry for distribution, but some of them have had to change the Cache-Control header.

The ResourceHandlerRegistry cannot dynamically set the response headers according to the request URL or file extension.
I figured this goal could be accomplished with the WebFilter as well as the Servet API.

I used WebFilter in WebFlux to check the HTTP request and tried to change the HTTP header of the response, but the
I've noticed that WebFlux has different caveats than the Servlet API.
e.g, when I was getting a path from ServerHttpRequest, I had to call request.getPath().pathWithinApplication() or else the context path would be included.

Spring MVC provided an AntMatcher, so I looked for Spring WebFlux to provide a useful matcher as well, so I found the RequestPredicate, but this is a class for the RouterFunction and seems to have some side effects.
e.g, RequestPredicates#path(String) rewrites the path variable in the request.

I then discovered that Spring Security had a ServerWebExchangeMatchers with no side effects, and I implemented my own matcher based on its implementation.

But this is reinventing the wheel.

I was referring to the Spring Security implementation in this case, but I think it would be a time saver for someone like me who wants to do conditional processing with WebFilter, if a similar feature was provided as part of Spring WebFlux.

The following code is a sample of my WebFilter implementation.
my.application.matcher.ServerWebExchangeMatcher is a matcher based on Spring Security ServerWebExchangeMatchers.

package my.application;

import java.util.List;

import my.application.matcher.ServerWebExchangeMatcher;

import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpStatus;
import org.springframework.http.server.reactive.ServerHttpResponse;
import org.springframework.web.server.ServerWebExchange;
import org.springframework.web.server.WebFilter;
import org.springframework.web.server.WebFilterChain;
import reactor.core.publisher.Mono;

public class NoCacheWebFilter implements WebFilter {

    public static final String CACHE_CONTROL_VALUE = "no-cache, no-store, max-age=0, must-revalidate";

    public static final String PRAGMA_VALUE = "no-cache";

    public static final String EXPIRES_VALUE = "0";

    private final ServerWebExchangeMatcher serverWebExchangeMatcher;

    public NoCacheWebFilter(
        ServerWebExchangeMatcher serverWebExchangeMatcher) {

        this.serverWebExchangeMatcher = serverWebExchangeMatcher;
    }

    @Override
    public Mono<Void> filter(ServerWebExchange exchange, WebFilterChain chain) {
        if (this.serverWebExchangeMatcher.matches(exchange)) {
            exchange.getResponse().beforeCommit(() ->
                Mono.fromRunnable(() -> writeNoCacheHeader(exchange)));
        }

        return chain.filter(exchange);
    }

    private void writeNoCacheHeader(ServerWebExchange exchange) {
        ServerHttpResponse response = exchange.getResponse();

        if (response.getStatusCode() == HttpStatus.NOT_MODIFIED) {
            return;
        }

        HttpHeaders responseHeaders = response.getHeaders();
        responseHeaders.put(HttpHeaders.CACHE_CONTROL, List.of(CACHE_CONTROL_VALUE));
        responseHeaders.put(HttpHeaders.PRAGMA, List.of(PRAGMA_VALUE));
        responseHeaders.put(HttpHeaders.EXPIRES, List.of(EXPIRES_VALUE));
    }

}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 25, 2020
@tt4g tt4g changed the title Provide RequestPredicate with no side effects. Provide RequestPredicate with no side effects Sep 25, 2020
@rstoyanchev
Copy link
Contributor

The ResourceHandlerRegistry cannot dynamically set the response headers according to the request URL or file extension.

ResourceHandlerRegistry allows configuring different CacheControl settings by URL pattern. I'm wondering where does that fall short, can something be improved there?

I've noticed that WebFlux has different caveats than the Servlet API.
e.g, when I was getting a path from ServerHttpRequest, I had to call request.getPath().pathWithinApplication() or else the context path would be included.

I don't understand which part is the caveat. It gives you choice, so you can use what you need.

Spring MVC provided an AntMatcher, so I looked for Spring WebFlux to provide a useful matcher as well, so I found the RequestPredicate, but this is a class for the RouterFunction

WebFlux uses PathPatterns parsed with PathPatternParser. This has a number of advantages over AntPathMatcher which is why Spring MVC is also adopting use of the same in 5.3 (blog post for more context). If path pattern matching is what you need then use PathPatternParser.

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Sep 25, 2020
@tt4g
Copy link
Author

tt4g commented Sep 25, 2020

Thank you for your reply. I am not a native speaker, so please ask for details if you have difficulty understanding.

ResourceHandlerRegistry allows configuring different CacheControl settings by URL pattern. I'm wondering where does that fall short, can something be improved there?

In my case, there are relatively large files such as font files and Javascript frameworks in static resources.
I want these files to be cached in order to reduce browser communication costs.
These files are bundled with Webpack.
A content hash is added to the file names, so it doesn't matter if these files are updated.

On the other hand, I had to exclude some .html files from the directory where these files are stored.
An html file cannot be given a content hash to support bookmarks.
If the browser caches these files, it will not be able to serve the updated content to the user until something like a super reload is performed.

I am aware that ResourceHandlerRegistry does not support the ability to change the cache control for static files in the same directory depending on the directory path and extension.

Caching controls using spring.resources.chain.strategy.content.* are also unlikely to be available, as legacy web browsers must be supported.

I don't understand which part is the caveat. It gives you choice, so you can use what you need.

This is my problem, but after creating my own matcher, I noticed that the URIs don't match.
The cause was that the context path at the beginning of the URI did not match.
I didn't know about pathWithinApplication() and it took me a few hours to solve the problem.

WebFlux uses PathPatterns parsed with PathPatternParser. This has a number of advantages over AntPathMatcher which is why Spring MVC is also adopting use of the same in 5.3 (blog post for more context). If path pattern matching is what you need then use PathPatternParser.

That's right.

However, in order to do URI matching with PathPatternParser, I had to know about the features like pathWithinApplication(), which I don't usually use.

While digging through the source code and test code to find out how to use the PathPatternParser, I discovered the ServerWebExchangeMatcher and knew that the code I was developing was the reinventing the wheel.

After finding a helpful implementation, I quickly achieved my goal.
But the code I developed is code that already exists as part of the Spring Framework.

I opened this issue to see if this could be provided as part of the Spring Framework to make it easier for users to use.

This request may be a niche one.
If you do not see the need to provide it from the Spring Framework, you are free to close the issue.

Regards, all of our contributors.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 25, 2020
@tt4g
Copy link
Author

tt4g commented Sep 28, 2020

I've found that my goal can be achieved without custom WebFilter.

Since the only file that needs to be excluded from the cache is the .html extension, it works as a workaround by calling ResourceHandlerRegistry#addResourceHandler("/**/*.html") first.

@rstoyanchev Thank you for your kindness.

@tt4g tt4g closed this as completed Sep 28, 2020
@tt4g
Copy link
Author

tt4g commented Sep 28, 2020

I'm sorry. "/**/*.html" pattern will not work with WebFlux due to the PathPattern specification. That is my mistake.

The specs clearly stated that the ** in the middle of the path would not be matched:

* Notable behavior difference with {@code AntPathMatcher}:<br>
* {@code **} and its capturing variant <code>{*spring}</code> cannot be used in the middle of a pattern
* string, only at the end: {@code /pages/{**}} is valid, but {@code /pages/{**}/details} is not.

I still need the custom WebFilter for my purposes.

Reopen this issue, but close the issue once you have determined that you do not need to provide this feature in Spring Framework.

@tt4g tt4g reopened this Sep 28, 2020
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Sep 30, 2020

Indeed there may be room for improvement. The syntax for PathPattern can match 0 or more path segments at the end, e.g. "/resources/**" and it can match by path extension, e.g. "/resources/*.html", but there is no way to combine the two and match by path extension at any depth.

For the time being as a workaround you could put HTML files under a common base path, e.g. "/html/**" or depending on the depth add a few of those, e.g. addResourceHandler("/*/*.html", "/*/*/*.html", "/*/*/*/*.html").

Now if we could come up with some syntax that would allow to express the above without necessarily going back to allowing "**" anywhere in the middle of a pattern.

/cc @bclozel @aclement

@rstoyanchev rstoyanchev added status: pending-design-work Needs design work before any code can be developed in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement labels Sep 30, 2020
@snicoll snicoll removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 25, 2023
@snicoll snicoll added this to the General Backlog milestone Sep 25, 2023
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: feedback-provided Feedback has been provided status: pending-design-work Needs design work before any code can be developed type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants