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

Session store #86

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

Session store #86

wants to merge 3 commits into from

Conversation

marsh8
Copy link

@marsh8 marsh8 commented Jul 7, 2015

CAPTCHA_STORE setting added.

CAPTCHA_STORE = 'DB' (default). Captcha info stored in CaptchaStore model (same as before).
CAPTCHA_STORE = 'SESSION'. Captcha info stored in SessionStore (sessions should be enabled).

Known issue: django will made new migration after changing CAPTCHA_STORE setting and running makemigrations.

upd: django 1.4 compatible.

@mbi
Copy link
Owner

mbi commented Jul 7, 2015

Hi, thank you, this looks interesting! What are the advantages of storing the captcha in the session instead of the database? Performance?

Also, would you consider including a couple tests, covering the bits affected by the patch?

Cheers.

@marsh8
Copy link
Author

marsh8 commented Jul 7, 2015

Hi, and thanks for useful captcha app.

It should have better performance with some sessions settings (Memcached sessions for example).

I'll write additional tests after passing all of Travis' checks.

@marsh8 marsh8 force-pushed the SessionStore branch 2 times, most recently from b1bf212 to 2682e36 Compare July 9, 2015 09:04
@marsh8
Copy link
Author

marsh8 commented Jul 9, 2015

upd: minor fixes and few tests.
known issue: django 1.4 don't have SessionStore.clear_expired() function. store.remove_expired() will be skipped for django 1.4.

It would be nice if you could add this code to the installation package.

@mbi
Copy link
Owner

mbi commented Jul 19, 2015

Hi, just an update on this: I'm working on this pull request, but it's more complicated than it seems. Basically what I'd like to do is set CAPTCHA_STORE = 'SESSION' in the testproject settings and make sure the tests still pass using this backend. They currently do not, but not necessarily because your code isn't working.

@marsh8
Copy link
Author

marsh8 commented Jul 22, 2015

Hi.
I tried that too, but it didn't go so well. Storages broke some test (differences in structure, SessionStore can't use objects.get(...), additional characters in hashkey etc). It's either rewriting code or making new tests.
I took two previous tests, made some minor changes in code and switched storage from 'DB' to 'SESSION' in these tests on a fly (it's testFormSubmit and testWrongSubmit in StoresCase class).
In the end it was test that checks both 'DB' and 'SESSION' storages in a single run without changing settings of test project.
It worked better than changing settings for whole test project.

@ealogar
Copy link

ealogar commented Feb 26, 2016

Are you going to aprove this PR? The feature seems interesting to have...

@mbi
Copy link
Owner

mbi commented Feb 26, 2016

@ealogar not in its current state, sorry.

@ealogar
Copy link

ealogar commented Feb 26, 2016

@mbi Thanks for your response,
do you see interesting to add a setting to override the CaptchaStore model and provide a custom one?
CAPTCHA_STORAGE_MODEL = getattr...
In this way people would be able to change django orm to a storage in mongodb (as my case). If you see interesting I would launch a pr...

@mbi
Copy link
Owner

mbi commented Feb 26, 2016

@ealogar sure, granted it comes with tests and everything that currently relies on the Django ORM still works :)

@ealogar
Copy link

ealogar commented Feb 26, 2016

@mbi I am working on it. I will add a test with an in-memory storage dao. Up to people the dao they provide, just fine if they follow The BaseCaptchaDao duck-typing

@vstoykov
Copy link
Contributor

I see that there is another implementation (but using cache instead) in #103.

Probably something combined from both will be very good.

@9mido
Copy link

9mido commented May 2, 2020

Any update on this or on #103 ? I would very much prefer to use anything other than the database to store the captchas temporarily or better yet redesign the app to not even need to store the captchas at all somehow? Storing it in cache might also be costly to the hardware especially if there is not a caching invalidation strategy.

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.

5 participants