Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flagsmith doesn't use boolean defaults and uses enablement status instead of flag value #1025

Open
uriell opened this issue Jul 25, 2024 · 3 comments

Comments

@uriell
Copy link

uriell commented Jul 25, 2024

Introduction

Hey team, we've began using Flagsmith in scenarios other than the most common or basic, specifically creating a boolean flag that can be overriden in certain cenarios for part of the clients based off a segment.

Basically the flag is usually a truthy value, but a segment override that will be edited over time may decrease the amount of clients that receive true and will now get a false value. Kinda like a soft phase-out of a feature.

While building that we noticed that the provider didn't take into account falsey default values in boolean flags, using a fixed truthy value in the code, and the evaluation for these boolean values was based off their existance/enablement, and not their actual value, preventing us from using these overrides that change the value, not the enabled status.

I took the liberty to aside from reporting this, also preparing a reproducible environment and a PR fix, but please feel free to discuss and opinate on the issue or solution.

Links

Reproducible Example: https://github.com/uriell/openfeature-flagsmith-boolean-issues
Pull Request: #1026

@beeme1mr
Copy link
Member

Thanks @uriell, circling in the Flagsmith team.

@dabeeeenster @matthewelwell

@uriell
Copy link
Author

uriell commented Jul 25, 2024

Something worth noting: Flags in Flagsmith don't require a value to be created, only a name and their enabled status. Therefore my PR in its current state would bring breaking changes to clients using flags that have no value set and rely on enabled.

I do however, think this proposal is still relevant since other OpenFeature SDKs for Flagsmith, like GoLang allow value to be used for booleans instead of enabled (at the developer's discretion). So there are two proposed scenarios:

  • Current, and probably default: Boolean flags evaluate based off enabled status:
    a. Overrides are not possible, values and default values are never used1;
  • useValueForBooleans in constructor: Boolean flags evaluate based off enabled, value and defaultValue:
    a. When a flag doesn't exist, it should return undefined and therefore use defaultValue;
    b. When enabled is true, but a flag value is not set, it will be falsey until explicitly set to true in flagsmith2;
    c. When enabled is true, the flag value should be returned;
    d. When enabled is false, should the flag value or defaultValue be returned?3;

Footnotes

  1. Currently, when a flag doesn't exist, flagsmith's hasFeature will return false, and since it is the correct expected type, it will be returned to the client without using the specified defaultValue;

  2. This is Flagsmith's getValue current behavior when a flag returns empty, and since it is the correct expected type, it will be returned to the client without using the specified defaultValue;

  3. I brought up this discussion since in GoLang's Flagsmith Provider, defaultValue is used when the flag has enabled set to false, and I'm not sure this would work for this client since hasFeature can return false both for missing flags and for flags that have enabled set to false, being slightly ambiguous;

@toddbaert
Copy link
Member

@dabeeeenster @matthewelwell any ideas on this? Maybe this could be a configuration thing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants