improve performance of alias checking #8433
Conversation
Signed-off-by: Lachlan Roberts <[email protected]>
| @@ -1967,7 +1967,7 @@ public boolean checkAlias(String path, Resource resource) | |||
| LOG.debug("Aliased resource: {}~={}", resource, resource.getAlias()); | |||
|
|
|||
| // alias checks | |||
| for (AliasCheck check : getAliasChecks()) | |||
There was a problem hiding this comment.
The reason this existed this way was to support folks that wanted to have adjustable (at runtime) list of alias checks.
There was a problem hiding this comment.
Hmm I thought alias checks could only be added before start but looks like that is not the case.
So maybe we do still need the CopyOnWriteArrayList, but we don't need to use getAliasChecks() for the unmodifiable list here.
There was a problem hiding this comment.
I don't have a strong opinion on the solution you are working towards.
Just explaining the history of why things are the way they are in Jetty 11 and earlier.
Up to you to decide if this behavior is worth keeping around or changing for Jetty 12 tho.
In regards to Jetty 12 effort though ...
To add to this, there once was a desire to have different Alias checks depending on url-pattern.
So that you could reject aliases for say /archives/* and allow aliases for /avatars/*.
Frankly, only the Jetty 12 ResourceService should have the alias check lists, as that's where all of our serve-static-content lies now (for all EE levels). The uses of say ServletContext.getResource(String) should allow anything that is requested from it as that's programmatic from Servlet APIs, not something that a user request can trigger. It's up to the webapp developer to decide how to sanitize / evaluate if sane the user request before they call ServletContext.getResource(String). (Example: Jersey and Spring serve static content themselves, not via the DefaultServlet / ResourceHandler. But spark uses the ResourceHandler.)
There was a problem hiding this comment.
I'm not convinced we need to allow getAliasChecks to be overridable, as I don't really see the use-case for a dynamic list, nor is there any context passed on which to make that choice. So I'm OK with this change..... but equally we could assign a single immutable list as a field and return that.
Signed-off-by: Lachlan Roberts <[email protected]>
We don't want to create the unmodifiableList every time we check an alias, so use
_aliasChecksdirectly instead of using the getter.Also I don't think we need theCopyOnWriteArrayListbecause we do not allow modifications after theContextHandlerhas been started.