Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions src/main/java/dev/openfeature/sdk/ErrorDetails.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package dev.openfeature.sdk;

import dev.openfeature.sdk.exceptions.OpenFeatureError;
import lombok.Builder;
import lombok.Value;

/**
* Represents details about an error that occurred during a flag evaluation or other operations.
* This class captures the exception, evaluation details, error code, and an error message.
*
* @param <T> The type of the value being evaluated in the {@link FlagEvaluationDetails}.
*/
@Value
@Builder
public class ErrorDetails<T> {
Exception error;
FlagEvaluationDetails<T> details;
ErrorCode errorCode;
String errorMessage;
Comment on lines +16 to +19
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.


/**
* Creates an {@code ErrorDetails} instance from the given {@link FlagEvaluationDetails}.
* This method extracts the error message and error code from the provided details.
*
* @param details The {@link FlagEvaluationDetails} object containing flag evaluation information.
* @param <T> The type of the value being evaluated in the {@link FlagEvaluationDetails}.
* @return An {@code ErrorDetails} object populated with the provided evaluation details.
*/
public static <T> ErrorDetails<T> from(FlagEvaluationDetails<T> details) {
return ErrorDetails.<T>builder()
.details(details)
.errorMessage(details.getErrorMessage())
.errorCode(details.getErrorCode())
.build();
}

/**
* Creates an {@code ErrorDetails} instance from the given exception and {@link FlagEvaluationDetails}.
* If the exception is an instance of {@link OpenFeatureError}, its error code is extracted
* and set in the {@link FlagEvaluationDetails}. Otherwise, a general error code is used.
* The exception's message is also set as the error message.
*
* @param exception The exception that occurred during the operation.
* @param details The {@link FlagEvaluationDetails} object containing flag evaluation information.
* @param <T> The type of the value being evaluated in the {@link FlagEvaluationDetails}.
* @return An {@code ErrorDetails} object populated with the exception and evaluation details.
*/
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();
}
Comment on lines +48 to +60
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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();
    }

}
16 changes: 15 additions & 1 deletion src/main/java/dev/openfeature/sdk/Hook.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package dev.openfeature.sdk;

import dev.openfeature.sdk.exceptions.ExceptionUtils;
import java.util.Map;
import java.util.Optional;

