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

Break up Cookie into Request and Response #1676

Merged
merged 6 commits into from Mar 4, 2018

Conversation

@kevinmeredith
Copy link
Contributor

@kevinmeredith kevinmeredith commented Feb 25, 2018

This PR adds a RequestCookie and ResponseCookie per the discussion in https://gitter.im/http4s/http4s?at=5a90d82b0202dc012e79241a.

I opened this PR due to the following motivation:

The only reason I ask is that, at work, I, having not used Cookie's in a real app, wrongly wrote code that checked on an HTTP Request's Cookie's cookie.isSecure && cookie.isHttpOnly. If an HTTP Request's Cookie only contained name=value pairs, then that would eliminate that type of error

per https://gitter.im/http4s/http4s?at=5a90d82b0202dc012e79241a.

It uses the abstract class approach that @jmcardon mentioned in https://gitter.im/http4s/http4s?at=5a90d8cd0202dc012e79266d.

@kevinmeredith kevinmeredith force-pushed the separate-req-vs-resp-cookie branch from 50e4aec to 819f455 Feb 25, 2018
@kevinmeredith kevinmeredith changed the title Break up Cookie into Request and Response WIP - Break up Cookie into Request and Response Feb 25, 2018
Copy link
Member

@rossabaker rossabaker left a comment

Looks like a good start.

Loading

