Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Address Uri#fromString Defect for Parsing the Port #2687

Merged
merged 7 commits into from
Jul 5, 2019
Merged

Address Uri#fromString Defect for Parsing the Port #2687

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ad2f1da
Fail to parse 'Port' if 'number' isn't an 'Int'.
kevinmeredith Jul 1, 2019
f425ad3
Replace 'parse port' example spec with property-based one.
kevinmeredith Jul 1, 2019
6e9f135
Add specs for failing to parse port.
kevinmeredith Jul 1, 2019
283cae5
Run scalafmt.
kevinmeredith Jul 1, 2019
66252fa
Update spec to suceed for ':' followed by empty String.
kevinmeredith Jul 1, 2019
e997a9e
Narrow implicits to avoid 2.11.12 compile-time error.
Jul 3, 2019
8664ed7
Run scalafmt.
Jul 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion core/src/main/scala/org/http4s/parser/Rfc3986Parser.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,9 @@ private[parser] trait Rfc3986Parser

def Port = rule {
":" ~ (capture(oneOrMore(Digit)) ~> { s: String =>
(Some(s.toInt))
val int: Option[Int] = Either.catchOnly[NumberFormatException](s.toInt).toOption
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to use your suggestion, #2664 (comment), @SystemFw. Thanks for it!


test(int.nonEmpty) ~ push(int)
} | push(None)) | push(None)
}

Expand Down
48 changes: 44 additions & 4 deletions tests/src/test/scala/org/http4s/UriSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,12 @@ class UriSpec extends Http4sSpec with MustThrownMatchers {
}

"parse port correctly" >> {
"if there is one" in {
val uri = getUri("http://localhost:8080/")
uri.port must_=== Some(8080)
}
"if there is a valid (non-negative) one" >> prop { nonNegative: Int =>
{
val uri = getUri(s"http://localhost:$nonNegative/")
uri.port must_=== Some(nonNegative)
}
}.setGen(Gen.choose[Int](0, Int.MaxValue))
"if there is none" in {
val uri = getUri("http://localhost/")
uri.port must_=== None
Expand All @@ -84,6 +86,44 @@ http://example.org/a file
}
}

"fail to parse port" >> {
"if it's negative" >> prop { negative: Int =>
{
val uri: ParseResult[Uri] = Uri.fromString(s"http://localhost:$negative/")
uri match {
case Left(ParseFailure("Invalid URI", _)) => ok
case unexpected => ko(unexpected.toString)
}
}
}.setGen(Gen.choose[Int](Int.MinValue, -1))
"if it's larger than Int.MaxValue" >> prop { tooBig: Long =>
{
val uri: ParseResult[Uri] = Uri.fromString(s"http://localhost:$tooBig/")
uri match {
case Left(ParseFailure("Invalid URI", _)) => ok
case unexpected => ko(unexpected.toString)
}
}
}.setGen(Gen.choose[Long]((Int.MaxValue: Long) + 1, Long.MaxValue))
"if it's not a number or an empty String" >> prop { notNumber: String =>
{
val uri: ParseResult[Uri] = Uri.fromString(s"http://localhost:$notNumber/")
uri match {
case Left(ParseFailure("Invalid URI", _)) => ok
case unexpected => ko(unexpected.toString)
}
}
}.setGen(Gen.alphaNumStr.suchThat(str =>
str.nonEmpty && Either.catchOnly[NumberFormatException](str.toInt).isLeft))
"for an empty String" in {
Copy link
Contributor Author

@kevinmeredith kevinmeredith Jul 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Referencing #2664 (comment), locallyy, this test failed:

[info]   fail to parse port
[info]     + if it's negative (214 ms)
[info]     + if it's larger than Int.MaxValue (230 ms)
[info]     + if it's not a number or an empty String (256 ms)
[error]     x for an empty String (271 ms)
[error]      Right(http://localhost/) (UriSpec.scala:121)
[error] org.http4s.UriSpec.$anonfun$new$51(UriSpec.scala:121)

@rossabaker.

Per https://tools.ietf.org/html/rfc3986#page-22,

3.2.3.  Port

   The port subcomponent of authority is designated by an optional port
   number in decimal following the host and delimited from it by a
   single colon (":") character.

      port        = *DIGIT

   A scheme may define a default port.  For example, the "http" scheme
   defines a default port of "80", corresponding to its reserved TCP
   port number.  The type of port designated by the port number (e.g.,
   TCP, UDP, SCTP) is defined by the URI scheme.  URI producers and
   normalizers should omit the port component and its ":" delimiter if
   port is empty or if its value would be the same as that of the
   scheme's default.

Per https://tools.ietf.org/html/rfc3986#page-22,

6.2.3.  Scheme-Based Normalization

   The syntax and semantics of URIs vary from scheme to scheme, as
   described by the defining specification for each scheme.
   Implementations may use scheme-specific rules, at further processing
   cost, to reduce the probability of false negatives.  For example,
   because the "http" scheme makes use of an authority component, has a
   default port of "80", and defines an empty path to be equivalent to
   "/", the following four URIs are equivalent:

      http://example.com
      http://example.com/
      http://example.com:/
      http://example.com:80/

this test, it seems clear to me, should fail, not pass. In other words, I should replace this "should fail" test with a "should parse to port 80" one.

Please let me know if you disagree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, That seems correct via the spec.

val uri: ParseResult[Uri] = Uri.fromString("http://localhost:/")
uri match {
case Left(ParseFailure("Invalid URI", _)) => ok
case unexpected => ko(unexpected.toString)
}
}
}

"support a '/' operator when original uri has trailing slash" in {
val uri = getUri("http://localhost:8080/")
val newUri = uri / "echo"
Expand Down