Protect against '//' in UriComponentsBuilder

Refactor UriComponentsBuilder to ensure that paths do not contain empty
segments.

For example, prior to this commit:

    fromUriString("http://example.com/abc/").path("/x/y/z")

would build the URL "http://example.com/abc//x/y/z" where as it will
now build "http://example.com/abc/x/y/z".

Issue: SPR-10270

Author: philwebb

spring-web/src/main/java/org/springframework/web/util/UriComponentsBuilder.java

@@ -18,7 +18,7 @@
 
 import java.net.URI;
 import java.util.ArrayList;
-import java.util.Collection;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.regex.Matcher;
@@ -29,6 +29,7 @@
 import org.springframework.util.MultiValueMap;
 import org.springframework.util.ObjectUtils;
 import org.springframework.util.StringUtils;
+import org.springframework.web.util.HierarchicalUriComponents.PathComponent;
 
 /**
  * Builder for {@link UriComponents}.
@@ -46,6 +47,7 @@
  *
  * @author Arjen Poutsma
  * @author Rossen Stoyanchev
+ * @author Phillip Webb
  * @since 3.1
  * @see #newInstance()
  * @see #fromPath(String)
@@ -91,7 +93,7 @@ public class UriComponentsBuilder {
 
 	private int port = -1;
 
-	private PathComponentBuilder pathBuilder = NULL_PATH_COMPONENT_BUILDER;
+	private CompositePathComponentBuilder pathBuilder = new CompositePathComponentBuilder();
 
 	private final MultiValueMap<String, String> queryParams = new LinkedMultiValueMap<String, String>();
 
@@ -334,7 +336,7 @@ public UriComponentsBuilder uri(URI uri) {
 				this.port = uri.getPort();
 			}
 			if (StringUtils.hasLength(uri.getRawPath())) {
-				this.pathBuilder = new FullPathComponentBuilder(uri.getRawPath());
+				this.pathBuilder = new CompositePathComponentBuilder(uri.getRawPath());
 			}
 			if (StringUtils.hasLength(uri.getRawQuery())) {
 				this.queryParams.clear();
@@ -352,7 +354,7 @@ private void resetHierarchicalComponents() {
 		this.userInfo = null;
 		this.host = null;
 		this.port = -1;
-		this.pathBuilder = NULL_PATH_COMPONENT_BUILDER;
+		this.pathBuilder = new CompositePathComponentBuilder();
 		this.queryParams.clear();
 	}
 
@@ -436,12 +438,7 @@ public UriComponentsBuilder port(int port) {
 	 * @return this UriComponentsBuilder
 	 */
 	public UriComponentsBuilder path(String path) {
-		if (path != null) {
-			this.pathBuilder = this.pathBuilder.appendPath(path);
-		}
-		else {
-			this.pathBuilder = NULL_PATH_COMPONENT_BUILDER;
-		}
+		this.pathBuilder.addPath(path);
 		resetSchemeSpecificPart();
 		return this;
 	}