final case class Cookie(
name: String,
content: String,
sealed abstract class Cookie(val name: String, val content: String) extends Renderable
Copy link
Member

@rossabaker rossabaker Feb 26, 2018

Choose a reason for hiding this comment

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

I think the file should still be called Cookie.scala.

Loading

Copy link
Member

@jmcardon jmcardon Feb 26, 2018

Choose a reason for hiding this comment

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

I'm personally more partial to:

sealed trait Cookie extends Renderable{
   def name: String
   def content: String
   ...
}

Mostly being convention I see a lot in the scala world, for what in this sense we're defining to be an interface specifically.

Loading

Copy link
Member

@rossabaker rossabaker Feb 26, 2018

Choose a reason for hiding this comment

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

Where in the API would we continue to abstract over Cookie? Do we even need a base at all?

Loading

Copy link
Contributor Author

@kevinmeredith kevinmeredith Feb 27, 2018

Choose a reason for hiding this comment

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

I echo the questioning of the need to actually abstract over Cookie. Also, per https://www.reddit.com/r/scala/comments/3887az/everything_you_ever_wanted_to_know_about_sealed/crtvpm5/, it seems wrong to me, albeit allowed in Scala, to use an ADT's sub-type in a signature.

Loading

Copy link
Contributor Author

@kevinmeredith kevinmeredith Feb 27, 2018

Choose a reason for hiding this comment

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

Please note that I respectfully question the need. I realize that I've made no commits to this code-base. I look forward to learning from maintainers and authors, yielding to them on this PR!

Loading

Copy link
Member

@rossabaker rossabaker Feb 27, 2018

Choose a reason for hiding this comment

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

And I appreciate the help!

What I would do is see what doesn't compile if Cookie goes away. Definitely in the RequestCookieJar, which would be for RequestCookie. There might be a few more references in the tests. If everything is obviously related to a Request or a Response, fix it. If something serves in both roles, maybe then we have a reason for the base Cookie to remain. But I doubt that this case exists.

Loading

Copy link
Member

@jmcardon jmcardon Feb 27, 2018

Choose a reason for hiding this comment

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

To be fair @kevinmeredith, I agree with you, but I say merely for the sake of convention, approaching Cookie like an interface rather than an ADT which we will exhaustively match on, given this is scala, not haskell, wherein the subtypes themselves are types.

If this were Haskell, there would be no inheritance whatsoever, simply two record types, as we don't want to work ever in terms of Cookie.

IMHO we should get rid of inheriting from a cookie trait altogether, given that whether it's an abstract class or a trait, we can still exhaustively pattern match on Cookie like if it was an ADT if we are ever presented with it, so it's of no real difference to use an abstract class over a trait in this case.

I'm just being a pedant maybe, but if we're going for this, I'm thinking we should stop people from creating signatures in terms of Cookie in the first place.

Loading

@@ -63,7 +63,7 @@ class RequestCookieJar(private val headers: Seq[Cookie])
def get(key: String): Option[Cookie] = headers.find(_.name == key)
def apply(key: String): Cookie = get(key).getOrElse(default(key))
def contains(key: String): Boolean = headers.exists(_.name == key)
def getOrElse(key: String, default: => String): Cookie = get(key).getOrElse(Cookie(key, default))
def getOrElse(key: String, default: => String): Cookie = get(key).getOrElse(RequestCookie(key, default))
Copy link
Member

@rossabaker rossabaker Feb 26, 2018

Choose a reason for hiding this comment

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

Doesn't this whole Cookie jar pertain to RequestCookies?

This cookie jar is full of unsafe methods, isn't particularly useful to a client maintaining an active conversation, etc. I think it's a candidate for deprecation and/or serious improvement. In any case, I would also move it to a RequestCookieJar file.

Loading

Copy link
Member

@jmcardon jmcardon left a comment

Good work so far.

Loading

final case class Cookie(
name: String,
content: String,
sealed abstract class Cookie(val name: String, val content: String) extends Renderable
Copy link
Member

@jmcardon jmcardon Feb 26, 2018

Choose a reason for hiding this comment

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

I'm personally more partial to:

sealed trait Cookie extends Renderable{
   def name: String
   def content: String
   ...
}

Mostly being convention I see a lot in the scala world, for what in this sense we're defining to be an interface specifically.

Loading

@kevinmeredith kevinmeredith force-pushed the separate-req-vs-resp-cookie branch from 819f455 to 6415819 Mar 2, 2018
@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Mar 2, 2018

Please excuse the git push -f. I almost always avoid re-basing on a PR with comments.

Anyway, I was able to run sbt "; project tests; test successfully on this branch.

Loading

@kevinmeredith kevinmeredith changed the title WIP - Break up Cookie into Request and Response Break up Cookie into Request and Response Mar 2, 2018
@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Mar 2, 2018

Putting back the WIP since I see sbt ci had a few failures. Sorry for the premature removal of WIP!

Loading

@kevinmeredith kevinmeredith changed the title Break up Cookie into Request and Response WIP - Break up Cookie into Request and Response Mar 2, 2018
aeons
aeons approved these changes Mar 2, 2018
Copy link
Member

@aeons aeons left a comment

Looks good!

Loading

@kevinmeredith kevinmeredith changed the title WIP - Break up Cookie into Request and Response Break up Cookie into Request and Response Mar 2, 2018
@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Mar 2, 2018

Could you please take a look, @rossabaker or @jmcardon?

Jose - I know that I didn't use the trait Cookie approach. Please let me you know if you disagree with the PR. Thanks!

Loading

Copy link
Member

@jmcardon jmcardon left a comment

Looks great to me!

In regards to the trait Cookie approach, I explicitly noted in a comment I would rather have no superclass to both cookies. You did that and I'm happy.

👍

Thank you very much for your contribution in the project 😄

Loading

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Mar 2, 2018

Thank you very much for your contribution in the project

Thank you and the maintainers for this excellent library!

Loading

Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

👍 LGTM

Loading

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Mar 3, 2018

Please let me know if you're waiting on me to do anything more before merging! Since this is a breaking change, would you expect it to be part of v19 or 20?

Thanks!

Loading

@jmcardon jmcardon merged commit d023fcf into http4s:master Mar 4, 2018
2 checks passed
Loading
@jmcardon
Copy link
Member

@jmcardon jmcardon commented Mar 4, 2018

@kevinmeredith nope! All good, thank you!

Just waiting on someone to hit the green button is all. You got three 👍 from maintainers, so it's all good.

Loading

@kevinmeredith kevinmeredith deleted the separate-req-vs-resp-cookie branch Mar 4, 2018
@rossabaker rossabaker mentioned this pull request Apr 20, 2018
31 tasks
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

5 participants