search

disqus / gargoyle

Feature switches in Django
http://engineering.disqus.com
Apache License 2.0
746 stars 112 forks source link

Adding an "exclude" condition causes undefined behaviour #105

Open robgolding opened 8 years ago

robgolding commented 8 years ago

This is a pretty serious bug introduced by https://github.com/disqus/gargoyle/pull/53.

Since this commit, the order of the conditions is now important, and since this library is backed by modeldict, that order can not be guaranteed. Say you have multiple conditions over multiple fields:

If the conditions come out in the order above, then the condition set is always true. If they come out in the opposite order, the condition set is only true if the IP address is 127.0.0.1.

What we're seeing is that the conditions are sometimes respected correctly (resulting in the expected behaviour), but the vast majority of the time the switch is just enabled. It's almost impossible to reproduce by running the code in a shell, because the order of a dictionary changes based on how the memory is laid out, and that's unique for each process.

Furthermore, I don't think the "default" should ever be that a switch is on. If a switch is in selective mode and has a single exclude condition, then that switch should not be enabled for anyone. You should have to selectively opt-in groups users to the switch, then opt-out individuals if necessary.

I don't expect there's any chance of the default behaviour being changed again, so I'll just update our fork to revert this change, but I wanted to raise this in case anyone comes across this crazy bug in the future!

jlward commented 8 years ago

I just ran into this updating from v0.10.8 (the version right before the bug)