feat: Add support for fallback-action #93
Conversation
dennis-menge
changed the title
| @@ -15,6 +15,10 @@ default boolean isEnabled(String toggleName, UnleashContext context, boolean def | |||
| return isEnabled(toggleName, defaultSetting); | |||
| } | |||
|
|
|||
| boolean isEnabled(final String toggleName, boolean defaultSetting, final FallbackAction fallbackAction); | |||
There was a problem hiding this comment.
Instead of defining a new interface we could actually use one of the functional interfaces: BiFunction<String, UnleashContext, Boolean> instead.
There was a problem hiding this comment.
Oh yeah, completely forgot about these, I will change that accordingly.
| fallbackAction.apply(toggleName, contextProvider.getContext()); | ||
| } | ||
|
|
||
| return isEnabled(featureToggle, contextProvider.getContext(), defaultSetting); |
There was a problem hiding this comment.
I would rather invert it and have this function being the master and have the other isEnabled calls send in a proper fallback-action.
…methods according to feedback, added tests
| return isEnabled(toggleName, contextProvider.getContext(), defaultSetting, fallbackAction); | ||
| } | ||
|
|
||
| public boolean isEnabled(final String toggleName, UnleashContext context, boolean defaultSetting, final BiFunction<String, UnleashContext, Boolean> fallbackAction) { | ||
| FeatureToggle featureToggle = toggleRepository.getToggle(toggleName); | ||
| boolean enabled = isEnabled(featureToggle, context, defaultSetting); |
There was a problem hiding this comment.
I believe it would be better if we modify the private isEnabled-method (line 99) to take FallbackAction as last input and have all public isEnabled-methods call this one. There are some reasons for this:
- Avoid duplication of logic.
- ensure that FallbackAction gets the enhanced context injected.
This change would require existing isEnabled-methods to also send in a fallback function, this can be achieved via a lambda that just returns the fallbackValue injected to the method (or false)
| return isEnabled(toggleName, false, fallbackAction); | ||
| } | ||
|
|
||
| boolean isEnabled(final String toggleName, boolean defaultSetting, final BiFunction<String, UnleashContext, Boolean> fallbackAction); |
There was a problem hiding this comment.
By not providing fallback for new interface methods we will break existing implementations of the interface. This is probably ok, but needs to be at least a minor.
There was a problem hiding this comment.
Added a default implementation like for the other methods
…h implementation, added default implementation in Unleash interface to avoid breaking existing implementation
|
This is really closing in to something good now 🚀. I have limited time today, but hope to be able to do a proper validation tomorrow! Feel free to also join us on slack for more in depth discussions: |
| return isEnabled(toggleName, false, fallbackAction); | ||
| } | ||
|
|
||
| default boolean isEnabled(String toggleName, UnleashContext context, boolean defaultSetting, BiFunction<String, UnleashContext, Boolean> fallbackAction) { |
There was a problem hiding this comment.
This one is not needed. It does not make sense to allow both defaultSetting and a fallbackAction.
Should not be possible to define both a fallback action and a defaultSetting at the same time.
|
Hi again @dennis-menge. I did a few adjustment to this PR. Could you look it over and see if iy make sense? |
Use mocked functions and verify that the fallBack action is actually called with correct arguments.
Hi Ivar, looks nice, I misunderstood the whole time that defaultSetting and fallbackAction should be provided at the same time with fallbackAction taking precedence, which obviously makes no sense. Thank you! |
dennis-menge
changed the title
ivarconr
changed the title
* closes #89: to Added first implementation (draft) of fallbackAction * Migrated to BiFunction for definition of fallback action, refactored methods according to feedback, added tests * changed one test case to reflect correct implementation * Added default implementation for isEnabled(String, BiFunction) * Incorporated feedback, changed logical method layout in DefaultUnleash implementation, added default implementation in Unleash interface to avoid breaking existing implementation * fix: fallbackAction and defaultSetting Should not be possible to define both a fallback action and a defaultSetting at the same time. * fix: Improve fallbackAction unit-tests Use mocked functions and verify that the fallBack action is actually called with correct arguments. * fix: remove final arguments in interface * fix: simplify FakeUnleash for fallbackAction * fix: formatting * fix: counting at right place
Today it is possible to define a fallback value which the
isEnabledwill return if the feature toggle is not defined.We want to also allow the user to send in a fallbackFunction instead of a fallback value (which will take precedence over fallback value).
The fallback function should get to arguments injected:
toggleNameand the enhancedunleashContext. The function is implemented by the caller and should return a boolean value.Example on how it would work:
isEnabled("someToggle", context, (toggleName, eContext) -> false)