-
Notifications
You must be signed in to change notification settings - Fork 49
Description
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 issue is about requirement 4.1 Hook Context. Another discrepancy was reported here.
Context
This was initially reported to the python SDK here: open-feature/python-sdk#521
Relevant specification sections
The spec mandates that the hooks will receive a hook context which contains the evaluation context:
Hook context MUST provide: the flag key, flag value type, evaluation context, default value, and hook data.
It does not explicitly specify if "evaluation context" refers to the context provided by the caller (aka "invocation") or to the merged evaluation context, as specified in Requirement 3.2.3:
Evaluation context MUST be merged in the order: API (global; lowest precedence) -> transaction -> client -> invocation -> before hooks (highest precedence), with duplicate values being overwritten.
Difference between JS and Python implementations
In the JS case the SDK provides hooks with the evaluation context after the invocation context is merged into the global and transaction contexts.
In the python case this is not the case, with the hooks being presented with only the invocation context, although there is a PR meant to bring into alignment with the JS case here: open-feature/python-sdk#523
Proposal
It seems more reasonable to assume the correct behavior is what happens in the JS SDK, but it would be good if requirement 4.1.1 was updated to make explicit what the correct behavior is.
When executing the hooks, the hook context should contain the result of the merged evaluation context as specified in requirement 3.2.3. For example 4.1.1 could be updated to read:
Hook context MUST provide: the flag key, flag value type, merged evaluation context, default value, and hook data.