Clarify 4.3.5 to say subsequent hook executions be made aware of previous updates to evaluation context

[kouk]

Overview

Hi, some discrepancies between the JS and Python SDKs was noticed with regards the implementation of the "before" hooks, that probably warrants an update to the spec to clarify the expected behavior (in favor of the JS implementation's behavior). This is about 4.3. Hook creation and parameters. Another discrepancy is reported here.

Context

This issue was first discussed in a PR to the python SDK here: open-feature/python-sdk#523

Relevant specification sections

The spec mandates that the before hooks should be able to update the evaluation context:

The evaluation context MUST be mutable only within the before hook.

Requirement 4.1.4.1

It also says that the before hooks can return an evaluation context which is used to update the existing evaluation context:

The before stage MUST run before flag resolution occurs. It accepts a hook context (required) and hook hints (optional) as parameters and returns either an evaluation context or nothing.

Requirement 4.3.2.1.

Evaluation context MUST be merged in the order: API (global; lowest precedence) -> transaction -> client -> invocation -> before hooks (highest precedence), with duplicate values being overwritten.

When before hooks have finished executing, any resulting evaluation context MUST be merged with the existing evaluation context.

Requirement 4.3.5

This is presumably so that Requirement 3.2.3 can be met, which says that the result of the before hooks be merged into the evaluation context in a given order.

However the spec does not explicitly say what "existing evaluation context" refers to. It is perhaps obvious that it would refer to the evaluation context that will be used when doing the flag evaluation. But it could additionally refer to the hook context that is provided to hooks via the hook context (as per 4.1.1). And it does not say when this merging should happen.

Question posed by the JS and Python implementations

The question I believe is whether requirement 4.3.5 implies that the hook context of subsequent hook executions should be made aware of evaluation context updates from previous hook executions, by doing the merging specified in 3.2.3 immediately after each before hook is executed.

In the JS case the implementation runs each "before" hook and immediately updates the evaluation context contained inside the hook context with the evaluation context returned by the hook. It then returns the resulting merged evaluation context (after all the "before" hooks had a chance to update it) to be used in evaluating the flag. The "error" or "after" hooks will receive the hook context and be aware of extra attributes that were added to the evaluation context by the "before" hooks.

However in the Python case the implementation does not update the evaluation context contained inside the hook context, but just returns the resulting merged evaluation context for use in evaluating the flag:

https://github.com/open-feature/python-sdk/blob/5652c0c457cc5a524e91405f3b229cf245ae4531/openfeature/hook/_hook_support.py#L92
https://github.com/open-feature/python-sdk/blob/5652c0c457cc5a524e91405f3b229cf245ae4531/openfeature/hook/_hook_support.py#L51
https://github.com/open-feature/python-sdk/blob/5652c0c457cc5a524e91405f3b229cf245ae4531/openfeature/client.py#L480

This results in a discrepancy where in the JS case subsequent hooks (including "before" hooks but also the "error" and "after" hooks) will receive a hook context which includes the most recently updated evaluation context, while in the Python case subsequent hooks will receive a hook context which includes the evaluation context as it was initially, before any "before" hooks potentially modified the attributes.

Proposal

It seems more reasonable to assume the correct behavior is what happens in the JS SDK, but it would be good if requirements 4.3.5 was updated to make explicit what the correct behavior is.

IMHO when a hook generates an update to the evaluation context it should be with the existing evaluation context and should affect not only the evaluation of the flag but also the execution of subsequent hooks (both subsequent "before" hooks and the "error" and "after" hooks that may be executed after evaluation)

The spec for example could be updated to read:

After each before hook has finished executing, any resulting evaluation context MUST be merged with the existing evaluation context and the result should be available in the hook context of subsequent hook executions (of any kind) as well as during flag evaluation.