chore: changing error hook api to contain object #1525
Conversation
aepfli
force-pushed
the
demo/Errordetails_instead_of_exception
branch
from
f4e94d4 to
d717373
Compare
aepfli
changed the title
aepfli
force-pushed
the
demo/Errordetails_instead_of_exception
branch
from
d717373 to
ebcb5f4
Compare
This is a fast demo of open-feature/spec#326 As errors in the flag evaluation do not have to be exceptions, this opens up improvements for flow control via error codes from the EvaluationDetails. Signed-off-by: Simon Schrottner <[email protected]>
aepfli
force-pushed
the
demo/Errordetails_instead_of_exception
branch
from
ebcb5f4 to
ec55588
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1525 +/- ##
============================================
+ Coverage 92.82% 93.31% +0.49%
- Complexity 487 492 +5
============================================
Files 46 47 +1
Lines 1170 1182 +12
Branches 103 102 -1
============================================
+ Hits 1086 1103 +17
+ Misses 54 49 -5
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the error handling in hooks by introducing an ErrorDetails object, which is a great improvement for flow control and aligns with the OpenFeature specification. The implementation is mostly solid, with updated tests to match the new API. However, I've identified a critical bug in the new ErrorDetails class where a field is not being set, which could lead to null pointer exceptions. I've also included a suggestion to improve the design by reducing data redundancy. Please see the detailed comments.
| public static <T> ErrorDetails<T> from(Exception exception, FlagEvaluationDetails<T> details) { | ||
| if (exception instanceof OpenFeatureError) { | ||
| details.setErrorCode(((OpenFeatureError) exception).getErrorCode()); | ||
| } else { | ||
| details.setErrorCode(ErrorCode.GENERAL); | ||
| } | ||
| details.setErrorMessage(exception.getMessage()); | ||
| return ErrorDetails.<T>builder() | ||
| .error(exception) | ||
| .errorMessage(exception.getMessage()) | ||
| .errorCode(details.getErrorCode()) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
There's a critical issue in this factory method: the details field is not being set on the ErrorDetails object being built. This will cause getDetails() to return null on the created object, which could lead to NullPointerExceptions downstream.
Additionally, this method has a side effect of mutating the details parameter. While this seems to be intentional to centralize logic from OpenFeatureClient, it's generally better for factory methods named from() to be pure and not modify their arguments. This can make the code easier to reason about.
The suggestion below fixes the critical bug. You might also consider moving the mutation logic back to the caller in OpenFeatureClient to remove the side effect.
public static <T> ErrorDetails<T> from(Exception exception, FlagEvaluationDetails<T> details) {
if (exception instanceof OpenFeatureError) {
details.setErrorCode(((OpenFeatureError) exception).getErrorCode());
} else {
details.setErrorCode(ErrorCode.GENERAL);
}
details.setErrorMessage(exception.getMessage());
return ErrorDetails.<T>builder()
.error(exception)
.details(details)
.errorMessage(exception.getMessage())
.errorCode(details.getErrorCode())
.build();
}| Exception error; | ||
| FlagEvaluationDetails<T> details; | ||
| ErrorCode errorCode; | ||
| String errorMessage; |
There was a problem hiding this comment.
The ErrorDetails class contains errorCode and errorMessage fields, which are also present in the FlagEvaluationDetails object stored in the details field. This introduces data redundancy. After fixing the bug in the from(Exception, FlagEvaluationDetails) factory where details is not set, the details field will always be present.
To simplify the design and avoid redundancy, you could consider removing errorCode and errorMessage from ErrorDetails and access them via getDetails().getErrorCode() and getDetails().getErrorMessage(). Alternatively, you could add getter methods to ErrorDetails that delegate to the details object if you want to keep the convenient accessors.
|
This is a fast demo of open-feature/spec#326
As errors in the flag evaluation do not have to be exceptions, this opens up improvements for flow control via error codes from the EvaluationDetails.