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

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

Open
thomaspoignant opened this issue May 29, 2024 · 5 comments

Comments

@thomaspoignant
Copy link
Member

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
Copy link
Contributor

grimly commented May 29, 2024

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
Copy link
Contributor

Kavindu-Dodan commented May 29, 2024

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
Copy link
Contributor

grimly commented May 29, 2024

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
Copy link
Contributor

Kavindu-Dodan commented May 30, 2024

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
Copy link
Member

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.

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

No branches or pull requests

4 participants