-
Notifications
You must be signed in to change notification settings - Fork 39
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.
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
Fix partial match #622
Fix partial match #622
Changes from 4 commits
5dd4203
af2e0db
8f09a3e
94779d3
eb66229
f755008
569ccfd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,14 +13,14 @@ | |
|
||
package org.apache.pekko.http.impl.engine.http2 | ||
|
||
import com.typesafe.config.ConfigFactory | ||
import java.time.format.DateTimeFormatter | ||
import org.apache.pekko | ||
import pekko.http.scaladsl.settings.{ ClientConnectionSettings, ServerSettings } | ||
NavidJalali marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import pekko.event.NoLogging | ||
import pekko.http.impl.engine.rendering.DateHeaderRendering | ||
import pekko.http.scaladsl.model.headers._ | ||
import pekko.http.scaladsl.model.{ ContentTypes, DateTime, HttpHeader, TransferEncodings } | ||
|
||
import scala.collection.immutable.Seq | ||
import pekko.http.scaladsl.model._ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. keep the imports in some order - why are you changing the order of these imports? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've committed a change to the PR branch to try to order the imports There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was a bit too slow 😄 |
||
import scala.collection.immutable.VectorBuilder | ||
import scala.util.Try | ||
import org.scalatest.matchers.should.Matchers | ||
|
@@ -47,7 +47,7 @@ | |
ETag("tagetitag"), | ||
RawHeader("raw", "whatever"), | ||
new MyCustomHeader("whatever", renderInResponses = true)) | ||
renderServerHeaders(headers, builder) | ||
Check failure on line 50 in http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRenderingSpec.scala GitHub Actions / Compile and test (2.12, 8)
|
||
val out = builder.result() | ||
out.exists(_._1 == "etag") shouldBe true | ||
out.exists(_._1 == "raw") shouldBe true | ||
|
@@ -56,7 +56,7 @@ | |
|
||
"add a date header when none is present" in { | ||
val builder = new VectorBuilder[(String, String)] | ||
renderServerHeaders(Seq.empty, builder) | ||
Check failure on line 59 in http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRenderingSpec.scala GitHub Actions / Compile and test (2.12, 8)
|
||
val date = builder.result().collectFirst { | ||
case ("date", str) => str | ||
} | ||
|
@@ -70,7 +70,7 @@ | |
"keep the date header if it already is present" in { | ||
val builder = new VectorBuilder[(String, String)] | ||
val originalDateTime = DateTime(1981, 3, 6, 20, 30, 24) | ||
renderServerHeaders(Seq(Date(originalDateTime)), builder) | ||
Check failure on line 73 in http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRenderingSpec.scala GitHub Actions / Compile and test (2.12, 8)
|
||
val date = builder.result().collectFirst { | ||
case ("date", str) => str | ||
} | ||
|
@@ -80,14 +80,14 @@ | |
|
||
"add server header if default provided (in server mode)" in { | ||
val builder = new VectorBuilder[(String, String)] | ||
renderServerHeaders(Seq.empty, builder, Some(("server", "default server"))) | ||
Check failure on line 83 in http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRenderingSpec.scala GitHub Actions / Compile and test (2.12, 8)
|
||
val result = builder.result().find(_._1 == "server").map(_._2) | ||
result shouldEqual Some("default server") | ||
} | ||
|
||
"keep server header if explicitly provided (in server mode)" in { | ||
val builder = new VectorBuilder[(String, String)] | ||
renderServerHeaders(Seq(Server("explicit server")), builder, Some(("server", "default server"))) | ||
Check failure on line 90 in http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRenderingSpec.scala GitHub Actions / Compile and test (2.12, 8)
|
||
val result = builder.result().find(_._1 == "server").map(_._2) | ||
result shouldEqual Some("explicit server") | ||
} | ||
|
@@ -99,7 +99,7 @@ | |
`Content-Length`(42L), | ||
`Content-Type`(ContentTypes.`application/json`), | ||
`Transfer-Encoding`(TransferEncodings.gzip)) | ||
renderServerHeaders(invalidExplicitHeaders, builder) | ||
Check failure on line 102 in http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRenderingSpec.scala GitHub Actions / Compile and test (2.12, 8)
|
||
builder.result().exists(_._1 != "date") shouldBe false | ||
} | ||
|
||
|
@@ -108,7 +108,7 @@ | |
val shouldNotBeRendered = Seq( | ||
Host("example.com", 80), | ||
new MyCustomHeader("whatever", renderInResponses = false)) | ||
renderServerHeaders(shouldNotBeRendered, builder) | ||
Check failure on line 111 in http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRenderingSpec.scala GitHub Actions / Compile and test (2.12, 8)
|
||
val value1 = builder.result() | ||
value1.exists(_._1 != "date") shouldBe false | ||
} | ||
|
@@ -118,21 +118,21 @@ | |
val invalidRawHeaders = Seq( | ||
"connection", "content-length", "content-type", "transfer-encoding", "date", "server").map(name => | ||
RawHeader(name, "whatever")) | ||
renderServerHeaders(invalidRawHeaders, builder) | ||
Check failure on line 121 in http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRenderingSpec.scala GitHub Actions / Compile and test (2.12, 8)
|
||
builder.result().exists(_._1 != "date") shouldBe false | ||
} | ||
|
||
// Client-specific tests | ||
"add user-agent header if default provided (in client mode)" in { | ||
val builder = new VectorBuilder[(String, String)] | ||
renderClientHeaders(Seq(`User-Agent`("fancy browser")), builder) | ||
Check failure on line 128 in http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRenderingSpec.scala GitHub Actions / Compile and test (2.12, 8)
|
||
val result = builder.result().find(_._1 == "user-agent").map(_._2) | ||
result shouldEqual Some("fancy browser") | ||
} | ||
|
||
"keep user-agent header if explicitly provided (in client mode)" in { | ||
val builder = new VectorBuilder[(String, String)] | ||
renderClientHeaders(Seq(`User-Agent`("fancier client")), builder, Some(("user-agent", "fancy browser"))) | ||
Check failure on line 135 in http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRenderingSpec.scala GitHub Actions / Compile and test (2.12, 8)
|
||
val result = builder.result().find(_._1 == "user-agent").map(_._2) | ||
result shouldEqual Some("fancier client") | ||
} | ||
|
@@ -147,21 +147,39 @@ | |
value1.exists(_._1 == "date") shouldBe false | ||
} | ||
|
||
"handle empty trailer" in { | ||
val config = ConfigFactory.load("reference.conf") | ||
Try { | ||
val rendering = new RequestRendering(ClientConnectionSettings(config), NoLogging) | ||
rendering(HttpRequest().withAttributes(Map(AttributeKeys.trailer -> Trailer()))) | ||
}.isSuccess shouldBe true | ||
Try { | ||
val rendering = new ResponseRendering(ServerSettings(config), NoLogging, dateHeaderRendering) | ||
rendering( | ||
HttpResponse().withAttributes( | ||
Map( | ||
AttributeKeys.trailer -> Trailer(), | ||
Http2.streamId -> 0))) | ||
}.isSuccess shouldBe true | ||
} | ||
|
||
} | ||
|
||
private def renderClientHeaders(headers: immutable.Seq[HttpHeader], builder: VectorBuilder[(String, String)], | ||
Check warning on line 168 in http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRenderingSpec.scala GitHub Actions / Compile and test (2.12, 8)
|
||
NavidJalali marked this conversation as resolved.
Show resolved
Hide resolved
|
||
peerIdHeader: Option[(String, String)] = None): Unit = | ||
Check warning on line 169 in http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRenderingSpec.scala GitHub Actions / Compile and test (2.12, 8)
|
||
HttpMessageRendering.renderHeaders(headers, builder, peerIdHeader, NoLogging, isServer = false, | ||
shouldRenderAutoHeaders = true, dateHeaderRendering = DateHeaderRendering.Unavailable) | ||
|
||
private def renderServerHeaders(headers: immutable.Seq[HttpHeader], builder: VectorBuilder[(String, String)], | ||
peerIdHeader: Option[(String, String)] = None): Unit = | ||
HttpMessageRendering.renderHeaders(headers, builder, peerIdHeader, NoLogging, isServer = true, | ||
shouldRenderAutoHeaders = true, | ||
dateHeaderRendering = new DateHeaderRendering { | ||
// fake date rendering | ||
override def renderHeaderPair(): (String, String) = "date" -> DateTime.now.toRfc1123DateTimeString | ||
override def renderHeaderBytes(): Array[Byte] = ??? | ||
override def renderHeaderValue(): String = ??? | ||
}) | ||
dateHeaderRendering = dateHeaderRendering) | ||
|
||
private lazy val dateHeaderRendering: DateHeaderRendering = new DateHeaderRendering { | ||
// fake date rendering | ||
override def renderHeaderPair(): (String, String) = "date" -> DateTime.now.toRfc1123DateTimeString | ||
override def renderHeaderBytes(): Array[Byte] = ??? | ||
override def renderHeaderValue(): String = ??? | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the question here is if someone added a
AttributeKeys.trailer
but it contained no headers, should we produce aParsedHeadersFrame
here? Looking atHttp2SubStream
/OutStreamImpl
, it does seem safe (it will produce an emptyDataFrame
withendStream = true
if necessary). It seems unlikely that any downstream implementation would rely on receiving an empty frame of trailing headers.All in all I think this is OK, though adding a
AttributeKeys.trailer
without any headers seems strange, the code that caused that might deserve a closer look.