Code review, minor suggestions/comment changes, questions. Looks great!

This commit is contained in:
Todd Baert 2022-06-15 16:05:21 -04:00
parent 95305ec513
commit 555f954236
No known key found for this signature in database
GPG Key ID: 6832CDB677D5E06D
6 changed files with 11 additions and 3 deletions

View File

@ -1,7 +1,7 @@
package dev.openfeature.javasdk;
/**
* We differ between the evaluation results that providers return and what is given to the end users. This is a common interface between them.
* This is the common interface between the evaluation results that providers return and what is given to the end users.
* @param <T> The type of flag being evaluated.
*/
public interface BaseEvaluation<T> {

View File

@ -1,6 +1,6 @@
package dev.openfeature.javasdk;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.ObjectMapper; // jackson is pretty standard, I'm glad you decided on this for object support
import lombok.*;
import java.time.ZonedDateTime;
@ -28,6 +28,8 @@ public class EvaluationContext {
}
// TODO Not sure if I should have sneakythrows or checked exceptions here..
// do you mean un-checked exceptions?
// I had no idea about SneakyThrows, but it seems like a nicer solution than swallowing in an unchecked exception.
@SneakyThrows
public <T> void addStructureAttribute(String key, T value) {
jsonAttributes.put(key, objMapper.writeValueAsString(value));

View File

@ -5,6 +5,7 @@ package dev.openfeature.javasdk;
*/
public interface FeatureProvider {
Metadata getMetadata();
// I think default methods are nicer than an abstract class, +1 to this.
ProviderEvaluation<Boolean> getBooleanEvaluation(String key, Boolean defaultValue, EvaluationContext ctx, FlagEvaluationOptions options);
ProviderEvaluation<String> getStringEvaluation(String key, String defaultValue, EvaluationContext ctx, FlagEvaluationOptions options);
ProviderEvaluation<Integer> getIntegerEvaluation(String key, Integer defaultValue, EvaluationContext ctx, FlagEvaluationOptions options);

View File

@ -1,7 +1,7 @@
package dev.openfeature.javasdk;
/**
* An API for the type-specific fetch methods we offer end users.
* An API for the type-specific fetch methods offered to users.
*/
public interface Features {

View File

@ -9,6 +9,8 @@ import java.util.List;
@Data @Builder
public class FlagEvaluationOptions {
// I guess because we are using boxed types, we can pass "Object" as T everywhere to avoid the
// "raw types" warning, but I'm not sure if that's really too helpful beyond removing some warnings
@Singular private List<Hook> hooks;
@Builder.Default
private ImmutableMap<String, Object> hookHints = ImmutableMap.of();

View File

@ -84,6 +84,7 @@ public class OpenFeatureClient implements Client {
}
details.value = defaultValue;
details.reason = Reason.ERROR;
// We may want to extend exception and add a defined code field, instead of just using message.
details.errorCode = e.getMessage();
this.errorHooks(hookCtx, e, mergedHooks, hints);
} finally {
@ -93,6 +94,7 @@ public class OpenFeatureClient implements Client {
return details;
}
// errorHooks and afterAllHooks error handling seems consistent with spec, and what I implemented. I'm glad we landed here.
private void errorHooks(HookContext hookCtx, Exception e, List<Hook> hooks, ImmutableMap<String, Object> hints) {
for (Hook hook : hooks) {
try {
@ -122,6 +124,7 @@ public class OpenFeatureClient implements Client {
private EvaluationContext beforeHooks(HookContext hookCtx, List<Hook> hooks, ImmutableMap<String, Object> hints) {
// These traverse backwards from normal.
EvaluationContext ctx = hookCtx.getCtx();
// This is a bit counter-intuitive to me, just want to make sure... this is the stage that gets reversed because ArrayList.addAll() does a prepend, right?
for (Hook hook : Lists.reverse(hooks)) {
Optional<EvaluationContext> newCtx = hook.before(hookCtx, hints);
if (newCtx != null && newCtx.isPresent()) {