Expand Down Expand Up @@ -35,9 +36,22 @@ default void after(HookContext<T> ctx, FlagEvaluationDetails<T> details, Map<Str
* Run when evaluation encounters an error. This will always run. Errors thrown will be swallowed.
*
* @param ctx Information about the particular flag evaluation
* @param error The exception that was thrown.
* @param error The error details might contain an exception that was thrown.
* @param hints An immutable mapping of data for users to communicate to the hooks.
*/
default void error(HookContext<T> ctx, ErrorDetails<T> error, Map<String, Object> hints) {
error(ctx, ExceptionUtils.instantiateErrorByErrorCode(error.getErrorCode(), error.getErrorMessage()), hints);
}

/**
* Run when evaluation encounters an error. This will always run. Errors thrown will be swallowed.
*
* @param ctx Information about the particular flag evaluation
* @param error The exception that was thrown or instantiated.
* @param hints An immutable mapping of data for users to communicate to the hooks.
* @deprecated Please use ErrorDetails instead of Exceptions.
*/
@Deprecated
default void error(HookContext<T> ctx, Exception error, Map<String, Object> hints) {}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/dev/openfeature/sdk/HookSupport.java
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public void afterAllHooks(
public void errorHooks(
FlagValueType flagValueType,
HookContext hookCtx,
Exception e,
ErrorDetails e,
List<Hook> hooks,
Map<String, Object> hints) {
executeHooks(flagValueType, hooks, "error", hook -> hook.error(hookCtx, e, hints));
Expand Down
14 changes: 3 additions & 11 deletions src/main/java/dev/openfeature/sdk/OpenFeatureClient.java
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
package dev.openfeature.sdk;

import dev.openfeature.sdk.exceptions.ExceptionUtils;
import dev.openfeature.sdk.exceptions.FatalError;
import dev.openfeature.sdk.exceptions.GeneralError;
import dev.openfeature.sdk.exceptions.OpenFeatureError;
import dev.openfeature.sdk.exceptions.ProviderNotReadyError;
import dev.openfeature.sdk.internal.ObjectUtils;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
Expand Down Expand Up @@ -204,9 +202,8 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(

details = FlagEvaluationDetails.from(providerEval, key);
if (details.getErrorCode() != null) {
var error =
ExceptionUtils.instantiateErrorByErrorCode(details.getErrorCode(), details.getErrorMessage());
enrichDetailsWithErrorDefaults(defaultValue, details);
ErrorDetails error = ErrorDetails.from(details);
hookSupport.errorHooks(type, afterHookContext, error, mergedHooks, hints);
} else {
hookSupport.afterHooks(type, afterHookContext, details, mergedHooks, hints);
Expand All @@ -215,14 +212,9 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(
if (details == null) {
details = FlagEvaluationDetails.<T>builder().flagKey(key).build();
}
if (e instanceof OpenFeatureError) {
details.setErrorCode(((OpenFeatureError) e).getErrorCode());
} else {
details.setErrorCode(ErrorCode.GENERAL);
}
details.setErrorMessage(e.getMessage());
enrichDetailsWithErrorDefaults(defaultValue, details);
hookSupport.errorHooks(type, afterHookContext, e, mergedHooks, hints);
ErrorDetails error = ErrorDetails.from(e, details);
hookSupport.errorHooks(type, afterHookContext, error, mergedHooks, hints);
} finally {
hookSupport.afterAllHooks(type, afterHookContext, details, mergedHooks, hints);
}
Expand Down
38 changes: 19 additions & 19 deletions src/test/java/dev/openfeature/sdk/HookSpecTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import static org.assertj.core.api.Assertions.assertThatCode;
import static org.assertj.core.api.Assertions.fail;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
Expand All @@ -16,7 +15,6 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import dev.openfeature.sdk.exceptions.FlagNotFoundError;
import dev.openfeature.sdk.fixtures.HookFixtures;
import dev.openfeature.sdk.testutils.TestEventsProvider;
import java.util.ArrayList;
Expand Down Expand Up @@ -200,7 +198,7 @@ void error_hook_run_during_non_finally_stage() {
Hook h = mockBooleanHook();
doThrow(RuntimeException.class).when(h).finallyAfter(any(), any(), any());

verify(h, times(0)).error(any(), any(), any());
verify(h, times(0)).error(any(), any(Exception.class), any());
}

@Test
Expand All @@ -225,16 +223,16 @@ void error_hook_must_run_if_resolution_details_returns_an_error_code() {
invocationCtx,
FlagEvaluationOptions.builder().hook(hook).build());

ArgumentCaptor<Exception> captor = ArgumentCaptor.forClass(Exception.class);
ArgumentCaptor<ErrorDetails> captor = ArgumentCaptor.forClass(ErrorDetails.class);

verify(hook, times(1)).before(any(), any());
verify(hook, times(1)).error(any(), captor.capture(), any());
verify(hook, times(1)).finallyAfter(any(), any(), any());
verify(hook, never()).after(any(), any(), any());

Exception exception = captor.getValue();
assertEquals(errorMessage, exception.getMessage());
assertInstanceOf(FlagNotFoundError.class, exception);
ErrorDetails errorDetails = captor.getValue();
assertEquals(errorMessage, errorDetails.getErrorMessage());
assertEquals(ErrorCode.FLAG_NOT_FOUND, errorDetails.getErrorCode());
}

@Specification(
Expand Down Expand Up @@ -279,7 +277,8 @@ public void after(
}

@Override
public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object> hints) {
public void error(
HookContext<Boolean> ctx, ErrorDetails<Boolean> error, Map<String, Object> hints) {
evalOrder.add("provider error");
}

Expand Down Expand Up @@ -308,7 +307,7 @@ public void after(
}

@Override
public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object> hints) {
public void error(HookContext<Boolean> ctx, ErrorDetails<Boolean> error, Map<String, Object> hints) {
evalOrder.add("api error");
}

Expand All @@ -334,7 +333,7 @@ public void after(
}

@Override
public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object> hints) {
public void error(HookContext<Boolean> ctx, ErrorDetails<Boolean> error, Map<String, Object> hints) {
evalOrder.add("client error");
}

Expand Down Expand Up @@ -367,7 +366,8 @@ public void after(
}

@Override
public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object> hints) {
public void error(
HookContext<Boolean> ctx, ErrorDetails<Boolean> error, Map<String, Object> hints) {
evalOrder.add("invocation error");
}

Expand Down Expand Up @@ -546,7 +546,7 @@ void error_hooks__before() {
new ImmutableContext(),
FlagEvaluationOptions.builder().hook(hook).build());
verify(hook, times(1)).before(any(), any());
verify(hook, times(1)).error(any(), any(), any());
verify(hook, times(1)).error(any(), any(ErrorDetails.class), any());
assertEquals(false, value, "Falls through to the default.");
}

Expand All @@ -564,7 +564,7 @@ void error_hooks__after() {
new ImmutableContext(),
FlagEvaluationOptions.builder().hook(hook).build());
verify(hook, times(1)).after(any(), any(), any());
verify(hook, times(1)).error(any(), any(), any());
verify(hook, times(1)).error(any(), any(ErrorDetails.class), any());
}

@Test
Expand Down Expand Up @@ -609,7 +609,7 @@ void shortCircuit_flagResolution_runsHooksWithAllFields() {
FlagEvaluationOptions.builder().hook(hook).build());

verify(hook).before(any(), any());
verify(hook).error(any(HookContext.class), any(Exception.class), any(Map.class));
verify(hook).error(any(HookContext.class), any(ErrorDetails.class), any(Map.class));
verify(hook).finallyAfter(any(HookContext.class), any(FlagEvaluationDetails.class), any(Map.class));
}

Expand Down Expand Up @@ -655,8 +655,8 @@ void multi_hooks_early_out__before() {
verify(hook, times(1)).before(any(), any());
verify(hook2, times(0)).before(any(), any());

verify(hook, times(1)).error(any(), any(), any());
verify(hook2, times(1)).error(any(), any(), any());
verify(hook, times(1)).error(any(), any(ErrorDetails.class), any());
verify(hook2, times(1)).error(any(), any(ErrorDetails.class), any());
}

@Specification(number = "4.1.4", text = "The evaluation context MUST be mutable only within the before hook.")
Expand Down Expand Up @@ -760,7 +760,7 @@ void first_finally_broken() {
void first_error_broken() {
Hook hook = mockBooleanHook();
doThrow(RuntimeException.class).when(hook).before(any(), any());
doThrow(RuntimeException.class).when(hook).error(any(), any(), any());
doThrow(RuntimeException.class).when(hook).error(any(), any(Exception.class), any());
Hook hook2 = mockBooleanHook();
InOrder order = inOrder(hook, hook2);

Expand All @@ -772,8 +772,8 @@ void first_error_broken() {
FlagEvaluationOptions.builder().hook(hook2).hook(hook).build());

order.verify(hook).before(any(), any());
order.verify(hook2).error(any(), any(), any());
order.verify(hook).error(any(), any(), any());
order.verify(hook2).error(any(), any(ErrorDetails.class), any());
order.verify(hook).error(any(), any(ErrorDetails.class), any());
}

private Client getClient(FeatureProvider provider) {
Expand Down
23 changes: 7 additions & 16 deletions src/test/java/dev/openfeature/sdk/HookSupportTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,31 +55,22 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {
() -> "client",
() -> "provider");

FlagEvaluationDetails<Object> details = FlagEvaluationDetails.builder().build();
ErrorDetails<Object> error = ErrorDetails.from(expectedException, details);

hookSupport.beforeHooks(
flagValueType, hookContext, Collections.singletonList(genericHook), Collections.emptyMap());
hookSupport.afterHooks(
flagValueType,
hookContext,
FlagEvaluationDetails.builder().build(),
Collections.singletonList(genericHook),
Collections.emptyMap());
flagValueType, hookContext, details, Collections.singletonList(genericHook), Collections.emptyMap());
hookSupport.afterAllHooks(
flagValueType,
hookContext,
FlagEvaluationDetails.builder().build(),
Collections.singletonList(genericHook),
Collections.emptyMap());
flagValueType, hookContext, details, Collections.singletonList(genericHook), Collections.emptyMap());
hookSupport.errorHooks(
flagValueType,
hookContext,
expectedException,
Collections.singletonList(genericHook),
Collections.emptyMap());
flagValueType, hookContext, error, Collections.singletonList(genericHook), Collections.emptyMap());

verify(genericHook).before(any(), any());
verify(genericHook).after(any(), any(), any());
verify(genericHook).finallyAfter(any(), any(), any());
verify(genericHook).error(any(), any(), any());
verify(genericHook).error(any(), any(ErrorDetails.class), any());
}

private Object createDefaultValue(FlagValueType flagValueType) {
Expand Down
Loading