feat!: errorCode as enum, reason as string

[toddbaert]

• changes errorCode in resolution/evaluation structures to enum
• changes reason in resolution/evaluation structures to string

Closes: #78

Besides being compliant with the new spec changes, this makes our error handling more rigorous. Before, we were setting the error code from excpetion.getMessage(), which is not guaranteed to be a meaningful error code.

⚠️ the flagd integration tests WILL fail with this change ⚠️ - that's because the flagd provider needs these changes (a bit of a chicken/egg situation). I have a provider update ready locally to push as soon as this is published, which will then get the integration tests working again. This PR temporarily deactivates them. I don't expect this will happen often; this specifically made breaking changes to the provider contract.

[toddbaert]

I think it's still good to have an enum of default reasons. We just convert them to strings where applicable. We can instead use string constants... anyone have opinions on this?

[thomaspoignant]

Yes this a good idea to keep the enum 👍

[toddbaert]

Spec change.

[justinabrahms]

I think I missed this. We have both reason and message? Why would you want both?

[toddbaert]

message corresponds to the message property of a thrown exception. You can see we populate it with e.getMessage() in the handler.

I think it was one of the more popular parts of this change because actual error messages were being swallowed. It's quite useful for debugging, especially when developing providers: https://github.com/open-feature/spec/blob/main/specification/types.md#resolution-details

I'm thinking though that after this question, we may want to call it errorMessage (and it seems more consistent with the spec).

[toddbaert]

I've updated this to errorMessage

[toddbaert]

Spec change.

[codecov-commenter]

Codecov Report

Merging #80 (a495396) into main (e108666) will increase coverage by 0.59%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main      #80      +/-   ##
============================================
+ Coverage     91.24%   91.84%   +0.59%     
- Complexity      173      175       +2     
============================================
  Files            19       19              
  Lines           377      380       +3     
  Branches         22       23       +1     
============================================
+ Hits            344      349       +5     
+ Misses           22       20       -2     
  Partials         11       11              

Flag	Coverage Δ
unittests	91.84% <100.00%> (+0.59%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...dev/openfeature/javasdk/FlagEvaluationDetails.java	100.00% <ø> (ø)
...c/main/java/dev/openfeature/javasdk/ErrorCode.java	100.00% <100.00%> (+100.00%)	⬆️
...ain/java/dev/openfeature/javasdk/NoOpProvider.java	96.55% <100.00%> (ø)
...ava/dev/openfeature/javasdk/OpenFeatureClient.java	97.61% <100.00%> (+0.08%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[toddbaert]

Removing the integration-test profile disables the integration test suite as discussed in the description.

[toddbaert]

FYI @thomaspoignant I have updates to the contribs pending locally, including the flagd and the go-feature-flag provider for this.

[toddbaert]

I think this was an existing bug, unless I'm missing something.

[toddbaert]

This is the only behavioral change - if we can get an errorCode, use it. Otherwise use GENERAL.

Also, get the message from the message. Previously, the code was coming from the message.

[kinyoklion]

Would it be useful to have an overload for setting the reason based on a Reason? I am not really sure how these lombok classes work, aside from magic.

[toddbaert]

I'm hesitant to add it b/c it's not something lombok can do for us. I'm lazy.

[toddbaert]

Similar PR in c# for any interested: open-feature/dotnet-sdk#72

[skyerus]

Should we consider whether a provider might want to different reason to ERROR?

[toddbaert]

The spec says that reason must indicate an error in error cases (1.4.8). This is in the evaluationAPI section, not the provider section: https://github.com/open-feature/spec/blob/f74a8d98763bcac5d9a5c67a40ca9d7c2d445180/specification/sections/01-flag-evaluation.md#requirement-148. This is more of a SDK functionality concern in my opinion than an provider one.

I suppose the provider could provide a more specific error reason, but I think additional information can be better conveyed by the errorMessage and errorCode fields, and that should be the provider's role here. We can also have exceptions thrown outside of providers (hooks) - it's probably unrealistic to expect them to set a reason since flag resolution is not their concern.