open-feature / spec

OpenFeature specification
https://openfeature.dev
Apache License 2.0
667 stars 35 forks source link

flag type in definition #108

Closed yousef-soliman closed 2 years ago

yousef-soliman commented 2 years ago

i think it will be better to define the flag type in a definition like this

"myBoolFlag": {
      "state": "ENABLED",
      "type": "BOOLEAN",
      "variants": {
        "on": true,
        "off": false
      },
      "defaultVariant": "on"
    },

it will let us abstract the endpoints at flagd

"/flags/{flag-key}/resolve/boolean"
"/flags/{flag-key}/resolve/numbe"
"/flags/{flag-key}/resolve/object"
"/flags/{flag-key}/resolve/string"

to one endpoint

"/flags/{flag-key}/resolve/"
beeme1mr commented 2 years ago

We originally had the flag configuration structured how you're proposing but decided to remove the return type because it can be inferred from the variant values.

The proposal regarding the rest endpoints could be doable either way. The current structure requires flagd to handle response type validation. The way you're proposing would require the provider to handle it. Perhaps we could consider having both.

What do you think @toddbaert?

toddbaert commented 2 years ago

Yes, as @beeme1mr notes, with multiple endpoints, the API itself can enforce types to make sure the flag value returned is coherent with what the client expected. This is consistent with the existing semantics of the specification, which require strongly typed flag resolution methods where possible: https://github.com/open-feature/spec/blob/main/specification/flag-evaluation.md#conditional-requirement-1321

We could handle this in the client, but the mode of thinking with flagd clients/providers is to keep them as thin as possible, so that they are very simple to implement in any language, offloading the evaluation logic to flagd itself.

As far as the schema, the type can be inferred from the values, and mixed variants are not allowed. Put another way, since you can't mix variant values like this:

      "variants": {
        "on": true,
        "off": 1
      },

simply specifying your variants implicitly gives a type to the flag, making the "type" field a bit redundant. I think. There might be some reason it's valid to have a flag with no variants... but I can't think of any practical one.

toddbaert commented 2 years ago

As a somewhat related point... we do have some code duplication in flagd as a consequence of this, and the generated endpoints. I would like to eventually rectify that issue with an upgrade to go 1.18 and the use of generics.

toddbaert commented 2 years ago

Closing this for now