Skip to content

Optimize urlRewrite #11600 #11601

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.

Sign up for GitHub

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

Draft
rymsha wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Optimize urlRewrite #11600 #11601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 53 additions & 53 deletions modules/web/web-api/src/main/java/com/enonic/xp/web/servlet/ServletRequestUrlHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.Objects;
import java.util.stream.Collectors;

import com.google.common.base.Splitter;

Expand All @@ -12,8 +14,6 @@
import com.enonic.xp.web.vhost.VirtualHost;
import com.enonic.xp.web.vhost.VirtualHostHelper;

import static com.google.common.base.Strings.isNullOrEmpty;

@PublicApi
public final class ServletRequestUrlHelper
{
Expand All @@ -22,10 +22,15 @@ public final class ServletRequestUrlHelper

public static String createUri( final HttpServletRequest req, final String path )
{
return rewriteUri( req, path ).getRewrittenUri();
return Objects.requireNonNullElse( rewriteUri( VirtualHostHelper.getVirtualHost( req ), path ), path );
}

public static String getServerUrl( final HttpServletRequest req )
{
return getOrigin( req ).toString();
}

private static StringBuilder getOrigin( final HttpServletRequest req )
{
final StringBuilder str = new StringBuilder();

Expand All @@ -43,18 +48,14 @@ public static String getServerUrl( final HttpServletRequest req )
str.append( ":" ).append( port );
}

return str.toString();
return str;
}

