search

jazzband / django-waffle

A feature flipper for Django
https://waffle.readthedocs.io
BSD 3-Clause "New" or "Revised" License
1.13k stars 259 forks source link

Fix caching of auto-created missing flags #383

Closed simonlindholm closed 3 years ago

simonlindholm commented 4 years ago

Embarrassing follow-up to #379. It turns out the caching didn't actually work in some circumstances, because of the use of put() instead of set() -- the former does nothing when the key already exists. The test added in that PR passes for complicated reasons: get_or_create calls save() which is hooked to flush caches on transaction commit, and due to how transactions are set up in the test runner this occurs just in time for the call to put().

In a test suite at work, the flush doesn't happen before put(), so put() does nothing and we get one DB query per "flag_is_active" check. I don't know Django well enough to know how the two scenarios differ exactly. Django's "run tests in rolled-back transactions" behavior is probably involved somehow. I wouldn't be surprised if the buggy behavior is test-only.

Anyway, this makes the code more obviously correct and helps with the test suite at work. I don't really know how to test this...

simonlindholm commented 4 years ago

Perhaps it should actually be if created: cache.set(...)

fc commented 4 years ago

When I set a switch, it is randomly using the previous setting. Could the issue described here be the reason why? The issue happened in 1.0 then I upgraded to 2.0 but the issue still appears.

fc commented 4 years ago

I also just tested using a flag too and it will randomly be either on or off.