Closed alxckn closed 3 months ago
From my look just now, it looks like reason
completely matches the spec, and we're missing the PROVIDER_FATAL error code on error codes. Would adding that be sufficient here, or am I missing something?
I may be biased by working on the flagd provider but my thinking behind this issue is that any open feature compliant provider may respond with spec compatible reason or error code. It'd be great to just be able to forward the codes sent by the remote providers, but with the current implementation I don't think we can and it will require mappings for each provider implementation even if open feature compliant.
For instance flagd may return "TARGETING_MATCH"
as reason but "TARGETING_MATCH" != OpenFeature::SDK::Provider::Reason::TARGETING_MATCH
but with the current implementation I don't think we can and it will require mappings for each provider implementation even if open feature compliant.
I'd think the provider's job is to provide the mapping from the provider's error domain into the standard OF error domain (with the ability to add any additional context in reason
, as that field has pre-defined types in the OF spec, but is not limited to those).
That said, I might still be missing what you're saying. Could you provide an example limitation in the flagd
implementation that is requiring extra work to be compatible with the current SDK?
In the flagd provider implementation, here is how the result is built: https://github.com/open-feature/ruby-sdk-contrib/blob/main/providers/openfeature-flagd-provider/lib/openfeature/flagd/provider/client.rb#L85
As you can see in this test, the reason
is STATIC
(upper case).
I think the SDK could support additional enum fields for reason, error or other fields but perhaps there's no need to reimplement custom mappings for OF compliant providers?
For the providers we've implemented, we've had a mapper that goes from the domain of the provider-specific results/errors into an OF-based reason, as defined in here. The backing strings aren't upper-case, which was an oversight over the type definition for reason. I'd accept a PR to change that casing.
The reason is an optional, open-ended string field. If the provider returns something equivalent to one of the enumerated reasons, we should map to those for consistency for users. If it doesn't map or could be interpreted as two different reasons, I would just pass back either what the provider returned or a provider-defined reason string that biases toward explanation.
The enum values should perhaps match the specification:
Existing open feature compliant providers such as flagd return these exact codes, I believe it could be beneficial tohomogenize for matching purposes and in case a provider proxy implementation needs to generate errors as well