cfpb / django-flags

Feature flags for Django projects
https://cfpb.github.io/django-flags/
Creative Commons Zero v1.0 Universal
260 stars 32 forks source link

Fix a TypeError when a configured condition has no function registered #15

Closed willbarton closed 6 years ago

willbarton commented 6 years ago

This PR fixes a bug where a TypeError can be raised if a condition function is not registered for a condition name that a flag attempts to check.

Prior to this change the following would raise a TypeError:

condition = Condition('my_condition', 'value')
condition.check()

If there is not a corresponding condition.register('my_condition', fn=my_condition_function).

After this change, condition.check() will return None, which is Falsy.

coveralls commented 6 years ago

Pull Request Test Coverage Report for Build 222


Totals Coverage Status
Change from base Build 198: 0.0%
Covered Lines: 867
Relevant Lines: 867

💛 - Coveralls
chosak commented 6 years ago

This approach allows invalid Condition objects to be created and used as if they were valid; they act as if the condition fails (by returning None) even though no check is really happening. I can see this being confusing for someone debugging flag code and wondering why condition.fn is null.

What would you think about an alternate implementation where this line raises a ValueError (because the provided condition argument is invalid) and then this logic catches the error, maybe logs a warning, and skips adding it to the list of flag conditions? This would leave each flag with a true set of conditions, skipping and logging any that weren't valid.

willbarton commented 6 years ago

@chosak I believe off the top of my head that will prevent it from showing up in the UI in Wagtail-Flags, thus allowing flag states with orphaned conditions to continue to exist without any way for a user to remove them.

I do like the idea of logging it as a problem though.

willbarton commented 6 years ago

I think that returning None (like we do in flag_state) is an attempt to make the problem debuggable — since the condition doesn't return False it can be inspected if need be.

An alternative would be to raise a ValueError and maybe check for it in flag_state() but I'm not sure that's any different, and I'd kinda rather that Condition objects be the sole arbiters of conditions. None seems like a good compromise to return to indicate there was something amiss but not disrupt the whole chain of condition checks and maintain the list of conditions to contain all those that have been defined.