API brush-up, more unit test coverage

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<T> {
             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<T> {
         return create(scheme, domain, port, path, query);
     }
 
+    public HttpURL<T> withPort(int port) {
+        return create(scheme, domain, port, path, query);
+    }
+
+    public HttpURL<T> withoutPort() {
+        return create(scheme, domain, -1, path, query);
+    }
+
     public HttpURL<T> withPath(Path<T> path) {
         return create(scheme, domain, port, path, query);
     }
@@ -172,14 +181,23 @@ public class HttpURL<T> {
             this.inverse = requireNonNull(inverse);
         }
 
+        /** Creates a new, empty path, with a trailing slash. */
+        public static Path<String> 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 <T extends StringWrapper<T>> Path<T> empty(Function<String, T> validator) {
             return new Path<>(List.of(), true, validator, T::value);
         }
+        /** Creates a new path with the given <em>decoded</em> segments. */
+        public static Path<String> from(List<String> segments) {
+            return empty().append(segments);
+        }
 
-        /** Creates a new, empty path, with a trailing slash. */
-        public static Path<String> empty() {
-            return new Path<>(List.of(), true, identity(), identity());
+        /** Creates a new path with the given <em>decoded</em> segments, and the validator applied to each segment. */
+        public static <T extends StringWrapper<T>> Path<T> from(List<String> segments, Function<String, T> 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<T> {
                            segment, "path segments cannot be \"\", \".\", or \"..\"");
         }
 
-        /** Returns a copy of this which eliminates the given number of segments, from the root down. */
-        public Path<T> 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<T> 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<T> 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<T> cut(int count) {
+            return new Path<>(segments.subList(0, segments.size() - count), trailingSlash, validator, inverse);
         }
 
         /** Returns a copy of this with the <em>decoded</em> segment appended at the end; it may not be either of {@code ""}, {@code "."} or {@code ".."}. */
-        public Path<T> with(T segment) {
-            List<T> copy = new ArrayList<>(segments);
-            copy.add(requireNonNull(segment));
-            return new Path<>(copy, trailingSlash, validator, inverse);
+        public Path<T> 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 <U> Path<T> with(Path<U> other) {
-            List<T> 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 <U> Path<T> append(Path<U> 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<T> append(List<T> segments) {
+            return append(segments, inverse, trailingSlash);
+        }
+
+        private <U> Path<T> append(List<U> segments, Function<U, String> inverse, boolean trailingSlash) {
+            List<T> 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<T> {
             return new Path<>(segments, false, validator, inverse);
         }
 
-        /** The URL decoded segments that make up this path; never {@code ""}, {@code "."} or {@code ".."}. */
-        public List<T> decoded() {
+        /** The <em>URL decoded</em> segments that make up this path; never {@code null}, {@code ""}, {@code "."} or {@code ".."}. */
+        public List<T> segments() {
             return Collections.unmodifiableList(segments);
         }
 
@@ -256,6 +280,25 @@ public class HttpURL<T> {
             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<T> {
             this.inverse = requireNonNull(inverse);
         }
 
+        /** Creates a new, empty query part. */
+        public static Query<String> 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 <T extends StringWrapper<T>> Query<T> empty(Function<String, T> validator) {
+            return new Query<>(Map.of(), validator, T::value);
+        }
+        /** Creates a new query part with the given <em>decoded</em> values. */
+        public static Query<String> from(Map<String, String> values) {
+            return empty().merge(values);
+        }
+
+        /** Creates a new query part with the given <em>decoded</em> values, and the validator applied to each pair. */
+        public static <T extends StringWrapper<T>> Query<T> from(Map<String, String> values, Function<String, T> 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 <T extends StringWrapper<T>> Query<T> parse(String raw, Function<String, T> validator) {
             return parse(raw, validator, T::value);
@@ -294,39 +356,46 @@ public class HttpURL<T> {
             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 <T extends StringWrapper<T>> Query<T> empty(Function<String, T> validator) {
-            return new Query<T>(Map.of(), validator, T::value);
-        }
-
-        /** Creates a new, empty query part. */
-        public static <T> Query<String> empty() {
-            return new Query<>(Map.of(), identity(), identity());
-        }
-
         /** Returns a copy of this with the <em>decoded</em> non-null key pointing to the <em>decoded</em> non-null value. */
-        public Query<T> with(T key, T value) {
+        public Query<T> put(String key, String value) {
             Map<T, T> 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 <em>decoded</em> non-null key pointing to "nothing". */
-        public Query<T> with(T key) {
+        public Query<T> add(String key) {
             Map<T, T> 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 <em>decoded</em> key. */
-        public Query<T> without(T key) {
+        public Query<T> remove(String key) {
             Map<T, T> 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<T, T> decoded() {
+        /** Returns a copy of this with all mappings from the other query added to this, possibly overwriting existing mappings. */
+        public <U> Query<T> merge(Query<U> 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<T> merge(Map<T, T> values) {
+            return merge(values, inverse);
+        }
+
+        private <U> Query<T> merge(Map<U, U> values, Function<U, String> inverse) {
+            Map<T, T> 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 <em>URL decoded</em> 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<T, T> entries() {
             return unmodifiableMap(values);
         }
 
@@ -338,6 +407,25 @@ public class HttpURL<T> {
             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<Name> 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<Name> path = HttpURL.Path.parse("foo/bar/baz", Name::of);
+        List<Name> 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<Name> 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<Name> query = Query.parse("foo=bar&baz", Name::of);
+        Map<Name, Name> 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<Name, Name> names = new LinkedHashMap<>();
+        names.put(null, Name.of("hax"));
+        assertThrows(NullPointerException.class,
+                     () -> query.merge(names));
+    }
+
 }