Skip to content

Commit 3c1d00b

Browse files
suthar26toddbaert
suthar26
and
toddbaert
authored
feat!: DISABLED is a successful evaluation (still defaults) (#1968)
- Disabled flags now resolve successfully with `reason=DISABLED` instead of returning a `FLAG_DISABLED` error. See the [ADR](https://github.com/open-feature/flagd/blob/main/docs/architecture-decisions/disabled-flag-evaluation.md). - Breaking, but barely: resolved values are unchanged (SDKs still surface the caller-provided default). - The break is only observable for consumers that inspect `reason` / `errorCode`, call flagd directly over gRPC / OFREP, or import `core/pkg/model`. ### Related Issues Fixes #1966 Part of #1965 --------- Signed-off-by: Parth Suthar <parth.suthar@dynatrace.com> Signed-off-by: Todd Baert <todd.baert@dynatrace.com> Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
1 parent 1c7f7bc commit 3c1d00b

7 files changed

Lines changed: 78 additions & 63 deletions

File tree

‎core/pkg/evaluator/json.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -175,11 +175,6 @@ func (je *Resolver) ResolveAllValues(ctx context.Context, reqID string, context
175175
var metadata map[string]interface{}
176176

177177
for _, flag := range allFlags {
178-
if flag.State == Disabled {
179-
// ignore evaluation of disabled flag
180-
continue
181-
}
182-
183178
defaultValue := flag.Variants[flag.DefaultVariant]
184179
switch defaultValue.(type) {
185180
case bool:
@@ -313,6 +308,11 @@ func resolve[T constraints](ctx context.Context, reqID string, key string, conte
313308
return zero, variant, model.FallbackReason, metadata, nil
314309
}
315310

311+
if reason == model.DisabledReason {
312+
var zero T
313+
return zero, "", model.DisabledReason, metadata, nil
314+
}
315+
316316
var ok bool
317317
value, ok = variants[variant].(T)
318318
if !ok {
@@ -348,7 +348,7 @@ func (je *Resolver) evaluateVariant(ctx context.Context, reqID string, flagKey s
348348

349349
if flag.State == Disabled {
350350
je.Logger.DebugWithID(reqID, fmt.Sprintf("requested flag is disabled: %s", flagKey))
351-
return "", flag.Variants, model.ErrorReason, metadata, errors.New(model.FlagDisabledErrorCode)
351+
return "", nil, model.DisabledReason, metadata, nil
352352
}
353353

354354
// get the targeting logic, if any

‎core/pkg/evaluator/json_test.go‎

Lines changed: 43 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -467,9 +467,11 @@ func TestResolveAllValues(t *testing.T) {
467467
}
468468

469469
for _, val := range vals {
470-
// disabled flag must be ignored from bulk evaluation
470+
// disabled flag must not be ignored from bulk evaluation
471471
if val.FlagKey == DisabledFlag {
472-
t.Errorf("disabled flag '%s' is present in evaluation results", DisabledFlag)
472+
assert.Equal(t, model.DisabledReason, val.Reason)
473+
assert.Nil(t, val.Error)
474+
continue
473475
}
474476

475477
switch vT := val.Value.(type) {
@@ -510,7 +512,7 @@ func TestResolveBooleanValue(t *testing.T) {
510512
{DynamicBoolFlag, map[string]interface{}{ColorProp: ColorValue}, StaticBoolValue, model.TargetingMatchReason, ""},
511513
{StaticObjectFlag, nil, StaticBoolValue, model.ErrorReason, model.TypeMismatchErrorCode},
512514
{MissingFlag, nil, StaticBoolValue, model.ErrorReason, model.FlagNotFoundErrorCode},
513-
{DisabledFlag, nil, StaticBoolValue, model.ErrorReason, model.FlagDisabledErrorCode},
515+
{DisabledFlag, nil, false, model.DisabledReason, ""},
514516
}
515517
const reqID = "default"
516518
evaluator := flagdEvaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
@@ -545,7 +547,7 @@ func BenchmarkResolveBooleanValue(b *testing.B) {
545547
{DynamicBoolFlag, map[string]interface{}{ColorProp: ColorValue}, StaticBoolValue, model.TargetingMatchReason, ""},
546548
{StaticObjectFlag, nil, StaticBoolValue, model.ErrorReason, model.TypeMismatchErrorCode},
547549
{MissingFlag, nil, StaticBoolValue, model.ErrorReason, model.FlagNotFoundErrorCode},
548-
{DisabledFlag, nil, StaticBoolValue, model.ErrorReason, model.FlagDisabledErrorCode},
550+
{DisabledFlag, nil, false, model.DisabledReason, ""},
549551
}
550552

551553
evaluator := flagdEvaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
@@ -585,7 +587,7 @@ func TestResolveStringValue(t *testing.T) {
585587
{DynamicStringFlag, map[string]interface{}{ColorProp: ColorValue}, DynamicStringValue, model.TargetingMatchReason, ""},
586588
{StaticObjectFlag, nil, "", model.ErrorReason, model.TypeMismatchErrorCode},
587589
{MissingFlag, nil, "", model.ErrorReason, model.FlagNotFoundErrorCode},
588-
{DisabledFlag, nil, "", model.ErrorReason, model.FlagDisabledErrorCode},
590+
{DisabledFlag, nil, "", model.DisabledReason, ""},
589591
}
590592
const reqID = "default"
591593
evaluator := flagdEvaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
@@ -621,7 +623,7 @@ func BenchmarkResolveStringValue(b *testing.B) {
621623
{DynamicStringFlag, map[string]interface{}{ColorProp: ColorValue}, DynamicStringValue, model.TargetingMatchReason, ""},
622624
{StaticObjectFlag, nil, "", model.ErrorReason, model.TypeMismatchErrorCode},
623625
{MissingFlag, nil, "", model.ErrorReason, model.FlagNotFoundErrorCode},
624-
{DisabledFlag, nil, "", model.ErrorReason, model.FlagDisabledErrorCode},
626+
{DisabledFlag, nil, "", model.DisabledReason, ""},
625627
}
626628

627629
evaluator := flagdEvaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
@@ -661,7 +663,7 @@ func TestResolveFloatValue(t *testing.T) {
661663
{DynamicFloatFlag, map[string]interface{}{ColorProp: ColorValue}, DynamicFloatValue, model.TargetingMatchReason, ""},
662664
{StaticObjectFlag, nil, 13, model.ErrorReason, model.TypeMismatchErrorCode},
663665
{MissingFlag, nil, 13, model.ErrorReason, model.FlagNotFoundErrorCode},
664-
{DisabledFlag, nil, 0, model.ErrorReason, model.FlagDisabledErrorCode},
666+
{DisabledFlag, nil, 0, model.DisabledReason, ""},
665667
}
666668
const reqID = "default"
667669
evaluator := flagdEvaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
@@ -697,7 +699,7 @@ func BenchmarkResolveFloatValue(b *testing.B) {
697699
{DynamicFloatFlag, map[string]interface{}{ColorProp: ColorValue}, DynamicFloatValue, model.TargetingMatchReason, ""},
698700
{StaticObjectFlag, nil, 13, model.ErrorReason, model.TypeMismatchErrorCode},
699701
{MissingFlag, nil, 13, model.ErrorReason, model.FlagNotFoundErrorCode},
700-
{DisabledFlag, nil, 0, model.ErrorReason, model.FlagDisabledErrorCode},
702+
{DisabledFlag, nil, 0, model.DisabledReason, ""},
701703
}
702704

703705
evaluator := flagdEvaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
@@ -737,7 +739,7 @@ func TestResolveIntValue(t *testing.T) {
737739
{DynamicIntFlag, map[string]interface{}{ColorProp: ColorValue}, DynamicIntValue, model.TargetingMatchReason, ""},
738740
{StaticObjectFlag, nil, 13, model.ErrorReason, model.TypeMismatchErrorCode},
739741
{MissingFlag, nil, 13, model.ErrorReason, model.FlagNotFoundErrorCode},
740-
{DisabledFlag, nil, 0, model.ErrorReason, model.FlagDisabledErrorCode},
742+
{DisabledFlag, nil, 0, model.DisabledReason, ""},
741743
}
742744
const reqID = "default"
743745
evaluator := flagdEvaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
@@ -773,7 +775,7 @@ func BenchmarkResolveIntValue(b *testing.B) {
773775
{DynamicIntFlag, map[string]interface{}{ColorProp: ColorValue}, DynamicIntValue, model.TargetingMatchReason, ""},
774776
{StaticObjectFlag, nil, 13, model.ErrorReason, model.TypeMismatchErrorCode},
775777
{MissingFlag, nil, 13, model.ErrorReason, model.FlagNotFoundErrorCode},
776-
{DisabledFlag, nil, 0, model.ErrorReason, model.FlagDisabledErrorCode},
778+
{DisabledFlag, nil, 0, model.DisabledReason, ""},
777779
}
778780

779781
evaluator := flagdEvaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
@@ -801,6 +803,33 @@ func BenchmarkResolveIntValue(b *testing.B) {
801803
}
802804
}
803805

806+
func assertObjectResolveResult(
807+
t *testing.T,
808+
flagKey, expectedJSON, expectedReason, errorCode string,
809+
val map[string]any,
810+
reason string,
811+
err error,
812+
) {
813+
t.Helper()
814+
if errorCode != "" {
815+
assert.Equal(t, model.ErrorReason, reason)
816+
assert.EqualError(t, err, errorCode)
817+
return
818+
}
819+
if !assert.NoError(t, err) {
820+
return
821+
}
822+
if flagKey == DisabledFlag {
823+
assert.Nil(t, val)
824+
} else {
825+
marshalled, marshalErr := json.Marshal(val)
826+
if assert.NoError(t, marshalErr) {
827+
assert.JSONEq(t, expectedJSON, string(marshalled))
828+
}
829+
}
830+
assert.Equal(t, expectedReason, reason)
831+
}
832+
804833
func TestResolveObjectValue(t *testing.T) {
805834
tests := []struct {
806835
flagKey string
@@ -813,7 +842,7 @@ func TestResolveObjectValue(t *testing.T) {
813842
{DynamicObjectFlag, map[string]interface{}{ColorProp: ColorValue}, DynamicObjectValue, model.TargetingMatchReason, ""},
814843
{StaticBoolFlag, nil, "{}", model.ErrorReason, model.TypeMismatchErrorCode},
815844
{MissingFlag, nil, "{}", model.ErrorReason, model.FlagNotFoundErrorCode},
816-
{DisabledFlag, nil, "{}", model.ErrorReason, model.FlagDisabledErrorCode},
845+
{DisabledFlag, nil, "", model.DisabledReason, ""},
817846
}
818847
const reqID = "default"
819848
evaluator := flagdEvaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
@@ -824,19 +853,7 @@ func TestResolveObjectValue(t *testing.T) {
824853

825854
for _, test := range tests {
826855
val, _, reason, _, err := evaluator.ResolveObjectValue(context.TODO(), reqID, test.flagKey, test.context)
827-
828-
if test.errorCode == "" {
829-
if assert.NoError(t, err) {
830-
marshalled, err := json.Marshal(val)
831-
if assert.NoError(t, err) {
832-
assert.JSONEq(t, test.val, string(marshalled))
833-
assert.Equal(t, test.reason, reason)
834-
}
835-
}
836-
} else {
837-
assert.Equal(t, model.ErrorReason, reason)
838-
assert.EqualError(t, err, test.errorCode)
839-
}
856+
assertObjectResolveResult(t, test.flagKey, test.val, test.reason, test.errorCode, val, reason, err)
840857
}
841858
}
842859

@@ -852,7 +869,7 @@ func BenchmarkResolveObjectValue(b *testing.B) {
852869
{DynamicObjectFlag, map[string]interface{}{ColorProp: ColorValue}, DynamicObjectValue, model.TargetingMatchReason, ""},
853870
{StaticBoolFlag, nil, "{}", model.ErrorReason, model.TypeMismatchErrorCode},
854871
{MissingFlag, nil, "{}", model.ErrorReason, model.FlagNotFoundErrorCode},
855-
{DisabledFlag, nil, "{}", model.ErrorReason, model.FlagDisabledErrorCode},
872+
{DisabledFlag, nil, "null", model.DisabledReason, ""},
856873
}
857874

858875
evaluator := flagdEvaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
@@ -897,7 +914,7 @@ func TestResolveAsAnyValue(t *testing.T) {
897914
{DynamicObjectFlag, map[string]interface{}{ColorProp: ColorValue}, DynamicObjectValue, model.TargetingMatchReason, ""},
898915
// errors
899916
{MissingFlag, nil, "{}", model.ErrorReason, model.FlagNotFoundErrorCode},
900-
{DisabledFlag, nil, "{}", model.ErrorReason, model.FlagDisabledErrorCode},
917+
{DisabledFlag, nil, "{}", model.DisabledReason, ""},
901918
}
902919

903920
evaluator := flagdEvaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())

‎core/pkg/service/ofrep/models.go‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,16 @@ func SuccessResponseFrom(result evaluator.AnyValue) EvaluationSuccess {
7070
Metadata: result.Metadata,
7171
}
7272
}
73+
// if reason is disabled, we want to omit the value and variant from the response
74+
if result.Reason == model.DisabledReason {
75+
return EvaluationSuccess{
76+
Value: nil, // not marshalled due to omitempty
77+
Key: result.FlagKey,
78+
Reason: model.DisabledReason,
79+
Variant: "", // not marshalled due to omitempty
80+
Metadata: result.Metadata,
81+
}
82+
}
7383
return EvaluationSuccess{
7484
Value: result.Value,
7585
Key: result.FlagKey,
@@ -114,10 +124,6 @@ func EvaluationErrorResponseFrom(result evaluator.AnyValue) (int, EvaluationErro
114124
status = 404
115125
payload.ErrorCode = model.FlagNotFoundErrorCode
116126
payload.ErrorDetails = fmt.Sprintf("flag `%s` does not exist", result.FlagKey)
117-
case model.FlagDisabledErrorCode:
118-
status = 404
119-
payload.ErrorCode = model.FlagNotFoundErrorCode
120-
payload.ErrorDetails = fmt.Sprintf("flag `%s` is disabled", result.FlagKey)
121127
case model.ParseErrorCode:
122128
payload.ErrorCode = model.ParseErrorCode
123129
payload.ErrorDetails = fmt.Sprintf("error parsing the flag `%s`", result.FlagKey)

‎core/pkg/service/ofrep/models_test.go‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -327,12 +327,6 @@ func TestErrorStatus(t *testing.T) {
327327
expectedStatus: 400,
328328
expectedCode: model.ParseErrorCode,
329329
},
330-
{
331-
name: "flag disabled",
332-
modelError: model.FlagDisabledErrorCode,
333-
expectedStatus: 404,
334-
expectedCode: model.FlagNotFoundErrorCode,
335-
},
336330
{
337331
name: "general error",
338332
modelError: model.GeneralErrorCode,

‎docs/reference/flag-definitions.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ See below for a detailed description of each property.
7575

7676
`state` is a **required** property.
7777
Validate states are "ENABLED" or "DISABLED".
78-
When the state is set to "DISABLED", flagd will behave like the flag doesn't exist.
78+
When the state is set to "DISABLED", flagd returns a successful evaluation with `reason=DISABLED` and no value or variant; the SDK surfaces the caller-provided default.
7979

8080
Example:
8181

‎flagd/pkg/service/flag-evaluation/flag_evaluator_types.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ func (r *objectResponse) SetResult(value map[string]any, variant, reason string,
176176
// V2 response types with optional value and variant
177177

178178
type responseV2[T constraints] interface {
179-
SetResult(value T, variant, reason string, metadata map[string]interface{}) error
179+
response[T]
180180
SetReasonOnly(reason string, metadata map[string]interface{}) error
181181
}
182182

‎flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go‎

Lines changed: 17 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -244,14 +244,18 @@ func resolveV2[T constraints](ctx context.Context, logger *logger.Logger, resolv
244244
spanFromContext := trace.SpanFromContext(ctx)
245245
spanFromContext.SetAttributes(telemetry.SemConvFeatureFlagAttributes(flagKey, variant)...)
246246

247-
if reason == model.FallbackReason {
248-
if respV2, ok := resp.(responseV2[T]); ok {
249-
if err := respV2.SetReasonOnly(model.DefaultReason, metadata); err != nil {
250-
logger.ErrorWithID(reqID, err.Error())
251-
return fmt.Errorf("error setting response result: %w", err)
252-
}
247+
switch reason {
248+
case model.FallbackReason:
249+
if err := setReasonOnlyV2(resp, model.DefaultReason, metadata); err != nil {
250+
logger.ErrorWithID(reqID, err.Error())
251+
return fmt.Errorf("error setting fallback response: %w", err)
253252
}
254-
} else {
253+
case model.DisabledReason:
254+
if err := setReasonOnlyV2(resp, model.DisabledReason, metadata); err != nil {
255+
logger.ErrorWithID(reqID, err.Error())
256+
return fmt.Errorf("error setting disabled response: %w", err)
257+
}
258+
default:
255259
if err := resp.SetResult(result, variant, reason, metadata); err != nil && evalErr == nil {
256260
logger.ErrorWithID(reqID, err.Error())
257261
return fmt.Errorf("error setting response result: %w", err)
@@ -282,17 +286,11 @@ func recordResolveErrorV2(span trace.Span, err error, flagKey string) {
282286
}
283287
}
284288

285-
// errFormatV2 formats errors for V2 API, excluding FLAG_NOT_FOUND and PARSE_ERROR which are not errors in V2
286-
func errFormatV2(err error) error {
287-
ReadableErrorMsg := model.GetErrorMessage(err.Error())
288-
switch err.Error() {
289-
case model.FlagDisabledErrorCode:
290-
return connect.NewError(connect.CodeNotFound, fmt.Errorf("%s", ReadableErrorMsg))
291-
case model.TypeMismatchErrorCode:
292-
return connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("%s", ReadableErrorMsg))
293-
case model.GeneralErrorCode:
294-
return connect.NewError(connect.CodeUnknown, fmt.Errorf("%s", ReadableErrorMsg))
289+
// setReasonOnlyV2 writes a reason-only response (no value/variant) when resp is a V2 response.
290+
func setReasonOnlyV2[T constraints](resp response[T], reason string, metadata map[string]any) error {
291+
respV2, ok := resp.(responseV2[T])
292+
if !ok {
293+
return nil
295294
}
296-
297-
return err
295+
return respV2.SetReasonOnly(reason, metadata)
298296
}

0 commit comments

Comments
 (0)