Skip to content

Commit

Permalink
feat: return feature from isEnabled to avoid another lookup
Browse files Browse the repository at this point in the history
Previously GetVariant would do 2 storage.Get calls which might be costly
depending on the implementation. It doesn't need to do the second one if we
just return the feature from isEnabled.

Fixes #161
  • Loading branch information
jameshartig committed Dec 13, 2023
1 parent c42351c commit 0a64a52
Showing 1 changed file with 16 additions and 22 deletions.
38 changes: 16 additions & 22 deletions client.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,12 +245,13 @@ func (uc *Client) IsEnabled(feature string, options ...FeatureOption) (enabled b
uc.metrics.count(feature, enabled)
}()

return uc.isEnabled(feature, options...).Enabled
result, _ := uc.isEnabled(feature, options...)
return result.Enabled
}

// isEnabled abstracts away the details of checking if a toggle is turned on or off
// without metrics
func (uc *Client) isEnabled(feature string, options ...FeatureOption) api.StrategyResult {
func (uc *Client) isEnabled(feature string, options ...FeatureOption) (api.StrategyResult, *api.Feature) {
var opts featureOption
for _, o := range options {
o(&opts)
Expand All @@ -264,7 +265,7 @@ func (uc *Client) isEnabled(feature string, options ...FeatureOption) api.Strate
}

if f == nil {
return handleFallback(opts, feature, ctx)
return handleFallback(opts, feature, ctx), nil
}

if f.Dependencies != nil && len(*f.Dependencies) > 0 {
Expand All @@ -273,20 +274,20 @@ func (uc *Client) isEnabled(feature string, options ...FeatureOption) api.Strate
if !dependenciesSatisfied {
return api.StrategyResult{
Enabled: false,
}
}, f
}
}

if !f.Enabled {
return api.StrategyResult{
Enabled: false,
}
}, f
}

if len(f.Strategies) == 0 {
return api.StrategyResult{
Enabled: f.Enabled,
}
}, f
}

for _, s := range f.Strategies {
Expand All @@ -302,7 +303,7 @@ func (uc *Client) isEnabled(feature string, options ...FeatureOption) api.Strate
uc.errors <- err
return api.StrategyResult{
Enabled: false,
}
}, f
}

allConstraints := make([]api.Constraint, 0)
Expand All @@ -318,7 +319,7 @@ func (uc *Client) isEnabled(feature string, options ...FeatureOption) api.Strate
if !ok {
return api.StrategyResult{
Enabled: false,
}
}, f
}

return api.StrategyResult{
Expand All @@ -327,18 +328,18 @@ func (uc *Client) isEnabled(feature string, options ...FeatureOption) api.Strate
GroupId: groupId,
Variants: s.Variants,
}.GetVariant(ctx),
}
}, f
} else {
return api.StrategyResult{
Enabled: true,
}
}, f
}
}
}

return api.StrategyResult{
Enabled: false,
}
}, f
}

func (uc *Client) isParentDependencySatisfied(feature *api.Feature, context context.Context) bool {
Expand All @@ -356,7 +357,7 @@ func (uc *Client) isParentDependencySatisfied(feature *api.Feature, context cont
return false
}

enabledResult := uc.isEnabled(parent.Feature, WithContext(context))
enabledResult, _ := uc.isEnabled(parent.Feature, WithContext(context))
// According to the schema, if the enabled property is absent we assume it's true.
if parent.Enabled == nil || *parent.Enabled {
if parent.Variants != nil && len(*parent.Variants) > 0 && enabledResult.Variant != nil {
Expand Down Expand Up @@ -400,24 +401,17 @@ func (uc *Client) getVariantWithoutMetrics(feature string, options ...VariantOpt
}

var strategyResult api.StrategyResult

var f *api.Feature
if opts.resolver != nil {
strategyResult = uc.isEnabled(feature, WithContext(*ctx), WithResolver(opts.resolver))
strategyResult, f = uc.isEnabled(feature, WithContext(*ctx), WithResolver(opts.resolver))
} else {
strategyResult = uc.isEnabled(feature, WithContext(*ctx))
strategyResult, f = uc.isEnabled(feature, WithContext(*ctx))
}

if !strategyResult.Enabled {
return defaultVariant
}

var f *api.Feature
if opts.resolver != nil {
f = opts.resolver(feature)
} else {
f = uc.repository.getToggle(feature)
}

if f == nil {
if opts.variantFallbackFunc != nil {
return opts.variantFallbackFunc(feature, ctx)
Expand Down

0 comments on commit 0a64a52

Please sign in to comment.