Ambiguity around hook context and evaluation context.

[justinabrahms]

I'm attempting to implement merging before() hook values.

The logic is:

...
new_hook_ctx = run_before_hooks(hook_ctx, hook_hints)
eval_flag(new_hook_ctx.get_evaluation_context())
run_after_hooks(new_hook_ctx, hook_hints)
...

run_before_hooks will have an odd implementation because we want the before hooks to be able to alter the EvaluationContext. I don't think a before hook should be altering the other things in the HookCtx (flag key, default value, etc).

hook_ctx;
for hook in before_hooks:
  new_hook_ctx = hook(hook_ctx, hook_hints)
  hook_ctx.merge(new_hook_ctx)
return hook_ctx

Should the BeforeHook be returning a HookContext or the EvaluationContext? I'm thinking the latter, given that's the only thing I think we want to change.

That would make the code:

...
new_eval_context = run_before_hooks(hook_ctx, hook_hints)
new_hook_context = hook_ctx.with_context(new_eval_context)
eval_flag(new_hook_context)
run_after_hooks(new_hook_ctx, hook_hints)
...

run_before_hooks will have an odd implementation:

eval_ctx;
for hook in before_hooks:
  eval_ctx = hook(hook_ctx.with(eval_ctx), hook_hints)

return eval_ctx

This has implications for https://github.com/open-feature/spec/blob/main/specification/flag-evaluation/hooks.md#requirement-32

[beeme1mr]

I don't think a before hook should be altering the other things in the HookCtx (flag key, default value, etc).

I agree, these values should be immutable. Returning just the evaluation context in a before hook seems reasonable to me. @toddbaert, can you think of any issues with that?

[toddbaert]

I agree, these values should be immutable. Returning just the evaluation context in a before hook seems reasonable to me. @toddbaert, can you think of any issues with that?

No, makes sense to me. This is what happens in the playground - though there we also all it to return void (maybe your hook has a side effect only, and doesn't modify the context). Of course in that case, you can always return the context as well, but the purpose of the hook is a bit clearer if void returns are possible.

@justinabrahms what do you think of that? It might make the evaluation code a but more complex - you have to handle void returns.

EDIT: Oh, the spec already mentions "nothing", so I guess that's no problem.

[justinabrahms]

Void is fine. I'm using Optional<> in java land.