fix: Always check parent dependency if present #248
Conversation
| toggleName, | ||
| strategy.getName()); | ||
| } | ||
| if (featureToggle.getStrategies().isEmpty()) { |
There was a problem hiding this comment.
Should we do something like:
processedStrategies = featureToggle.getStrategies().map(this::getStrategy).filter(s -> s != UNKNOWN_STRATEGY);
if (processedStrategies.isEmpty()) {
...?
There was a problem hiding this comment.
Rethinking this is probably a good idea, but it doesn't relate to this PR, so I'd suggest leaving it for a separate sparring + PR session
| FeatureEvaluationResult result = | ||
| configuredStrategy.getResult( | ||
| strategy.getParameters(), | ||
| enhancedContext, | ||
| ConstraintMerger.mergeConstraints(featureRepository, strategy), | ||
| strategy.getVariants()); |
There was a problem hiding this comment.
I find it awkward that if we don't find strategies we return enabled=true but if we do find strategies but we can't map the strategy to a known strategy we (probably) return enabled=false at line 216
There was a problem hiding this comment.
I don't disagree, it's not because of this PR though.
Maybe a rethink here; as a separate thing. I'm open to some sparring
There was a problem hiding this comment.
I find it awkward that if we don't find strategies we return enabled=true but if we do find strategies but we can't map the strategy to a known strategy we (probably) return enabled=false at line 216
I think you have to take this up with v0.0.1 of the SDK spec. That's the correct behavior. An empty list of strategies is valid. A custom strategy that you defined server side but didn't implement client side is a broken and is expected to short circuit to false
There was a problem hiding this comment.
A custom strategy that you defined server side but didn't implement client side is a broken and is expected to short circuit to false
I am completely with @sighphyre here. If a strategy is not found, the safe and correct way is to return false for the toggle if the strategy is not found
| } | ||
| return new FeatureEvaluationResult(false, defaultVariant); |
There was a problem hiding this comment.
Something feels very subtly off here but it's not breaking tests sooo... it might be okay?
What's going through my head? Previously, we'd fall through to true, now we fall through to false. In theory, a toggle that has no strategies should fallback to the 'enabled' property, not true or false. That's distinct from a toggle that doesn't have strategies defined, which should never happen server side but when it does we turn off the toggle
So really we're into undefined territory here. It's probably okay but there's nothing in the client spec that enforces this behavior, so it's possible we break this again subtly
There was a problem hiding this comment.
But we've already checked the enabled property. If that evaluates to false, we short-circuit the entire evaluation. The one thing this does, that it didn't do previously, is the case where strategies was empty, now we have to check parent as well
There was a problem hiding this comment.
But, since we now have 3 people saying that we struggle with following the flow; I'm all for taking the time to make a new PR (Separate from this one) trying to clean up the logic, still making sure we pass all the tests, but so we don't need clarification talks every time we touch this.
Fixes: #246