From 2869d532de6a5a642fb42374eb1240eaa50aaa9f Mon Sep 17 00:00:00 2001 From: Yisraelu Date: Tue, 20 Dec 2022 13:40:31 -0500 Subject: [PATCH 1/8] fix path segment parsing when suffixed with query --- .../src/smithy4s/http/internals/package.scala | 17 +++++++++++------ .../src/smithy4s/http/internals/PathSpec.scala | 2 +- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/modules/core/src/smithy4s/http/internals/package.scala b/modules/core/src/smithy4s/http/internals/package.scala index ddaaafa5f..9158c1882 100644 --- a/modules/core/src/smithy4s/http/internals/package.scala +++ b/modules/core/src/smithy4s/http/internals/package.scala @@ -63,12 +63,17 @@ package object internals { } private def fromToString(str: String): Option[PathSegment] = { - if (str.isEmpty()) None - else if (str.startsWith("{") && str.endsWith("+}")) - Some(PathSegment.greedy(str.substring(1, str.length() - 2))) - else if (str.startsWith("{") && str.endsWith("}")) - Some(PathSegment.label(str.substring(1, str.length() - 1))) - else Some(PathSegment.static(str)) + Option(str).map { str => + { + // handle query params in path + val sanitized = str.split('?').head + if (sanitized.startsWith("{") && sanitized.endsWith("+}")) + PathSegment.greedy(sanitized.substring(1, sanitized.length() - 2)) + else if (sanitized.startsWith("{") && sanitized.endsWith("}")) + PathSegment.label(sanitized.substring(1, sanitized.length() - 1)) + else PathSegment.static(sanitized) + } + } } } diff --git a/modules/core/test/src/smithy4s/http/internals/PathSpec.scala b/modules/core/test/src/smithy4s/http/internals/PathSpec.scala index d56d8b7af..983fce1fe 100644 --- a/modules/core/test/src/smithy4s/http/internals/PathSpec.scala +++ b/modules/core/test/src/smithy4s/http/internals/PathSpec.scala @@ -46,7 +46,7 @@ class PathSpec() extends munit.FunSuite { } test("Parse path pattern into path segments") { - val result = pathSegments("/{head}/foo/{tail+}") + val result = pathSegments("/{head}/foo/{tail+}?hello=world") expect( result == Option( Vector( From 98a4a280a30349a849ac5ab51c218706bd4d41c1 Mon Sep 17 00:00:00 2001 From: Yisraelu Date: Tue, 20 Dec 2022 13:57:22 -0500 Subject: [PATCH 2/8] add back filtering for non emptiness on string --- modules/core/src/smithy4s/http/internals/package.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/src/smithy4s/http/internals/package.scala b/modules/core/src/smithy4s/http/internals/package.scala index 9158c1882..2e9a767a2 100644 --- a/modules/core/src/smithy4s/http/internals/package.scala +++ b/modules/core/src/smithy4s/http/internals/package.scala @@ -63,7 +63,7 @@ package object internals { } private def fromToString(str: String): Option[PathSegment] = { - Option(str).map { str => + Option(str).filter(_.nonEmpty).map { str => { // handle query params in path val sanitized = str.split('?').head From 2c167c854663a5f4fb014042261c53b1a7b1526e Mon Sep 17 00:00:00 2001 From: Yisraelu Date: Wed, 21 Dec 2022 09:59:03 -0500 Subject: [PATCH 3/8] adds separate test for parsing path params from path with query params --- .../src/smithy4s/http/internals/PathSpec.scala | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/modules/core/test/src/smithy4s/http/internals/PathSpec.scala b/modules/core/test/src/smithy4s/http/internals/PathSpec.scala index 983fce1fe..f2bf1fc82 100644 --- a/modules/core/test/src/smithy4s/http/internals/PathSpec.scala +++ b/modules/core/test/src/smithy4s/http/internals/PathSpec.scala @@ -46,7 +46,19 @@ class PathSpec() extends munit.FunSuite { } test("Parse path pattern into path segments") { - val result = pathSegments("/{head}/foo/{tail+}?hello=world") + val result = pathSegments("/{head}/foo/{tail+}") + expect( + result == Option( + Vector( + PathSegment.label("head"), + PathSegment.static("foo"), + PathSegment.greedy("tail") + ) + ) + ) + } + test("Parse path pattern from path that has query param into path segments") { + val result = pathSegments("/{head}/foo/{tail+}?hello=world&hi") expect( result == Option( Vector( @@ -57,7 +69,6 @@ class PathSpec() extends munit.FunSuite { ) ) } - test("Write PathParams for DummyPath") { val result = HttpEndpoint .cast( From 21ba6e053bd85f8402d1a6843559c24c1c846b05 Mon Sep 17 00:00:00 2001 From: Yisraelu Date: Wed, 4 Jan 2023 15:39:33 -0500 Subject: [PATCH 4/8] Changes: PathSegment implementation returns just the path, minus any query parameters A staticQueryParams method has been added HttpEndpoint, the Http4s client calls that method to retrieve any static query params and pass them on Tests: metadata.smithy has been modified and PathSpec includes tests for testing the HttpEndpoint functionality --- .../core/src/smithy4s/http/HttpEndpoint.scala | 6 +++ .../src/smithy4s/http/internals/package.scala | 39 ++++++++++++++----- .../smithy4s/http/internals/PathSpec.scala | 29 ++++++++++++++ .../SmithyHttp4sClientEndpoint.scala | 4 +- sampleSpecs/metadata.smithy | 2 +- 5 files changed, 68 insertions(+), 12 deletions(-) diff --git a/modules/core/src/smithy4s/http/HttpEndpoint.scala b/modules/core/src/smithy4s/http/HttpEndpoint.scala index 2d8ef0e82..6e38c90d4 100644 --- a/modules/core/src/smithy4s/http/HttpEndpoint.scala +++ b/modules/core/src/smithy4s/http/HttpEndpoint.scala @@ -26,6 +26,9 @@ trait HttpEndpoint[I] { // Returns a path template as a list of segments, which can be constant strings or placeholders. def path: List[PathSegment] + + // Returns a map of static query parameters that are found in the uri of Http hint, there cannot be multiple of the same key !. + def staticQueryParams: Map[String, String] def method: HttpMethod def code: Int @@ -48,6 +51,7 @@ object HttpEndpoint { .get(Http) .toRight(HttpEndpointError("Operation doesn't have a @http trait")) httpMethod = HttpMethod.fromStringOrDefault(http.method.value) + queryParams = internals.staticQueryParams(http.uri.value) httpPath <- internals .pathSegments(http.uri.value) .toRight( @@ -55,6 +59,7 @@ object HttpEndpoint { s"Unable to parse HTTP path template: ${http.uri.value}" ) ) + encoder <- SchemaVisitorPathEncoder( endpoint.input.addHints(http) ).toRight( @@ -64,6 +69,7 @@ object HttpEndpoint { } yield { new HttpEndpoint[I] { def path(input: I): List[String] = encoder.encode(input) + val staticQueryParams: Map[String,String] = queryParams val path: List[PathSegment] = httpPath.toList val method: HttpMethod = httpMethod val code: Int = http.code diff --git a/modules/core/src/smithy4s/http/internals/package.scala b/modules/core/src/smithy4s/http/internals/package.scala index 2e9a767a2..486b85843 100644 --- a/modules/core/src/smithy4s/http/internals/package.scala +++ b/modules/core/src/smithy4s/http/internals/package.scala @@ -56,22 +56,41 @@ package object internals { str: String ): Option[Vector[PathSegment]] = { str - .split('/') - .toVector - .filterNot(_.isEmpty()) - .traverse(fromToString(_)) + .split('?') + .headOption + .flatMap( + _.split('/').toVector + .filterNot(_.isEmpty()) + .traverse(fromToString(_)) + ) + } + + private[http] def staticQueryParams( + uri: String + ): Map[String, String] = { + uri.split("\\?", 2) match { + case Array(_) => Map.empty + case Array(_, query) => + query.split("&").toList.foldLeft(Map.empty[String, String]) { + case (acc, param) => + val (k, v) = param.split("=", 2) match { + case Array(key) => (key, "") + case Array(key, value) => (key, value) + } + acc.updated(k, v) + } + } } private def fromToString(str: String): Option[PathSegment] = { Option(str).filter(_.nonEmpty).map { str => { // handle query params in path - val sanitized = str.split('?').head - if (sanitized.startsWith("{") && sanitized.endsWith("+}")) - PathSegment.greedy(sanitized.substring(1, sanitized.length() - 2)) - else if (sanitized.startsWith("{") && sanitized.endsWith("}")) - PathSegment.label(sanitized.substring(1, sanitized.length() - 1)) - else PathSegment.static(sanitized) + if (str.startsWith("{") && str.endsWith("+}")) + PathSegment.greedy(str.substring(1, str.length() - 2)) + else if (str.startsWith("{") && str.endsWith("}")) + PathSegment.label(str.substring(1, str.length() - 1)) + else PathSegment.static(str) } } } diff --git a/modules/core/test/src/smithy4s/http/internals/PathSpec.scala b/modules/core/test/src/smithy4s/http/internals/PathSpec.scala index f2bf1fc82..266554538 100644 --- a/modules/core/test/src/smithy4s/http/internals/PathSpec.scala +++ b/modules/core/test/src/smithy4s/http/internals/PathSpec.scala @@ -69,6 +69,35 @@ class PathSpec() extends munit.FunSuite { ) ) } + test("parse static query params from DummyPath") { + val httpEndpoint = HttpEndpoint + .cast( + DummyPath + ) + .toOption + .get + + val sqp = httpEndpoint.staticQueryParams + val path = httpEndpoint.path + + val expectedQueryMap = Map("value" -> "foo", "baz" -> "bar") + expect(sqp == expectedQueryMap) + expect( + path == + List( + PathSegment.static("dummy-path"), + PathSegment.label("str"), + PathSegment.label("int"), + PathSegment.label("ts1"), + PathSegment.label("ts2"), + PathSegment.label("ts3"), + PathSegment.label("ts4"), + PathSegment.label("b"), + PathSegment.label("ie") + ) + ) + } + test("Write PathParams for DummyPath") { val result = HttpEndpoint .cast( diff --git a/modules/http4s/src/smithy4s/http4s/internals/SmithyHttp4sClientEndpoint.scala b/modules/http4s/src/smithy4s/http4s/internals/SmithyHttp4sClientEndpoint.scala index ee4ecb066..ccf1b41be 100644 --- a/modules/http4s/src/smithy4s/http4s/internals/SmithyHttp4sClientEndpoint.scala +++ b/modules/http4s/src/smithy4s/http4s/internals/SmithyHttp4sClientEndpoint.scala @@ -114,9 +114,11 @@ private[http4s] class SmithyHttp4sClientEndpointImpl[F[_], Op[_, _, _, _, _], I, def inputToRequest(input: I): Request[F] = { val metadata = inputMetadataEncoder.encode(input) val path = httpEndpoint.path(input) + val staticQueries = httpEndpoint.staticQueryParams val uri = baseUri .copy(path = baseUri.path.addSegments(path.map(Uri.Path.Segment(_)))) - .withMultiValueQueryParams(metadata.query) + .withQueryParams(staticQueries) + .withMultiValueQueryParams( metadata.query) val headers = toHeaders(metadata.headers) val baseRequest = Request[F](method, uri, headers = headers) if (inputHasBody) { diff --git a/sampleSpecs/metadata.smithy b/sampleSpecs/metadata.smithy index ff30786ea..f7735be6e 100644 --- a/sampleSpecs/metadata.smithy +++ b/sampleSpecs/metadata.smithy @@ -15,7 +15,7 @@ operation Dummy { input: Queries } -@http(method: "GET", uri: "/dummy-path/{str}/{int}/{ts1}/{ts2}/{ts3}/{ts4}/{b}/{ie}") +@http(method: "GET", uri: "/dummy-path/{str}/{int}/{ts1}/{ts2}/{ts3}/{ts4}/{b}/{ie}?value=foo&baz=bar") @readonly operation DummyPath { input: PathParams From f70d3cb936b619869d5cc9068abecbbbf72602e4 Mon Sep 17 00:00:00 2001 From: Yisraelu Date: Wed, 4 Jan 2023 16:09:51 -0500 Subject: [PATCH 5/8] formatting --- modules/core/src/smithy4s/http/HttpEndpoint.scala | 2 +- .../smithy4s/http4s/internals/SmithyHttp4sClientEndpoint.scala | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/core/src/smithy4s/http/HttpEndpoint.scala b/modules/core/src/smithy4s/http/HttpEndpoint.scala index 6e38c90d4..668a6093f 100644 --- a/modules/core/src/smithy4s/http/HttpEndpoint.scala +++ b/modules/core/src/smithy4s/http/HttpEndpoint.scala @@ -69,7 +69,7 @@ object HttpEndpoint { } yield { new HttpEndpoint[I] { def path(input: I): List[String] = encoder.encode(input) - val staticQueryParams: Map[String,String] = queryParams + val staticQueryParams: Map[String, String] = queryParams val path: List[PathSegment] = httpPath.toList val method: HttpMethod = httpMethod val code: Int = http.code diff --git a/modules/http4s/src/smithy4s/http4s/internals/SmithyHttp4sClientEndpoint.scala b/modules/http4s/src/smithy4s/http4s/internals/SmithyHttp4sClientEndpoint.scala index ccf1b41be..f1c25e2e2 100644 --- a/modules/http4s/src/smithy4s/http4s/internals/SmithyHttp4sClientEndpoint.scala +++ b/modules/http4s/src/smithy4s/http4s/internals/SmithyHttp4sClientEndpoint.scala @@ -118,7 +118,7 @@ private[http4s] class SmithyHttp4sClientEndpointImpl[F[_], Op[_, _, _, _, _], I, val uri = baseUri .copy(path = baseUri.path.addSegments(path.map(Uri.Path.Segment(_)))) .withQueryParams(staticQueries) - .withMultiValueQueryParams( metadata.query) + .withMultiValueQueryParams(metadata.query) val headers = toHeaders(metadata.headers) val baseRequest = Request[F](method, uri, headers = headers) if (inputHasBody) { From 151f71935bedef321b488915ef61dfe50ac489b8 Mon Sep 17 00:00:00 2001 From: Yisraelu Date: Thu, 5 Jan 2023 10:08:19 -0500 Subject: [PATCH 6/8] revert staticQueryParams to Map[String,Seq[String]] for future proofing sake --- modules/core/src/smithy4s/http/HttpEndpoint.scala | 6 +++--- modules/core/src/smithy4s/http/internals/package.scala | 6 +++--- .../http4s/internals/SmithyHttp4sClientEndpoint.scala | 3 +-- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/modules/core/src/smithy4s/http/HttpEndpoint.scala b/modules/core/src/smithy4s/http/HttpEndpoint.scala index 668a6093f..28b3d609c 100644 --- a/modules/core/src/smithy4s/http/HttpEndpoint.scala +++ b/modules/core/src/smithy4s/http/HttpEndpoint.scala @@ -27,8 +27,8 @@ trait HttpEndpoint[I] { // Returns a path template as a list of segments, which can be constant strings or placeholders. def path: List[PathSegment] - // Returns a map of static query parameters that are found in the uri of Http hint, there cannot be multiple of the same key !. - def staticQueryParams: Map[String, String] + // Returns a map of static query parameters that are found in the uri of Http hint. + def staticQueryParams: Map[String, Seq[String]] def method: HttpMethod def code: Int @@ -69,7 +69,7 @@ object HttpEndpoint { } yield { new HttpEndpoint[I] { def path(input: I): List[String] = encoder.encode(input) - val staticQueryParams: Map[String, String] = queryParams + val staticQueryParams: Map[String, Seq[String]] = queryParams val path: List[PathSegment] = httpPath.toList val method: HttpMethod = httpMethod val code: Int = http.code diff --git a/modules/core/src/smithy4s/http/internals/package.scala b/modules/core/src/smithy4s/http/internals/package.scala index 486b85843..118fb142e 100644 --- a/modules/core/src/smithy4s/http/internals/package.scala +++ b/modules/core/src/smithy4s/http/internals/package.scala @@ -67,17 +67,17 @@ package object internals { private[http] def staticQueryParams( uri: String - ): Map[String, String] = { + ): Map[String, Seq[String]] = { uri.split("\\?", 2) match { case Array(_) => Map.empty case Array(_, query) => - query.split("&").toList.foldLeft(Map.empty[String, String]) { + query.split("&").toList.foldLeft(Map.empty[String, Seq[String]]) { case (acc, param) => val (k, v) = param.split("=", 2) match { case Array(key) => (key, "") case Array(key, value) => (key, value) } - acc.updated(k, v) + acc.updated(k, acc.getOrElse(k, Seq.empty) :+ v) } } } diff --git a/modules/http4s/src/smithy4s/http4s/internals/SmithyHttp4sClientEndpoint.scala b/modules/http4s/src/smithy4s/http4s/internals/SmithyHttp4sClientEndpoint.scala index f1c25e2e2..5bc234ac4 100644 --- a/modules/http4s/src/smithy4s/http4s/internals/SmithyHttp4sClientEndpoint.scala +++ b/modules/http4s/src/smithy4s/http4s/internals/SmithyHttp4sClientEndpoint.scala @@ -117,8 +117,7 @@ private[http4s] class SmithyHttp4sClientEndpointImpl[F[_], Op[_, _, _, _, _], I, val staticQueries = httpEndpoint.staticQueryParams val uri = baseUri .copy(path = baseUri.path.addSegments(path.map(Uri.Path.Segment(_)))) - .withQueryParams(staticQueries) - .withMultiValueQueryParams(metadata.query) + .withMultiValueQueryParams(staticQueries ++ metadata.query) val headers = toHeaders(metadata.headers) val baseRequest = Request[F](method, uri, headers = headers) if (inputHasBody) { From 7bff3d81fa9652fa106ce406a8d7de6463374aca Mon Sep 17 00:00:00 2001 From: Yisraelu Date: Thu, 5 Jan 2023 10:34:01 -0500 Subject: [PATCH 7/8] fix test for new statiq query params interface --- modules/core/test/src/smithy4s/http/internals/PathSpec.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/test/src/smithy4s/http/internals/PathSpec.scala b/modules/core/test/src/smithy4s/http/internals/PathSpec.scala index 266554538..bc1af52c2 100644 --- a/modules/core/test/src/smithy4s/http/internals/PathSpec.scala +++ b/modules/core/test/src/smithy4s/http/internals/PathSpec.scala @@ -80,7 +80,7 @@ class PathSpec() extends munit.FunSuite { val sqp = httpEndpoint.staticQueryParams val path = httpEndpoint.path - val expectedQueryMap = Map("value" -> "foo", "baz" -> "bar") + val expectedQueryMap = Map("value" -> Seq("foo"), "baz" -> Seq("bar")) expect(sqp == expectedQueryMap) expect( path == From 9a014a68ab8bec34ba28ae59d9009b9a58e0cf5d Mon Sep 17 00:00:00 2001 From: Yisraelu Date: Thu, 5 Jan 2023 14:15:11 -0500 Subject: [PATCH 8/8] removed extra allocations --- .../src/smithy4s/http/internals/package.scala | 29 ++++++++----------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/modules/core/src/smithy4s/http/internals/package.scala b/modules/core/src/smithy4s/http/internals/package.scala index 118fb142e..5683f568b 100644 --- a/modules/core/src/smithy4s/http/internals/package.scala +++ b/modules/core/src/smithy4s/http/internals/package.scala @@ -57,12 +57,12 @@ package object internals { ): Option[Vector[PathSegment]] = { str .split('?') - .headOption - .flatMap( - _.split('/').toVector - .filterNot(_.isEmpty()) - .traverse(fromToString(_)) - ) + .head + .split('/') + .toVector + .filterNot(_.isEmpty()) + .traverse(fromToString(_)) + } private[http] def staticQueryParams( @@ -83,16 +83,11 @@ package object internals { } private def fromToString(str: String): Option[PathSegment] = { - Option(str).filter(_.nonEmpty).map { str => - { - // handle query params in path - if (str.startsWith("{") && str.endsWith("+}")) - PathSegment.greedy(str.substring(1, str.length() - 2)) - else if (str.startsWith("{") && str.endsWith("}")) - PathSegment.label(str.substring(1, str.length() - 1)) - else PathSegment.static(str) - } - } + if (str == null || str.isEmpty) None + else if (str.startsWith("{") && str.endsWith("+}")) + Some(PathSegment.greedy(str.substring(1, str.length() - 2))) + else if (str.startsWith("{") && str.endsWith("}")) + Some(PathSegment.label(str.substring(1, str.length() - 1))) + else Some(PathSegment.static(str)) } - }