open-feature / protocol

OpenFeature Remote Evaluation Protocol
https://openfeature.dev
Apache License 2.0
19 stars 3 forks source link

ADR: Add ADR for unsupportedTypes option in the configuration endpoint. #21

Open thomaspoignant opened 1 month ago

thomaspoignant commented 1 month ago

During the initial discussion about the configuration endpoint we decided to use unsupportedTypes rather than supportedTypes to be sure that the default list is to support all the types.

We need an ADR to explain the reasoning about this decision.

grimly commented 1 month ago

I would advocate a strictly additive capabilities model.

I understand the idea is for a client of the API to fail fast and prevent a call the server for unsupported types. But the client might not have the capability to check for this configuration.

I'm looking at the OIDC /.well-known/openid-configuration, they only provide *_supported attributes and all extensions do that as well. I think this is a good pattern.

Kavindu-Dodan commented 1 month ago

I'm looking at the OIDC /.well-known/openid-configuration, they only provide *_supported attributes and all extensions do that as well. I think this is a good pattern.

Yeah, this is a good example of additive capability. But in contrast, OpenFeature defines evaluation types through the API and provider is expected to implement them. So if the configuration endpoint defines flagEvaluation to be an optional configuration, OFREP provider has to assume that all types are supported by default.

grimly commented 1 month ago

OpenFeature defines evaluation types

ADR 3 discarded it here. I was recalled of it on Slack.

Also the OF spec defines "Structured" flags. They never said arrays were out of the question. I do have use cases of a country set flag and a currency set flags. In JSON they would be materialized by an array. Do we add array to the enum ? That itself is a breaking change. Do we change the definition of object ? Breaking change too

If one think of that right after a V1 of this spec, you will get a V2 really fast.

Also the flagEvaluation name itself is an issue to me, it is too generic considering the scope of OpenFeature. We want to use it for typed evaluation shortcut, but what if I want to build an extension that exposes a list of supported key patterns ? The name fits my purpose but too bad, it was taken.

Kavindu-Dodan commented 1 month ago

ADR 3 discarded it here. I was recalled of it on Slack.

Yes, the conclusion was to omit this at that stage and keep the payloads minimal.

I do have use cases of a country set flag and a currency set flags. In JSON they would be materialized by an array.

From OFREP point of view, you can evaluate a single flag or bulk flags. Single flag evaluation is recommended for server use cases where context can change per evaluation. Bulk evaluations are intended for client (browser, mobile) usage. Do you plan to evaluate multiple flags through Object/structured evaluation ? If so that's not the intended usage of an object/strucutre evaluation. Besides the interpretation of the object/structure result is at the application level. So you can convert the resulting evaluation value to a json array or any other dto used internally.

If one think of that right after a V1 of this spec, you will get a V2 really fast.

V1 path segment is there so we can differentiate any divergent spec in the future. But we are not V1 complete as stated in the readme as the spec is still WIP. This is why we appreciate feedback like yours to improve the spec :)

flagEvaluation

If you check the schema, flagEvaluation is an object allowing us to add extra content. So if we think of adding supportingTypes which take precedence over flagEvaluation, then can still do that.

  "flagEvaluation": {
    "unsupportedTypes": [
      "object",
      "int",
      "float"
    ],
    "supportingTypes":[
       "string",
      "boolean"
    ]
  }
justinabrahms commented 6 hours ago

Just a note.. I have a flagging endpoint which only supports booleans, because it's the only data type that makes sense for our use-case (e.g. "Am I allowed to call $service?"). I vote in favor of "this is the list of things I support", otherwise there is ongoing maintenance for our system to keep the SDK well-behaved. I expect the OF release cycle to be different than ours and wouldn't want bad UX for users as a result.