Fix partial match #622
Fix partial match #622
Conversation
|
|
@pjfanning I am honestly not sure what this code is really doing so I am a bit hesitant to assert things about this. I will have a look at the related tests to see if I can confidently test it |
|
@pjfanning can you take another look when you get the time? |
...-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRenderingSpec.scala
Outdated
Show resolved
Hide resolved
...-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRenderingSpec.scala
Show resolved
Hide resolved
NavidJalali
force-pushed
the
fix-partial-match
branch
from
c01cc2d
to
8f09a3e
Compare
...-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/HttpMessageRenderingSpec.scala
Outdated
Show resolved
Hide resolved
| 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.
keep the imports in some order - why are you changing the order of these imports?
There was a problem hiding this comment.
I've committed a change to the PR branch to try to order the imports
There was a problem hiding this comment.
I was a bit too slow 😄
|
Thanks @NavidJalali - merging |
|
@mdedetrich this looks useful to backport to 1.0.x - wdyt? |
| case Some(trailer) if trailer.headers.nonEmpty => | ||
| OptionVal.Some(ParsedHeadersFrame(streamId, endStream = true, trailer.headers, None)) | ||
| case None => OptionVal.None | ||
| case _ => OptionVal.None |
There was a problem hiding this comment.
So the question here is if someone added a AttributeKeys.trailer but it contained no headers, should we produce a ParsedHeadersFrame here? Looking at Http2SubStream/OutStreamImpl, it does seem safe (it will produce an empty DataFrame with endStream = 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.
|
@raboof this relates to apache/pekko-grpc#391 |
I hit this case in a test and it threw. I am not sure if when
trailer.headersis empty, it should return None or a parsed frame with empty headers but I figured the guard was there for a reason