fix parse broken when header value is null #575
Conversation
http-core/src/main/scala/org/apache/pekko/http/scaladsl/model/HttpHeader.scala
Outdated
Show resolved
Hide resolved
| @@ -876,6 +876,11 @@ class HttpHeaderSpec extends AnyFreeSpec with Matchers { | |||
| parse("User-Agent", "(" * 10000).errors.head shouldEqual | |||
| ErrorInfo("Illegal HTTP header 'User-Agent': Illegal header value", "Header comment nested too deeply") | |||
| } | |||
|
|
|||
| "should not broken when header-value is null" in { | |||
| parse("Content-Disposition", null).errors.head.isInstanceOf[ErrorInfo] shouldBe true | |||
There was a problem hiding this comment.
Please use:
parse("Content-Disposition", null).errors.head shouldBe an[ErrorInfo]
Your version will return an error like false os not true if it fails. My version will return a much more informative message.
| }).info | ||
| val info = error match { | ||
| case e: ParseError => parser.parseError(e).info | ||
| case e if e.getMessage == null => ErrorInfo() |
There was a problem hiding this comment.
I think this code should be reverted and the change should be in HeaderParser.scala instead. I have the change in #576. I created 576 to test if that approach works but I'd prefer to let you complete this PR. View 576 as a set of suggested changes for this PR.
There was a problem hiding this comment.
Thanks for your suggestion, e.toString() is good for NPE in java8/java11
|
|
||
| "should not broken when header-value is null" in { | ||
| val errors = parse("Content-Disposition", null).errors | ||
| errors.size shouldBe 1 |
There was a problem hiding this comment.
errors should have size 1 is a better way to do this. When this fails, scalatest will say something like
List(a1, a2) did not have size 1 - where a1 and a2 are actually the toString results of those list elements.
With errors.size shouldBe 1 fails, you will get an error like 2 was not 1. I much prefer getting the detailed message from my assertion. I would strongly recommend that you read up on scalatest. The assertions are carefully designed to give us lots of context when assertions fail. But if you go off and use the simplest assertions then you get very little info when things fail.
| case IllegalUriException(info) => info | ||
| case NonFatal(e) => ErrorInfo.fromCompoundString(e.getMessage) | ||
| case IllegalUriException(info) => info | ||
| case NonFatal(e) if e.getMessage == null => ErrorInfo.fromCompoundString(e.toString) |
There was a problem hiding this comment.
e.getMessage eq null ? I don't think it is good. eq is used to replace the equals method, and when compared to null, it is usually to use ==
|
Thanks @laglangyue - could you create a new PR that cherry picks this commit to the 1.0.x branch? |
* fix parse broken when header value is null * fix style * revert not need change * improve match case * revert HttpHeader and update HeaderParser.failure * update assert --------- Co-authored-by: tangjiafu <[email protected]>
* fix parse broken when header value is null * fix style * revert not need change * improve match case * revert HttpHeader and update HeaderParser.failure * update assert --------- Co-authored-by: Laglangyue <[email protected]> Co-authored-by: tangjiafu <[email protected]>
motivation
fix #574
test
by ci