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

Adding a check for existing vary header before overwriting with CORS … #2023

Merged
merged 2 commits into from Aug 21, 2018

Conversation

@yasuba
Copy link
Contributor

@yasuba yasuba commented Aug 20, 2018

…vary header

fixes #2003

@@ -54,19 +55,28 @@ object CORS {
else
config.exposedHeaders.map(headerFromStrings("Access-Control-Expose-Headers", _))

def checkVaryHeader(response: Response[G]): Option[Header] = {

This comment has been minimized.

@jmcardon

jmcardon Aug 21, 2018
Member

I think this function can be Response[G] => Response[G], saving you a fold down the line

If you look down below:

checkVaryHeader(withMethodBasedHeader)
          .fold(withMethodBasedHeader)(h => withMethodBasedHeader.putHeaders(h))

you could simplify this into:

def varyHeader(response: Response[G]): Response[G] =  {
  // Match avoids a fold higher order call, but you can just do this with fold if you don't want
  response.headers.get(CaseInsensitiveString("Vary")) match {
    case None => 
      response.putHeaders(Header("Vary", "Origin,Access-Control-Request-Method"))
     case _ => 
       response
  }
}

and then, down the line:

varyHeader(withMethodBasedHeader)
  .putHeaders(..
Copy link
Member

@rossabaker rossabaker left a comment

This looks good to me. I'll leave it open for a short while in case @yasuba wants to polish it a bit more, but I think the logic is correct.

def checkVaryHeader(response: Response[G]): Option[Header] = {
val maybeHeader = response.headers.get(CaseInsensitiveString("Vary"))
if (maybeHeader.isEmpty) Option(Header("Vary", "Origin,Access-Control-Request-Method"))
else maybeHeader

This comment has been minimized.

@rossabaker

rossabaker Aug 21, 2018
Member

This could be maybeHeader.getOrElse(Header(...)). But I like @jmcardon's idea.

This comment has been minimized.

@yasuba

yasuba Aug 21, 2018
Author Contributor

I'd like to polish the PR a bit - thanks for the comments

@@ -54,19 +55,28 @@ object CORS {
else
config.exposedHeaders.map(headerFromStrings("Access-Control-Expose-Headers", _))

def checkVaryHeader(response: Response[G]): Option[Header] = {
val maybeHeader = response.headers.get(CaseInsensitiveString("Vary"))
if (maybeHeader.isEmpty) Option(Header("Vary", "Origin,Access-Control-Request-Method"))

This comment has been minimized.

@rossabaker

rossabaker Aug 21, 2018
Member

Microoptimization: this default header could be stored in a val and shared by all requests.

This comment has been minimized.

@jmcardon

jmcardon Aug 21, 2018
Member

A microoptimization suggested by Ross himself?!

Are my repeated pedantic comments on microoptimizations finally making a dent?

Copy link
Member

@rossabaker rossabaker left a comment

👍

@jmcardon jmcardon merged commit 86995dc into http4s:master Aug 21, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jmcardon
Copy link
Member

@jmcardon jmcardon commented Aug 21, 2018

Thanks for the contribution @yasuba !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.