open-feature / spec

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

OpenFeature support for array-like flags. #138

Closed kinyoklion closed 1 year ago

kinyoklion commented 1 year ago

In the glossary of terms the example for values utilizes arrays to demonstrate the concept: https://github.com/open-feature/spec/blob/main/specification/glossary.md#values

The evaluation specification doesn't explicitly cover array-like flags: https://github.com/open-feature/spec/blob/main/specification/sections/01-flag-evaluation.md#requirement-131

Though the specification of a structure is vague and potentially could include array-like flags. I think it may be good to clarify their support, or lack of support.

toddbaert commented 1 year ago

Hey @kinyoklion thanks for noticing this.

I think this might be a good place for a brief discussion on returning arrays in the object evaluation... I wonder if it's worth considering returning our Value type in Java and dotnet, which can represent any JSON node (as well as other similar serialization formats). This would allow us to return arrays, though it also implies the object resolvers could return a wider variety of structures.

kinyoklion commented 1 year ago

I think that the Value type is a good representation for a flag value. That would also allow for heterogeneous flags in the case where a vendor supports it (without having to wrap that flag in an object).

toddbaert commented 1 year ago

I will play around with this option in the Java SDK. I don't think a spec change is required, since spec is intentionally vague here. I think for many SDKs in languages without static typing and with out-of-the-box JSON parsing, this should "just work".

toddbaert commented 1 year ago

I've tested this idea in the java-sdk and the java-flagd provider, and I have to say I think it's a good solution. It works out to be net negative lines of code in the provider and it gives us the flexibility to return arrays as the object return value (not that every provider would have to support doing so). This wouldn't be a spec change, it's just something that some SDKs would need to do (mostly dotnet and Java).

@rgrassian-split TLDR the discussion here is about changing the Java SDK object flag resolver to return a dev.openfeature.javasdk.Value instead of a dev.openfeature.javasdk.Structure. This allows the return of arrays, which some vendors support. I think it would be a fairly easy change to implement in the split provider. We're trying to get to 1.0 release candidates for all the SDKs, so we'd like to get any such breaking changes out of the way ASAP.

cc @kinyoklion @beeme1mr @justinabrahms @james-milligan @agentgonzo

rgrassian-split commented 1 year ago

@toddbaert @kinyoklion using a Value instead of Structure makes sense to me and should be a small change for our Provider. However I wanted to ask if there's a way we can change the way we create / get from the Value object, right now it has a bunch of constructors that take in different types of innerObjects and a bunch of gets that make it kind of hard to work with. Can we add an Object constructor as well? On our end we have to make a bunch of if statement checks to find the right constructor, and I could see other Providers facing the same issue to where it might be best to have that logic in the Value object itself. And for fetching, can we add a get innerObject that just returns an Object? Alternatively there, a variable to keep track of the type and a method to fetch that would also work instead. This is to avoid repeatedly calling the different get methods until a non-null value is returned. I doubt this would be a Split specific issue either and other Providers will probably face the same issues

toddbaert commented 1 year ago

Can we add an Object constructor as well? ... And for fetching, can we add a get innerObject that just returns an Object?

I see no issue with this additional flexibility, though I think I'd prefer to call it asObject(). Feel free to open a PR or I will do so when I action the Value/Structure change, unless anyone else has an objection.

toddbaert commented 1 year ago

@kinyoklion - closing this since I believe it's fully supported now via: https://github.com/open-feature/dotnet-sdk/pull/57 and https://github.com/open-feature/java-sdk/pull/65.

Please re-open if I'm wrong!