public static String getFullUrl( final HttpServletRequest req )
{
//Appends the server part
StringBuilder fullUrl = new StringBuilder( getServerUrl( req ) );

//Appends the path part
fullUrl.append( rewriteUri( req, req.getRequestURI() ).getRewrittenUri() );
final StringBuilder fullUrl = getOrigin( req );
fullUrl.append( createUri( req, req.getRequestURI() ) );

//Appends the query string part
final String queryString = req.getQueryString();
if ( queryString != null )
{
Expand All @@ -66,75 +67,74 @@ public static String getFullUrl( final HttpServletRequest req )

private static boolean needPortNumber( final String scheme, final int port )
{
final boolean isUndefined = port < 0;
final boolean isHttpOrWs = ( "http".equals( scheme ) || "ws".equals( scheme ) ) && 80 == port;
final boolean isHttpsOrWss = ( "https".equals( scheme ) || "wss".equals( scheme ) ) && 443 == port;
return !( isUndefined || isHttpOrWs || isHttpsOrWss );
return switch ( port )
{
case 80 -> !"http".equals( scheme ) && !"ws".equals( scheme );
case 443 -> !"https".equals( scheme ) && !"wss".equals( scheme );
default -> port > 0;
};
}

public static UriRewritingResult rewriteUri( final HttpServletRequest req, final String uri )
{
UriRewritingResult.Builder resultBuilder = UriRewritingResult.create();
final UriRewritingResult.Builder resultBuilder = UriRewritingResult.create();

final VirtualHost vhost = VirtualHostHelper.getVirtualHost( req );
if ( vhost == null )
{
return resultBuilder.rewrittenUri( uri ).build();
}

final String targetPath = vhost.getTarget();
if ( needRewrite( uri, targetPath ) )
{
final String result = uri.substring( targetPath.length() );
final String newUri = normalizePath( vhost.getSource() + ( "/".equals( targetPath ) ? "/" : "" ) + result );
return resultBuilder.rewrittenUri( newUri )
.deletedUriPrefix( targetPath )
.newUriPrefix( normalizePath( vhost.getSource() ) )
.build();
}
final String rewrittenUri = rewriteUri( vhost, uri );

return resultBuilder.rewrittenUri( normalizePath( uri ) ).outOfScope( true ).build();
final String source = vhost.getSource();
final String target = vhost.getTarget();

return resultBuilder.deletedUriPrefix( target )
.newUriPrefix( source )
.rewrittenUri( Objects.requireNonNullElse( rewrittenUri, uri ) )
.outOfScope( rewrittenUri == null )
.build();
}

private static boolean needRewrite( final String uri, final String targetPath )
private static String rewriteUri( final VirtualHost vhost, final String uri )
{
if ( targetPath.equals( "/" ) )
if ( vhost == null || !uri.startsWith( "/" ) )
{
return uri.startsWith( "/" );
return uri;
}
if ( uri.equals( targetPath ) )

final String source = vhost.getSource();
final String target = vhost.getTarget();

if ( target.equals( "/" ) )
{
return true;
return normalizePath( "/".equals( source ) ? uri : source + uri );
}

final int queryPos = uri.indexOf( '?' );
final int pathLength = queryPos == -1 ? uri.length() : queryPos;

if ( queryPos == -1 )
{
return uri.startsWith( targetPath + "/" );
}
else
final int targetLength = target.length();

if ( uri.startsWith( target ) &&
( pathLength == targetLength || ( pathLength > targetLength && uri.charAt( targetLength ) == '/' ) ) )
{
final String uriWithoutQuery = uri.substring( 0, queryPos );
if ( uriWithoutQuery.equals( targetPath ) )
final StringBuilder sb = new StringBuilder();
if ( !"/".equals( source ) )
{
return true;
}
else
{
return uriWithoutQuery.startsWith( targetPath + "/" );
sb.append( source );
}
sb.append( uri, targetLength, uri.length() );
return normalizePath( sb.toString() );
}

return null;
}

private static String normalizePath( final String value )
private static String normalizePath( final String path )
{
if ( isNullOrEmpty( value ) )
{
return "/";
}

final Iterable<String> parts = Splitter.on( '/' ).trimResults().omitEmptyStrings().split( value );
return "/" + String.join( "/", parts );
return Splitter.on( '/' ).omitEmptyStrings().trimResults().splitToStream( path ).collect( Collectors.joining( "/", "/", "" ) );
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}

public static String contentDispositionAttachment( final String fileName )
Expand All @@ -149,7 +149,7 @@ public static String contentDispositionAttachment( final String fileName )

private static void appendQuoted( final StringBuilder builder, final String input )
{
builder.append( "\"" );
builder.append( '"' );
input.codePoints().forEachOrdered( value -> {
if ( value == '"' )
{
Expand All @@ -160,7 +160,7 @@ private static void appendQuoted( final StringBuilder builder, final String inpu
builder.appendCodePoint( value );
}
} );
builder.append( "\"" );
builder.append( '"' );
}

private static void appendRfc8187Encoded( final StringBuilder builder, final String input, final Charset charset )
Expand Down
38 changes: 38 additions & 0 deletions modules/web/web-api/src/test/java/com/enonic/xp/web/servlet/ServletRequestUrlHelperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,30 @@ void rewriteUri_vhost()
assertFalse( rewritingResult3.isOutOfScope() );
}

@Test
void rewriteUri_vhost_trivial()
{
final VirtualHost vhost = mock( VirtualHost.class );
when( req.getAttribute( VirtualHost.class.getName() ) ).thenReturn( vhost );

when( vhost.getTarget() ).thenReturn( "/" );
when( vhost.getSource() ).thenReturn( "/" );

final UriRewritingResult rewritingResult = ServletRequestUrlHelper.rewriteUri( req, "/path/to/page" );
assertEquals( "/path/to/page", rewritingResult.getRewrittenUri() );
assertFalse( rewritingResult.isOutOfScope() );

when( vhost.getTarget() ).thenReturn( "/root/to/site" );
final UriRewritingResult rewritingResult2 = ServletRequestUrlHelper.rewriteUri( req, "/path/to/page" );
assertEquals( "/path/to/page", rewritingResult2.getRewrittenUri() );
assertTrue( rewritingResult2.isOutOfScope() );

when( vhost.getTarget() ).thenReturn( "/path/to" );
final UriRewritingResult rewritingResult3 = ServletRequestUrlHelper.rewriteUri( req, "/path/to/page" );
assertEquals( "/page", rewritingResult3.getRewrittenUri() );
assertFalse( rewritingResult3.isOutOfScope() );
}

@Test
void contentDispositionAttachment_filename_with_comma()
{
Expand Down Expand Up @@ -166,6 +190,20 @@ void rewriteUri_vhost_outOfScope()
assertFalse( rewritingResult.isOutOfScope() );
}

@Test
void rewriteUri_vhost_outOfScope_short()
{
final VirtualHost vhost = mock( VirtualHost.class );
when( req.getAttribute( VirtualHost.class.getName() ) ).thenReturn( vhost );

when( vhost.getTarget() ).thenReturn( "/site/default/draft/enonic" );
when( vhost.getSource() ).thenReturn( "/no" );

UriRewritingResult rewritingResult = ServletRequestUrlHelper.rewriteUri( req, "/site/default" );
assertEquals( "/site/default", rewritingResult.getRewrittenUri() );
assertTrue( rewritingResult.isOutOfScope() );
}

@Test
void createUri_admin_queryString()
{
Expand Down
22 changes: 6 additions & 16 deletions ...ost/src/main/java/com/enonic/xp/web/vhost/impl/mapping/VirtualHostIdProvidersMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,18 @@
package com.enonic.xp.web.vhost.impl.mapping;

import java.util.ArrayList;
import java.util.List;
import com.google.common.collect.ImmutableSet;

import com.enonic.xp.security.IdProviderKey;
import com.enonic.xp.security.IdProviderKeys;

public class VirtualHostIdProvidersMapping
{
private final IdProviderKey defaultIdProvider;

private final IdProviderKeys idProviderKeys;

public VirtualHostIdProvidersMapping( final Builder builder )
{
this.defaultIdProvider = builder.defaultIdProvider;
this.idProviderKeys = IdProviderKeys.from( builder.idProviderKeys );
this.idProviderKeys = IdProviderKeys.from(
ImmutableSet.<IdProviderKey>builder().add( builder.defaultIdProvider ).addAll( builder.idProviderKeys.build() ).build() );
Comment on lines +14 to +15
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}

public static Builder create()
Expand All @@ -25,7 +22,7 @@ public static Builder create()

public IdProviderKey getDefaultIdProvider()
{
return defaultIdProvider;
return idProviderKeys.first();
}

public IdProviderKeys getIdProviderKeys()
Expand All @@ -37,28 +34,21 @@ public static class Builder
{
private IdProviderKey defaultIdProvider;

private final List<IdProviderKey> idProviderKeys;
private final ImmutableSet.Builder<IdProviderKey> idProviderKeys = ImmutableSet.builder();

private Builder()
{
this.idProviderKeys = new ArrayList<>();
}

public Builder setDefaultIdProvider( final IdProviderKey defaultIdProvider )
{
this.defaultIdProvider = defaultIdProvider;
addIdProviderKey( defaultIdProvider );

return this;
}

public Builder addIdProviderKey( final IdProviderKey idProviderKey )
{
if ( !this.idProviderKeys.contains( idProviderKey ) )
{
this.idProviderKeys.add( idProviderKey );
}

this.idProviderKeys.add( idProviderKey );
return this;
}

Expand Down
Loading