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

Fix for withForm bug #51

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

exell-christopher
Copy link

No description provided.

christopher.exell added 2 commits April 3, 2015 19:20
… session has been changed, and a method on the wrapped response that triggers a cookie write is called.
@benlucchesi
Copy link
Owner

Hi Christopher,

Thanks for the fix. At first blush, I don't see a problem with the code, but I am concerned about the number of times the session gets written and when it can be written. If the session is written an excessive number of times, it impacts performance significantly. Also, if it gets written more than once into the response, then it runs the risk of changing size and overwriting other data in the response.

Can you do me a solid and checkout the repo: https://github.com/benlucchesi/test-cookie-session-plugin
There are two branches of interest: springsecurity2.0 and springsecurity1.0. This project is a test suite for the cookie session plugin and these branches test it with two version of spring security.

To launch the test suite use: grails test-app --https :spock

This will launch integration and functional tests. The functional tests use the firefox selenium drive so you'll need firefox installed.

You may need to add or configure logging to detect how and when the session gets written. There isn't a test specifically for this condition, but if there is a problem, hopefully something breaks.

thanks,
-ben

@@ -75,8 +69,10 @@ public void saveSession(){
return;
}

// flag the session as saved.
sessionSaved = true;
if(session.getIsDirty() == false ){

Choose a reason for hiding this comment

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

if (!session.getIsDirty()) {

@exell-christopher
Copy link
Author

Haven't had a chance to dig into testing this against the test harnesses. If someone has time, and wants to give it a shot it would be appreciated.

@benlucchesi
Copy link
Owner

I'll check it out soon.
On May 16, 2015 9:24 AM, "exell-christopher" [email protected]
wrote:

Haven't had a chance to dig into testing this against the test harnesses.
If someone has time, and wants to give it a shot it would be appreciated.


Reply to this email directly or view it on GitHub
#51 (comment)
.

@double16
Copy link

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

Successfully merging this pull request may close these issues.

4 participants