[BUG] inconsitent behaviour in server side hookcontext data for evaluation context

[aepfli]

Observed behavior

take a close look at the code, comparing java and js there is a difference in how the hookcontext is build/enriched with evalutation context.

js-code: https://github.com/open-feature/js-sdk/blob/main/packages/server/src/client/internal/open-feature-client.ts#L255-L327

java-code: https://github.com/open-feature/java-sdk/blob/a08ff4d96ccf9236ad882892280b600a05409394/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java#L160-L231

In java the evaluation context is everytime updated, when it was changed (due to hooks, etc). in js it is only set a the beginning with the merged context, but does not update with the frozenContext from the beforehooks.

This leads to the problem that in the after, finally and error hooks the information in the hookcontext is not up to date.

Expected Behavior

We should solve this in a way that this is backwards compatible and we always have the full context available

Steps to reproduce

No response

[beeme1mr]

Hey @aepfli, the result of the before hook does update the context. Are you seeing a behavior issue?

[aepfli]

It is updating the evaluationcontext, but not updating the evaluationcontext in the hookcontext

[MarkoMlakar]

Hello :)
I have also been looking into this issue and wanted to share my thoughts. I agree with @beeme1mr. By the time after/error/finally hooks run, they already have access to the complete evaluation context.
There are also passing unit tests (client, hooks) which would likely fail if the merging logic was not working properly.

[beeme1mr]

I do think we're merging context properly before a flag evaluation. However, we may have a subtle bug because we don't update the hook context with the latest merged evaluation context. This would affect after, error, and finally` hooks that may be interested in the evaluation context that was used when evaluating the flag.

[MarkoMlakar]

@beeme1mr Thanks for clarifying :) I might be misunderstanding, but isn't the evaluation context updated with the latest merged context inside this loop? From what I can see, it looks like the context object is shared with all hooks (same object reference). If that’s the case the loop wouldn't be needed and could be replaced with
Object.assign(hookContext.context, accumulatedContext); ?
Happy to be corrected if I'm missing something :)

[beeme1mr]

Hey @MarkoMlakar, I believe you're correct but I'd have to open it up in my IDE to fully verify the behavior. That may be a good optimization.