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

add Host header to requests submitted by Client.fromHttpService if the request URI is absolute #1874

Merged
merged 1 commit into from May 25, 2018

Conversation

Projects
None yet
5 participants
@bpholt
Contributor

bpholt commented May 25, 2018

Addresses #1872 in the easy case where the request URI is absolute.

private def addHostHeaderIfUriIsAbsolute[F[_]](req: Request[F]): Request[F] = {
val maybeHost = for {
host <- req.uri.host
if req.headers.get(Host).isEmpty

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport May 25, 2018

Member

_ <- Alternative[Option].guard(req.headers.get(Host).isEmpty)

This comment has been minimized.

@bpholt

bpholt May 25, 2018

Contributor

@ChristopherDavenport I don't understand the difference here. Can you explain why Alternative[Option].guard is preferred?

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport May 25, 2018

Member

One is implemented in terms of flatMap the other in terms of withFilter. I just see this as utilizing non-functional language features which has the possibility for issues in the future.

I'm sure people may disagree here.

This comment has been minimized.

@jmcardon

jmcardon May 25, 2018

Member

No comment on cats fp guard syntax (I don't care for it but I don't hate it), however, instead of the withfilter guard, you can always just

req.uri.host match {
  case Some(host) if req.headers.get(Host).isEmpty => 
     req.withHeaders(req.headers.put((Host(host.value, req.uri.port)))
  case _ => req
}

That's microscopically more efficient (no fold call + no withFilter)

This comment has been minimized.

@rossabaker

rossabaker May 25, 2018

Member

What is the functional objection to withFilter on a type that has a reasonable empty? I agree that @jmcardon's version saves a few nanoseconds, and I agree that withFilter on Future is regrettable, but I don't have a problem with what @bpholt wrote.

@rossabaker

Did you also want to set the secure flag if the scheme is https? That was mentioned on Gitter, and sounded like a smart idea. That's another thing that a real client backend does for free.

@ChristopherDavenport

I'm not sure I have a solid objection to withFilter on option other than a lack of trust in withFilter. As such this is an improvement, but if we want the secure flag I agree with that change as well.

@bpholt bpholt force-pushed the Dwolla:issue-1872 branch from 8c14201 to 5708bf4 May 25, 2018

@bpholt

This comment has been minimized.

Contributor

bpholt commented May 25, 2018

Updated with @jmcardon's pattern match suggestion.

I'm going to leave the secure flag alone, as it's not easily changed for this usage.

@aeons aeons merged commit ff640fe into http4s:release-0.18.x May 25, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bpholt bpholt deleted the Dwolla:issue-1872 branch May 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment