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.

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

Address Uri#fromString Defect for Parsing the Port #2687

Merged
merged 7 commits into from Jul 5, 2019

Conversation

@kevinmeredith
Copy link
Contributor

@kevinmeredith kevinmeredith commented Jul 1, 2019

Closes #2583 by implementing one of @rossabaker's suggestions from #2583 (comment).

Per #2664 (comment), I decided to close #2664 in favor of this PR.

}
}.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

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.

Loading

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.

Loading

@@ -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

@kevinmeredith kevinmeredith Jul 1, 2019

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!

Loading

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Jul 2, 2019

I closed, and then re-opened, due to, what appears to be, a flaky test.

Loading

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Jul 3, 2019

I need to address:

[error] /home/travis/build/http4s/http4s/core/src/main/scala/org/http4s/parser/Rfc3986Parser.scala:85:69: type mismatch;
[error]  found   : Either[NumberFormatException,Int]
[error]  required: ?{def toOption: ?}
[error] Note that implicit conversions are not applicable because they are ambiguous:
[error]  both method EitherSyntax in package http4s of type [L, R](self: Either[L,R])org.http4s.package.EitherSyntax[L,R]
[error]  and method catsSyntaxEither in trait EitherSyntax of type [A, B](eab: Either[A,B])cats.syntax.EitherOps[A,B]
[error]  are possible conversion functions from Either[NumberFormatException,Int] to ?{def toOption: ?}
[error]       val int: Option[Int] = Either.catchOnly[NumberFormatException](s.toInt).toOption
[error]                                                                     ^
[error] one error found
[error] (core / Compile / compileIncremental) Compilation failed
[error] Total time: 67 s, completed Jul 2, 2019 2:27:51 PM

per latest build failure.

Loading

```scala
[error] /home/travis/build/http4s/http4s/core/src/main/scala/org/http4s/parser/Rfc3986Parser.scala:85:69: type mismatch;
[error]  found   : Either[NumberFormatException,Int]
[error]  required: ?{def toOption: ?}
[error] Note that implicit conversions are not applicable because they are ambiguous:
[error]  both method EitherSyntax in package http4s of type [L, R](self: Either[L,R])org.http4s.package.EitherSyntax[L,R]
[error]  and method catsSyntaxEither in trait EitherSyntax of type [A, B](eab: Either[A,B])cats.syntax.EitherOps[A,B]
[error]  are possible conversion functions from Either[NumberFormatException,Int] to ?{def toOption: ?}
[error]       val int: Option[Int] = Either.catchOnly[NumberFormatException](s.toInt).toOption
[error]                                                                     ^
[error] one error found
[error] (core / Compile / compileIncremental) Compilation failed
[error] Total time: 67 s, completed Jul 2, 2019 2:27:51 PM
```
"if there is none" in {
val uri = getUri("http://localhost/")
uri.port must_=== None
}
// See RFC 3986, section 6.2.3
"for an empty String" in {

Choose a reason for hiding this comment

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

Given https://foobar:/ should this infer 443 rather than 80? I worry as I believe some may rely on this behavior by accident and then they would go to the unsecured port by default.

Loading

Copy link
Member

@rossabaker rossabaker Jul 3, 2019

Choose a reason for hiding this comment

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

Are we inferring anywhere in the Uri? We do in the clients, but the Uri has an optional port. You're correct that the inference is scheme specific, but I think this test is right.

Loading

Copy link
Contributor Author

@kevinmeredith kevinmeredith Jul 5, 2019

Choose a reason for hiding this comment

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

Given https://foobar:/ should this infer 443 rather than 80?

Is it worthwhile to open another issue, @ChristopherDavenport, if https://foobar:/ resolves a port of 80 rather than 443? I'm only asking since, I believe, my PR addresses #2583.

Loading

Copy link
Contributor Author

@kevinmeredith kevinmeredith Jul 10, 2019

Choose a reason for hiding this comment

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

Hi @ChristopherDavenport -

Given https://foobar:/ should this infer 443 rather than 80?

So the following looks right or wrong to you? I admit to not knowing how the Port is used if there's a supplied Scheme, i.e. my first Uri#fromString REPL example.

@ import $ivy.`org.http4s::http4s-core:0.20.6` 
import $ivy.$                               

@ Uri.fromString("https://www.google.com:80") 
res5: org.http4s.package.ParseResult[Uri] = Right(Uri(Some(Scheme(https)), Some(Authority(None, RegName(www.google.com), Some(80))), "", , None))

@ Uri.fromString("https://www.google.com:/") 
res6: org.http4s.package.ParseResult[Uri] = Right(Uri(Some(Scheme(https)), Some(Authority(None, RegName(www.google.com), None)), "/", , None))

Does this look right, Chris?

Loading

Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

Yeah, it can be handled somewhere else if we think that is appropriate.

Loading

@niij
Copy link

@niij niij commented Jul 5, 2019

Thanks @kevinmeredith !

Loading

@rossabaker rossabaker merged commit c89c26f into http4s:series/0.20 Jul 5, 2019
1 check passed
Loading
@kevinmeredith kevinmeredith deleted the address-uri-bug-20 branch Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants