Skip to content

Session store#86

Open
marsh8 wants to merge 3 commits into
mbi:masterfrom
marsh8:SessionStore
Open

Session store#86
marsh8 wants to merge 3 commits into
mbi:masterfrom
marsh8:SessionStore

Conversation

@marsh8
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

ealogar commented Feb 26, 2016

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

@mbi
Copy link
Copy Markdown
Owner

mbi commented Feb 26, 2016

@ealogar not in its current state, sorry.

@ealogar
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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.

@veto8
Copy link
Copy Markdown

veto8 commented Apr 9, 2025

Yes, I would also like this option, as I want to keep my relational database small and portable.

@veto8
Copy link
Copy Markdown

veto8 commented May 7, 2025

Hi, thank you, this looks interesting! What are the advantages of storing the captcha in the session instead of the database? Performance?
... for me, it's not to "contaminate" the sql database

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.

6 participants