@@ -453,22 +450,21 @@ public UriComponentsBuilder path(String path) {
 	 * @return this UriComponentsBuilder
 	 */
 	public UriComponentsBuilder replacePath(String path) {
-		this.pathBuilder = NULL_PATH_COMPONENT_BUILDER;
-		path(path);
+		this.pathBuilder = new CompositePathComponentBuilder(path);
 		resetSchemeSpecificPart();
 		return this;
 	}
 
 	/**
-	 * Appends the given path segments to the existing path of this builder. Each given path segments may contain URI
-	 * template variables.
+	 * Appends the given path segments to the existing path of this builder. Each given
+	 * path segments may contain URI template variables.
 	 *
 	 * @param pathSegments the URI path segments
 	 * @return this UriComponentsBuilder
 	 */
 	public UriComponentsBuilder pathSegment(String... pathSegments) throws IllegalArgumentException {
 		Assert.notNull(pathSegments, "'segments' must not be null");
-		this.pathBuilder = this.pathBuilder.appendPathSegments(pathSegments);
+		this.pathBuilder.addPathSegments(pathSegments);
 		resetSchemeSpecificPart();
 		return this;
 	}
@@ -590,131 +586,122 @@ public UriComponentsBuilder fragment(String fragment) {
 		return this;
 	}
 
-	/**
-	 * Represents a builder for {@link HierarchicalUriComponents.PathComponent}
-	 */
-	private interface PathComponentBuilder {
-
-		HierarchicalUriComponents.PathComponent build();
 
-		PathComponentBuilder appendPath(String path);
-
-		PathComponentBuilder appendPathSegments(String... pathSegments);
+	private interface PathComponentBuilder {
+		PathComponent build();
 	}
 
-	/**
-	 * Represents a builder for full string paths.
-	 */
-	private static class FullPathComponentBuilder implements PathComponentBuilder {
-
-		private final StringBuilder path;
+	private static class CompositePathComponentBuilder implements PathComponentBuilder {
 
-		private FullPathComponentBuilder(String path) {
-			this.path = new StringBuilder(path);
-		}
-
-		public HierarchicalUriComponents.PathComponent build() {
-			return new HierarchicalUriComponents.FullPathComponent(path.toString());
-		}
+		private LinkedList<PathComponentBuilder> componentBuilders = new LinkedList<PathComponentBuilder>();
 
-		public PathComponentBuilder appendPath(String path) {
-			this.path.append(path);
-			return this;
-		}
-
-		public PathComponentBuilder appendPathSegments(String... pathSegments) {
-			PathComponentCompositeBuilder builder = new PathComponentCompositeBuilder(this);
-			builder.appendPathSegments(pathSegments);
-			return builder;
+		public CompositePathComponentBuilder() {
 		}
-	}
-
-	/**
-	 * Represents a builder for paths segment paths.
-	 */
-	private static class PathSegmentComponentBuilder implements PathComponentBuilder {
-
-		private final List<String> pathSegments = new ArrayList<String>();
 
-		private PathSegmentComponentBuilder(String... pathSegments) {
-			this.pathSegments.addAll(removeEmptyPathSegments(pathSegments));
+		public CompositePathComponentBuilder(String path) {
+			addPath(path);
 		}
 
-		private Collection<String> removeEmptyPathSegments(String... pathSegments) {
-			List<String> result = new ArrayList<String>();
-			for (String segment : pathSegments) {
-				if (StringUtils.hasText(segment)) {
-					result.add(segment);
+		public void addPathSegments(String... pathSegments) {
+			if (!ObjectUtils.isEmpty(pathSegments)) {
+				PathSegmentComponentBuilder psBuilder = getLastBuilder(PathSegmentComponentBuilder.class);
+				FullPathComponentBuilder fpBuilder = getLastBuilder(FullPathComponentBuilder.class);
+				if (psBuilder == null) {
+					psBuilder = new PathSegmentComponentBuilder();
+					this.componentBuilders.add(psBuilder);
+					if (fpBuilder != null) {
+						fpBuilder.removeTrailingSlash();
+					}
 				}
+				psBuilder.append(pathSegments);
 			}
-			return result;
 		}
 
-		public HierarchicalUriComponents.PathComponent build() {
-			return new HierarchicalUriComponents.PathSegmentComponent(pathSegments);
+		public void addPath(String path) {
+			if (StringUtils.hasText(path)) {
+				PathSegmentComponentBuilder psBuilder = getLastBuilder(PathSegmentComponentBuilder.class);
+				FullPathComponentBuilder fpBuilder = getLastBuilder(FullPathComponentBuilder.class);
+				if (psBuilder != null) {
+					path = path.startsWith("/") ? path : "/" + path;
+				}
+				if (fpBuilder == null) {
+					fpBuilder = new FullPathComponentBuilder();
+					this.componentBuilders.add(fpBuilder);
+				}
+				fpBuilder.append(path);
+			}
 		}
 
-		public PathComponentBuilder appendPath(String path) {
-			PathComponentCompositeBuilder builder = new PathComponentCompositeBuilder(this);
-			builder.appendPath(path);
-			return builder;
+		@SuppressWarnings("unchecked")
+		private <T> T getLastBuilder(Class<T> builderClass) {
+			if (!this.componentBuilders.isEmpty()) {
+				PathComponentBuilder last = this.componentBuilders.getLast();
+				if (builderClass.isInstance(last)) {
+					return (T) last;
+				}
+			}
+			return null;
 		}
 
-		public PathComponentBuilder appendPathSegments(String... pathSegments) {
-			this.pathSegments.addAll(removeEmptyPathSegments(pathSegments));
-			return this;
+		public PathComponent build() {
+			int size = this.componentBuilders.size();
+			List<PathComponent> components = new ArrayList<PathComponent>(size);
+			for (int i = 0; i < size; i++) {
+				PathComponent pathComponent = this.componentBuilders.get(i).build();
+				if (pathComponent != null) {
+					components.add(pathComponent);
+				}
+			}
+			if (components.isEmpty()) {
+				return HierarchicalUriComponents.NULL_PATH_COMPONENT;
+			}
+			if (components.size() == 1) {
+				return components.get(0);
+			}
+			return new HierarchicalUriComponents.PathComponentComposite(components);
 		}
 	}
 
-	/**
-	 * Represents a builder for a collection of PathComponents.
-	 */
-	private static class PathComponentCompositeBuilder implements PathComponentBuilder {
+	private static class FullPathComponentBuilder implements PathComponentBuilder {
 
-		private final List<PathComponentBuilder> pathComponentBuilders = new ArrayList<PathComponentBuilder>();
+		private StringBuilder path = new StringBuilder();
 
-		private PathComponentCompositeBuilder(PathComponentBuilder builder) {
-			pathComponentBuilders.add(builder);
+		public void append(String path) {
+			this.path.append(path);
 		}
 
-		public HierarchicalUriComponents.PathComponent build() {
-			List<HierarchicalUriComponents.PathComponent> pathComponents =
-					new ArrayList<HierarchicalUriComponents.PathComponent>(pathComponentBuilders.size());
-
-			for (PathComponentBuilder pathComponentBuilder : pathComponentBuilders) {
-				pathComponents.add(pathComponentBuilder.build());
+		public PathComponent build() {
+			if (this.path.length() == 0) {
+				return null;
 			}
-			return new HierarchicalUriComponents.PathComponentComposite(pathComponents);
-		}
-
-		public PathComponentBuilder appendPath(String path) {
-			this.pathComponentBuilders.add(new FullPathComponentBuilder(path));
-			return this;
+			String path = this.path.toString().replace("//", "/");
+			return new HierarchicalUriComponents.FullPathComponent(path);
 		}
 
-		public PathComponentBuilder appendPathSegments(String... pathSegments) {
-			this.pathComponentBuilders.add(new PathSegmentComponentBuilder(pathSegments));
-			return this;
+		public void removeTrailingSlash() {
+			int index = this.path.length() - 1;
+			if (this.path.charAt(index) == '/') {
+				this.path.deleteCharAt(index);
+			}
 		}
 	}
 
+	private static class PathSegmentComponentBuilder implements PathComponentBuilder {
 
-	/**
-	 * Represents a builder for an empty path.
-	 */
-	private static PathComponentBuilder NULL_PATH_COMPONENT_BUILDER = new PathComponentBuilder() {
-
-		public HierarchicalUriComponents.PathComponent build() {
-			return HierarchicalUriComponents.NULL_PATH_COMPONENT;
-		}
+		private List<String> pathSegments = new LinkedList<String>();
 
-		public PathComponentBuilder appendPath(String path) {
-			return new FullPathComponentBuilder(path);
+		public void append(String... pathSegments) {
+			for (String pathSegment : pathSegments) {
+				if (StringUtils.hasText(pathSegment)) {
+					this.pathSegments.add(pathSegment);
+				}
+			}
 		}
 
-		public PathComponentBuilder appendPathSegments(String... pathSegments) {
-			return new PathSegmentComponentBuilder(pathSegments);
+		public PathComponent build() {
+			return this.pathSegments.isEmpty() ?
+					null : new HierarchicalUriComponents.PathSegmentComponent(this.pathSegments);
 		}
-	};
+	}
 
 }

spring-web/src/test/java/org/springframework/web/util/UriComponentsBuilderTests.java

@@ -346,4 +346,12 @@ public void relativeUrls() throws Exception {
 		assertThat(UriComponentsBuilder.fromUriString("http://example.com").path("foo/../bar").build().toUriString(), equalTo("http://example.com/foo/../bar"));
 		assertThat(UriComponentsBuilder.fromUriString("http://example.com").path("foo/../bar").build().toUri().getPath(), equalTo("/foo/../bar"));
 	}
+
+	@Test
+	public void emptySegments() throws Exception {
+		assertThat(UriComponentsBuilder.fromUriString("http://example.com/abc/").path("/x/y/z").build().toString(), equalTo("http://example.com/abc/x/y/z"));
+	    assertThat(UriComponentsBuilder.fromUriString("http://example.com/abc/").pathSegment("x", "y", "z").build().toString(), equalTo("http://example.com/abc/x/y/z"));
+		assertThat(UriComponentsBuilder.fromUriString("http://example.com/abc/").path("/x/").path("/y/z").build().toString(), equalTo("http://example.com/abc/x/y/z"));
+		assertThat(UriComponentsBuilder.fromUriString("http://example.com/abc/").pathSegment("x").path("y").build().toString(), equalTo("http://example.com/abc/x/y"));
+	}
 }