Optimize urlRewrite #11600 #11601
Optimize urlRewrite #11600 #11601
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes the URL rewriting logic in the virtual host handling code. The changes improve performance by streamlining the URI rewriting algorithm and eliminating redundant operations.
Changes:
- Refactored
VirtualHostIdProvidersMappingto useImmutableSetand eliminate duplicate storage of the default ID provider - Optimized
rewriteUrimethod with more efficient string matching and path handling logic - Added comprehensive test coverage for edge cases including trivial vhost configurations and short URI scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| VirtualHostIdProvidersMapping.java | Simplified data structure by storing default ID provider only once in the ID provider keys set using ImmutableSet.Builder |
| ServletRequestUrlHelper.java | Refactored URL rewriting logic with optimized string operations, modern Java switch expressions, and improved path normalization |
| ServletRequestUrlHelperTest.java | Added new test cases for trivial vhost configurations and edge cases with short URIs |
| this.idProviderKeys = IdProviderKeys.from( | ||
| ImmutableSet.<IdProviderKey>builder().add( builder.defaultIdProvider ).addAll( builder.idProviderKeys.build() ).build() ); |
There was a problem hiding this comment.
Potential NullPointerException if builder.defaultIdProvider is null. The .add() method on ImmutableSet.Builder will throw an NPE if passed a null value. Consider validating that defaultIdProvider is not null before adding it, or use appropriate null-safe handling.
|
|
||
| final Iterable<String> parts = Splitter.on( '/' ).trimResults().omitEmptyStrings().split( value ); | ||
| return "/" + String.join( "/", parts ); | ||
| return Splitter.on( '/' ).omitEmptyStrings().trimResults().splitToStream( path ).collect( Collectors.joining( "/", "/", "" ) ); |
There was a problem hiding this comment.
The normalizePath method produces paths with trailing slashes (e.g., /path/to/page/ instead of /path/to/page). The Collectors.joining call uses "/" as suffix, which adds a trailing slash to all normalized paths. This differs from the previous implementation which didn't add trailing slashes. This breaking change could affect path matching and routing logic.
No description provided.