From 533fef1b698ebb7f93026e12018e3ba52c29d099 Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Mon, 4 Apr 2022 19:21:26 +0200 Subject: API brush-up, more unit test coverage --- .../src/main/java/com/yahoo/restapi/HttpURL.java | 166 ++++++++++++++++----- .../test/java/com/yahoo/restapi/HttpURLTest.java | 152 ++++++++++++++++++- 2 files changed, 276 insertions(+), 42 deletions(-) diff --git a/container-core/src/main/java/com/yahoo/restapi/HttpURL.java b/container-core/src/main/java/com/yahoo/restapi/HttpURL.java index 9fcbd06b1dd..a43c5998c79 100644 --- a/container-core/src/main/java/com/yahoo/restapi/HttpURL.java +++ b/container-core/src/main/java/com/yahoo/restapi/HttpURL.java @@ -11,6 +11,7 @@ import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.OptionalInt; import java.util.StringJoiner; import java.util.function.Function; @@ -105,7 +106,7 @@ public class HttpURL { throw new IllegalArgumentException("uri should be normalized, but got: " + uri); return create(Scheme.of(uri.getScheme()), - DomainName.of(uri.getHost()), + DomainName.of(requireNonNull(uri.getHost(), "URI must specify a host")), uri.getPort(), Path.parse(uri.getRawPath(), validator, inverse), Query.parse(uri.getRawQuery(), validator, inverse)); @@ -119,6 +120,14 @@ public class HttpURL { return create(scheme, domain, port, path, query); } + public HttpURL withPort(int port) { + return create(scheme, domain, port, path, query); + } + + public HttpURL withoutPort() { + return create(scheme, domain, -1, path, query); + } + public HttpURL withPath(Path path) { return create(scheme, domain, port, path, query); } @@ -172,14 +181,23 @@ public class HttpURL { this.inverse = requireNonNull(inverse); } + /** Creates a new, empty path, with a trailing slash. */ + public static Path empty() { + return new Path<>(List.of(), true, identity(), identity()); + } + /** Creates a new, empty path, with a trailing slash, using the indicated string wrapper for segments. */ public static > Path empty(Function validator) { return new Path<>(List.of(), true, validator, T::value); } + /** Creates a new path with the given decoded segments. */ + public static Path from(List segments) { + return empty().append(segments); + } - /** Creates a new, empty path, with a trailing slash. */ - public static Path empty() { - return new Path<>(List.of(), true, identity(), identity()); + /** Creates a new path with the given decoded segments, and the validator applied to each segment. */ + public static > Path from(List segments, Function validator) { + return empty(validator).append(segments, identity(), true); } /** Parses the given raw, normalized path string; this ignores whether the path is absolute or relative.) */ @@ -208,29 +226,35 @@ public class HttpURL { segment, "path segments cannot be \"\", \".\", or \"..\""); } - /** Returns a copy of this which eliminates the given number of segments, from the root down. */ - public Path tailPath(int offset) { - return new Path<>(segments.subList(offset, segments.size()), trailingSlash, validator, inverse); + /** Returns a copy of this where the first segments are skipped. */ + public Path skip(int count) { + return new Path<>(segments.subList(count, segments.size()), trailingSlash, validator, inverse); } - /** Returns a copy of this which eliminates the given number of segments, from the end up. */ - public Path headPath(int offset) { - return new Path<>(segments.subList(0, segments.size() - offset), trailingSlash, validator, inverse); + /** Returns a copy of this where the last segments are cut off. */ + public Path cut(int count) { + return new Path<>(segments.subList(0, segments.size() - count), trailingSlash, validator, inverse); } /** Returns a copy of this with the decoded segment appended at the end; it may not be either of {@code ""}, {@code "."} or {@code ".."}. */ - public Path with(T segment) { - List copy = new ArrayList<>(segments); - copy.add(requireNonNull(segment)); - return new Path<>(copy, trailingSlash, validator, inverse); + public Path append(String segment) { + return append(List.of(segment), identity(), trailingSlash); } /** Returns a copy of this all segments of the other path appended, with a trailing slash as per the appendage. */ - public Path with(Path other) { - List copy = new ArrayList<>(segments); - for (U segment : other.segments) - copy.add(validator.apply(other.inverse.apply(segment))); - return new Path<>(copy, other.trailingSlash, validator, inverse); + public Path append(Path other) { + return append(other.segments, other.inverse, other.trailingSlash); + } + + /** Returns a copy of this all given segments appended, with a trailing slash as per this path. */ + public Path append(List segments) { + return append(segments, inverse, trailingSlash); + } + + private Path append(List segments, Function inverse, boolean trailingSlash) { + List copy = new ArrayList<>(this.segments); + for (U segment : segments) copy.add(validator.apply(requireNonNormalizable(inverse.apply(segment)))); + return new Path<>(copy, trailingSlash, validator, this.inverse); } /** Returns a copy of this which encodes a trailing slash. */ @@ -243,8 +267,8 @@ public class HttpURL { return new Path<>(segments, false, validator, inverse); } - /** The URL decoded segments that make up this path; never {@code ""}, {@code "."} or {@code ".."}. */ - public List decoded() { + /** The URL decoded segments that make up this path; never {@code null}, {@code ""}, {@code "."} or {@code ".."}. */ + public List segments() { return Collections.unmodifiableList(segments); } @@ -256,6 +280,25 @@ public class HttpURL { return joiner.toString(); } + /** Intentionally not usable for constructing new URIs. Use {@link HttpURL} for that instead. */ + @Override + public String toString() { + return "path '" + raw() + "'"; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Path path = (Path) o; + return trailingSlash == path.trailingSlash && segments.equals(path.segments); + } + + @Override + public int hashCode() { + return Objects.hash(segments, trailingSlash); + } + } @@ -271,6 +314,25 @@ public class HttpURL { this.inverse = requireNonNull(inverse); } + /** Creates a new, empty query part. */ + public static Query empty() { + return new Query<>(Map.of(), identity(), identity()); + } + + /** Creates a new, empty query part, using the indicated string wrapper for keys and non-null values. */ + public static > Query empty(Function validator) { + return new Query<>(Map.of(), validator, T::value); + } + /** Creates a new query part with the given decoded values. */ + public static Query from(Map values) { + return empty().merge(values); + } + + /** Creates a new query part with the given decoded values, and the validator applied to each pair. */ + public static > Query from(Map values, Function validator) { + return empty(validator).merge(values, identity()); + } + /** Parses the given raw query string, using the indicated string wrapper to hold keys and non-null values. */ public static > Query parse(String raw, Function validator) { return parse(raw, validator, T::value); @@ -294,39 +356,46 @@ public class HttpURL { return new Query<>(values, validator, inverse); } - /** Creates a new, empty query part, using the indicated string wrappper for keys and non-null values. */ - public static > Query empty(Function validator) { - return new Query(Map.of(), validator, T::value); - } - - /** Creates a new, empty query part. */ - public static Query empty() { - return new Query<>(Map.of(), identity(), identity()); - } - /** Returns a copy of this with the decoded non-null key pointing to the decoded non-null value. */ - public Query with(T key, T value) { + public Query put(String key, String value) { Map copy = new LinkedHashMap<>(values); - copy.put(requireNonNull(key), requireNonNull(value)); + copy.put(requireNonNull(validator.apply(key)), requireNonNull(validator.apply(value))); return new Query<>(copy, validator, inverse); } /** Returns a copy of this with the decoded non-null key pointing to "nothing". */ - public Query with(T key) { + public Query add(String key) { Map copy = new LinkedHashMap<>(values); - copy.put(requireNonNull(key), null); + copy.put(requireNonNull(validator.apply(key)), null); return new Query<>(copy, validator, inverse); } /** Returns a copy of this without any key-value pair with the decoded key. */ - public Query without(T key) { + public Query remove(String key) { Map copy = new LinkedHashMap<>(values); - copy.remove(requireNonNull(key)); + copy.remove(requireNonNull(validator.apply(key))); return new Query<>(copy, validator, inverse); } - /** The URL decoded key-value pairs that make up this query; keys and values may be {@code ""}, and values are {@code} null if only key was specified. */ - public Map decoded() { + /** Returns a copy of this with all mappings from the other query added to this, possibly overwriting existing mappings. */ + public Query merge(Query other) { + return merge(other.values, other.inverse); + } + + /** Returns a copy of this with all given mappings added to this, possibly overwriting existing mappings. */ + public Query merge(Map values) { + return merge(values, inverse); + } + + private Query merge(Map values, Function inverse) { + Map copy = new LinkedHashMap<>(this.values); + values.forEach((key, value) -> copy.put(validator.apply(inverse.apply(requireNonNull(key, "keys cannot be null"))), + value == null ? null : validator.apply(inverse.apply(value)))); + return new Query<>(copy, validator, this.inverse); + } + + /** The URL decoded key-value pairs that make up this query; keys and values may be {@code ""}, and values are {@code null} when only key was specified. */ + public Map entries() { return unmodifiableMap(values); } @@ -338,6 +407,25 @@ public class HttpURL { return joiner.toString(); } + /** Intentionally not usable for constructing new URIs. Use {@link HttpURL} for that instead. */ + @Override + public String toString() { + return "query '" + raw() + "'"; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Query query = (Query) o; + return values.equals(query.values); + } + + @Override + public int hashCode() { + return Objects.hash(values); + } + } diff --git a/container-core/src/test/java/com/yahoo/restapi/HttpURLTest.java b/container-core/src/test/java/com/yahoo/restapi/HttpURLTest.java index 00da850d739..de20fcb3193 100644 --- a/container-core/src/test/java/com/yahoo/restapi/HttpURLTest.java +++ b/container-core/src/test/java/com/yahoo/restapi/HttpURLTest.java @@ -1,14 +1,23 @@ // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.restapi; +import ai.vespa.validation.Name; +import com.yahoo.net.DomainName; +import com.yahoo.restapi.HttpURL.Query; import org.junit.jupiter.api.Test; import java.net.URI; +import java.util.ArrayList; +import java.util.LinkedHashMap; import java.util.List; -import java.util.stream.Collectors; -import java.util.stream.Stream; +import java.util.Map; +import java.util.OptionalInt; +import static com.yahoo.net.DomainName.localhost; +import static com.yahoo.restapi.HttpURL.Scheme.http; +import static com.yahoo.restapi.HttpURL.Scheme.https; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; /** * @author jonmv @@ -16,7 +25,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; class HttpURLTest { @Test - void testConversion() { + void testConversionBackAndForth() { for (String uri : List.of("http://minimal", "http://empty.query?", "http://zero-port:0?no=path", @@ -29,4 +38,141 @@ class HttpURLTest { "uri '" + uri + "' should be returned unchanged"); } + @Test + void testModification() { + HttpURL url = HttpURL.create(http, localhost, Name::of); + assertEquals(http, url.scheme()); + assertEquals(localhost, url.domain()); + assertEquals(OptionalInt.empty(), url.port()); + assertEquals(HttpURL.Path.empty(Name::of), url.path()); + assertEquals(HttpURL.Query.empty(Name::of), url.query()); + + url = url.withScheme(https) + .withDomain(DomainName.of("domain")) + .withPort(0) + .withPath(url.path().append("foo").withoutTrailingSlash()) + .withQuery(url.query().put("boo", "bar").add("baz")); + assertEquals(https, url.scheme()); + assertEquals(DomainName.of("domain"), url.domain()); + assertEquals(OptionalInt.of(0), url.port()); + assertEquals(HttpURL.Path.parse("/foo", Name::of), url.path()); + assertEquals(HttpURL.Query.parse("boo=bar&baz", Name::of), url.query()); + } + + @Test + void testInvalidURIs() { + assertEquals("scheme must be HTTP or HTTPS", + assertThrows(IllegalArgumentException.class, + () -> HttpURL.from(URI.create("file:/txt"))).getMessage()); + + assertEquals("URI must specify a host", + assertThrows(NullPointerException.class, + () -> HttpURL.from(URI.create("http:///foo"))).getMessage()); + + assertEquals("port number must be at least '-1' and at most '65535', but got: '65536'", + assertThrows(IllegalArgumentException.class, + () -> HttpURL.from(URI.create("http://foo:65536/bar"))).getMessage()); + + assertEquals("uri should be normalized, but got: http://foo//", + assertThrows(IllegalArgumentException.class, + () -> HttpURL.from(URI.create("http://foo//"))).getMessage()); + + assertEquals("uri should be normalized, but got: http://foo/./", + assertThrows(IllegalArgumentException.class, + () -> HttpURL.from(URI.create("http://foo/./"))).getMessage()); + + assertEquals("path segments cannot be \"\", \".\", or \"..\", but got: '..'", + assertThrows(IllegalArgumentException.class, + () -> HttpURL.from(URI.create("http://foo/.."))).getMessage()); + + assertEquals("path segments cannot be \"\", \".\", or \"..\", but got: '..'", + assertThrows(IllegalArgumentException.class, + () -> HttpURL.from(URI.create("http://foo/.%2E"))).getMessage()); + + assertEquals("name must match '[A-Za-z][A-Za-z0-9_-]{0,63}', but got: '/'", + assertThrows(IllegalArgumentException.class, + () -> HttpURL.from(URI.create("http://foo/%2F"), Name::of)).getMessage()); + + assertEquals("name must match '[A-Za-z][A-Za-z0-9_-]{0,63}', but got: '/'", + assertThrows(IllegalArgumentException.class, + () -> HttpURL.from(URI.create("http://foo?%2F"), Name::of)).getMessage()); + + assertEquals("name must match '[A-Za-z][A-Za-z0-9_-]{0,63}', but got: ''", + assertThrows(IllegalArgumentException.class, + () -> HttpURL.from(URI.create("http://foo?"), Name::of)).getMessage()); + } + + @Test + void testPath() { + HttpURL.Path path = HttpURL.Path.parse("foo/bar/baz", Name::of); + List expected = List.of(Name.of("foo"), Name.of("bar"), Name.of("baz")); + assertEquals(expected, path.segments()); + + assertEquals(expected.subList(1, 3), path.skip(1).segments()); + assertEquals(expected.subList(0, 2), path.cut(1).segments()); + assertEquals(expected.subList(1, 2), path.skip(1).cut(1).segments()); + + assertEquals("path '/foo/bar/baz/'", path.withTrailingSlash().toString()); + assertEquals(path, path.withoutTrailingSlash().withoutTrailingSlash()); + + assertEquals(List.of("one", "foo", "bar", "baz", "two"), + HttpURL.Path.from(List.of("one")).append(path).append("two").segments()); + + assertEquals(List.of(expected.get(2), expected.get(0)), + path.append(path).cut(2).skip(2).segments()); + + assertThrows(NullPointerException.class, + () -> path.append((String) null)); + + List names = new ArrayList<>(); + names.add(null); + assertThrows(NullPointerException.class, + () -> path.append(names)); + + assertEquals("name must match '[A-Za-z][A-Za-z0-9_-]{0,63}', but got: '???'", + assertThrows(IllegalArgumentException.class, + () -> path.append("???")).getMessage()); + + assertEquals("fromIndex(2) > toIndex(1)", + assertThrows(IllegalArgumentException.class, + () -> path.cut(2).skip(2)).getMessage()); + } + + @Test + void testQuery() { + Query query = Query.parse("foo=bar&baz", Name::of); + Map expected = new LinkedHashMap<>(); + expected.put(Name.of("foo"), Name.of("bar")); + expected.put(Name.of("baz"), null); + assertEquals(expected, query.entries()); + + expected.remove(Name.of("baz")); + assertEquals(expected, query.remove("baz").entries()); + + expected.put(Name.of("baz"), null); + expected.remove(Name.of("foo")); + assertEquals(expected, query.remove("foo").entries()); + assertEquals(expected, Query.empty(Name::of).add("baz").entries()); + + assertEquals("query '?foo=bar&baz=bax&quu=fez&moo'", + query.put("baz", "bax").merge(Query.from(Map.of("quu", "fez"))).add("moo").toString()); + + assertThrows(NullPointerException.class, + () -> query.remove(null)); + + assertThrows(NullPointerException.class, + () -> query.add(null)); + + assertThrows(NullPointerException.class, + () -> query.put(null, "hax")); + + assertThrows(NullPointerException.class, + () -> query.put("hax", null)); + + Map names = new LinkedHashMap<>(); + names.put(null, Name.of("hax")); + assertThrows(NullPointerException.class, + () -> query.merge(names)); + } + } -- cgit v1.2.3