feat: added implementation of immutable evaluation context

[thiyagu06]

Signed-off-by: thiyagu06 [email protected]

This PR

added implementation of immutable evaluation context

Related Issues

Fixes #161

Notes

Follow-up Tasks

How to test

[thiyagu06]

couple of questions to be confirmed.

1. Should we replace all the reference of mutableContext to ImmutableContext in tests?
2. Should ImmutableContext support merge method?

[toddbaert]

1. Should we replace all the reference of mutableContext to ImmutableContext in tests?

I think it doesn't matter so much in tests, but I would probably prefer it there.

What matters even more is that the (few) places where the SDK instantiates a context currently, it should be switched to the immutable context (where we currently create context in the SDK, they are not mutated)

2. Should ImmutableContext support merge method?

Yes, I think it needs to because the SDK calls merge internally. It should return a new immutable context.

[beeme1mr]

Hey @thiyagu06, thanks for your contribution. When you have a moment, please address the comments above and mark them as resolved. It would be great to include this change in an upcoming release. Thanks!

[codecov-commenter]

Codecov Report

Merging #210 (ecd0614) into main (6cb3fe4) will decrease coverage by 0.09%.
The diff coverage is 86.58%.

❗ Current head ecd0614 differs from pull request most recent head 96fdae5. Consider uploading reports for the commit 96fdae5 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main     #210      +/-   ##
============================================
- Coverage     91.60%   91.52%   -0.09%     
- Complexity      187      208      +21     
============================================
  Files            20       23       +3     
  Lines           417      472      +55     
  Branches         22       27       +5     
============================================
+ Hits            382      432      +50     
- Misses           23       26       +3     
- Partials         12       14       +2     

Flag	Coverage Δ
unittests	91.52% <86.58%> (-0.09%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ain/java/dev/openfeature/sdk/MutableStructure.java	96.66% <ø> (+3.80%)	⬆️
...n/java/dev/openfeature/sdk/ImmutableStructure.java	76.19% <76.19%> (ø)
src/main/java/dev/openfeature/sdk/Structure.java	88.46% <88.46%> (ø)
...ain/java/dev/openfeature/sdk/ImmutableContext.java	90.90% <90.90%> (ø)
src/main/java/dev/openfeature/sdk/Value.java	92.59% <91.66%> (+4.18%)	⬆️
...in/java/dev/openfeature/sdk/OpenFeatureClient.java	97.91% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[thiyagu06]

Not relevant to this PR, But I see the spec doesn't include evaluation context should support List type. But I see the support of List type in SDK. Do we need to update the spec?

https://github.com/open-feature/java-sdk/blob/main/src/main/java/dev/openfeature/sdk/Value.java#L42

https://docs.openfeature.dev/docs/specification/sections/evaluation-context#requirement-312

[toddbaert]

Not relevant to this PR, But I see the spec doesn't include evaluation context should support List type. But I see the support of List type in SDK. Do we need to update the spec?

https://github.com/open-feature/java-sdk/blob/main/src/main/java/dev/openfeature/sdk/Value.java#L42

https://docs.openfeature.dev/docs/specification/sections/evaluation-context#requirement-312

The spec references structure, which is an intentionally vague term that we define in our types. The structure may support lists or not, that's up to the implementation. This is an area we want to maintain as much flexibility as possible. This is part of the challenge in keeping a specification language agnostic.

[Kavindu-Dodan]

@thiyagu06 I see some of the conversations were marked "resolved" (ex:- #210 (comment)). But I am not seeing a new commit in the PR.

Could you please check whether you forgot to push your changes ☺️ ?

[thiyagu06]

@thiyagu06 I see some of the conversations were marked "resolved" (ex:- #210 (comment)). But I am not seeing a new commit in the PR.

Could you please check whether you forgot to push your changes ☺️ ?

Yes. I forgot 🤦‍♂️ . Pushed it now. thanks

[Kavindu-Dodan]

@thiyagu06 I see some of the conversations were marked "resolved" (ex:- #210 (comment)). But I am not seeing a new commit in the PR.
Could you please check whether you forgot to push your changes ☺️ ?

Yes. I forgot 🤦‍♂️ . Pushed it now. thanks

No worries.

I found few more improvements and once they are done, PR should be good to go :)

[Kavindu-Dodan]

Changes looks good to me

[toddbaert]

@Kavindu-Dodan @thiyagu06 The only thing I'm wondering here is if it's a good idea to throw in setTargetingKey or not. We could make it simply no-op and log a warning instead.

Either way, I think we should add a deprecated annotation and say it will be removed in the future. cc @justinabrahms @agentgonzo

[Kavindu-Dodan]

@Kavindu-Dodan @thiyagu06 The only thing I'm wondering here is if it's a good idea to throw in setTargetingKey or not. We could make it simply no-op and log a warning instead.

Either way, I think we should add a deprecated annotation and say it will be removed in the future. cc @justinabrahms @agentgonzo

In my view, throwing the exception is correct for the current implementation. The log could be missed and caller could be mislead as there is no reaction.

[toddbaert]

@Kavindu-Dodan @thiyagu06 The only thing I'm wondering here is if it's a good idea to throw in setTargetingKey or not. We could make it simply no-op and log a warning instead.
Either way, I think we should add a deprecated annotation and say it will be removed in the future. cc @justinabrahms @agentgonzo

In my view, throwing the exception is correct for the current implementation. The log could be missed and caller could be mislead as there is no reaction.

OK I've added an @Deprecated annotation and a comment.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

No Coverage information
0.0% Duplication

[toddbaert]

Thanks again for your contribution @thiyagu06 !