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

[#752] Support discarding cookies on a particular path, domain and ensure Session/Flash cookies use this #469

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@jroper
Member

jroper commented Sep 28, 2012

This is a backwards incompatible change, because it means discardingCookies now accepts a list of DiscardingCookie, which gives the developer the ability to discard cookies that were set on a particular domain, path or with secure set.

It fixes a bug in that when application.context, session.domain or session.secure was set, it was impossible to clear the session, and flash cookies were completely screwed.

Also in this request I changed some vals to defs in CookieBaker, Session and Flash objects, so that I could test with different configurations.

[#752] Support discarding cookies on a particular path, domain and wi…
…th secure set, and ensure Session/Flash cookies use this as appropriate
@betehess

This comment has been minimized.

Contributor

betehess commented Sep 28, 2012

Nice!

@@ -337,42 +337,42 @@ package play.api.mvc {
/**
* The cookie name.
*/
val COOKIE_NAME: String

This comment has been minimized.

@pk11

pk11 Oct 25, 2012

Collaborator

Why do we need to change these to def-s?

This comment has been minimized.

@jroper

jroper Oct 25, 2012

Member

I had to change some of them to def in order to test them... they were implemented as lazy val underneath, which meant I couldn't have one test testing with a domain or application context set on the session, and another without, without restarting the JVM. Changing to val solves this. It is a performance hit, though I don't believe it's a big one, and it's one that can be solved in other ways (we might decide to implement some sort of parsed config cache in application to deal with this, for example). And I think the tests here are really important. So on the trait I changed them all to def for consistency, and then in the implementation had some as vals and some as defs.

This comment has been minimized.

@pk11

pk11 Oct 26, 2012

Collaborator

I see, I remember we discussed this at some point. I suppose it's OK then. We can always change it back if it turns out to be too expensive (but as you said it looks like it should not be that bad)

@@ -648,7 +662,7 @@ package play.api.mvc {
* @param discard discard these cookies as well
* @return a valid Set-Cookie header value
*/
def encode(cookies: Seq[Cookie], discard: Seq[String] = Nil): String = {

This comment has been minimized.

@pk11

pk11 Oct 25, 2012

Collaborator

could we somehow avoid breaking compatibility?

This comment has been minimized.

@jroper

jroper Oct 25, 2012

Member

I don't think this is a method that we need to worry about, it's more of an implementation detail here. The methods that do matter are the Result.discardingCookies ones. If you'd like, I could make these methods overloaded, so they support both discarding by name, and with the entire cookie, but if I did that I think I'd still like to mark it @deprecated, since it's a bug waiting to happen.

This comment has been minimized.

@pk11

pk11 Oct 26, 2012

Collaborator

creating overloaded methods plus deprecating old API would be perfect. thanks

This comment has been minimized.

@jroper

jroper Oct 29, 2012

Member

Ok, I found out why I didn't do this in the first place, discardingCookies(names: String_) has the same method signature as discardingCookies(cookies: DiscardingCookie_) after type erasure since Scala uses Seqs and not Arrays for varargs methods, and so it won't compile. So we either come up with a new method name, or we just change the signature. I couldn't think of a better name, so I changed the signature.

@guillaumebort

This comment has been minimized.

Collaborator

guillaumebort commented Oct 26, 2012

Ok for me.

@@ -152,7 +152,8 @@ object PlayBuild extends Build {
publishArtifact in (Compile, packageSrc) := true,
resolvers += typesafe,
sourceGenerators in Compile <+= (dependencyClasspath in TemplatesCompilerProject in Runtime, packageBin in TemplatesCompilerProject in Compile, scalaSource in Compile, sourceManaged in Compile, streams) map ScalaTemplates,
compile in (Compile) <<= PostCompile
compile in (Compile) <<= PostCompile,
parallelExecution in Test := false

This comment has been minimized.

@mgibowski

mgibowski Oct 26, 2012

If it's only about ResultsSpec.scala why not adding sequential in that one spec, as described here: http://stackoverflow.com/questions/8026866/parallel-execution-of-tests?answertab=active#tab-top

This comment has been minimized.

@jroper

jroper Oct 27, 2012

Member

Because it's not about that one spec, adding sequential only makes that one spec run sequentially, but the issue is that you can't have two specs running an application at the same time due to Play's global state, and there is another spec in this project that runs an application, so unless we combine all the specs that run an application into one massive spec (and in future, as we increase Plays test coverage, there will be many, many more specs that run an application), sequential is of no use here.

@jroper

This comment has been minimized.

Member

jroper commented Oct 29, 2012

Cherry-picked in 51e6df3.

@jroper jroper closed this Oct 29, 2012

@mandubian

This comment has been minimized.

Collaborator

mandubian commented Oct 29, 2012

Did you try with 2 overloaded fcts?

discardingCookies(cookie: DiscardingCookie)
discardingCookies(cookie: DiscardingCookie, cookies: DiscardingCookie*)

or there is also the dumb DummyImplicit:
discardingCookies(cookie: DiscardingCookie*)(implicit dummy: DummyImplicit)

regards
Pascal

On Mon, Oct 29, 2012 at 1:23 AM, James Roper notifications@github.comwrote:

Cherry-picked in 51e6df351e6df3
.


Reply to this email directly or view it on GitHubhttps://github.com//pull/469#issuecomment-9853425.

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