feat!: use evaluation context interface

[toddbaert]

This attempts to defer our immutability issues. It does this by:

• Extracting the read operations of EvaluationContext into and interface called EvaluationContext
• renaming the old EvaluationContext to MutableContext, and making it implement EvaluationContext
• Extracting the read operations of Structure into and interface called Structure
• renaming the old Structure to MutableStructure, and making it implement Structure
• all SDK methods now work with the new EvaluationContext interface

We can now implement whatever context we like - context that lock with mutexes, immutable contexts, whatever.

[codecov-commenter]

Codecov Report

Merging #112 (700cf54) into main (faca917) will decrease coverage by 0.62%.
The diff coverage is 84.80%.

@@             Coverage Diff              @@
##               main     #112      +/-   ##
============================================
- Coverage     91.84%   91.21%   -0.63%     
- Complexity      175      177       +2     
============================================
  Files            19       19              
  Lines           380      387       +7     
  Branches         23       23              
============================================
+ Hits            349      353       +4     
- Misses           20       23       +3     
  Partials         11       11              

Flag	Coverage Δ
unittests	91.21% <84.80%> (-0.63%)	⬇️

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

Impacted Files	Coverage Δ
...rc/main/java/dev/openfeature/sdk/NoOpProvider.java	96.55% <ø> (ø)
.../main/java/dev/openfeature/sdk/MutableContext.java	68.08% <68.08%> (ø)
...ain/java/dev/openfeature/sdk/MutableStructure.java	92.85% <92.85%> (ø)
src/main/java/dev/openfeature/sdk/HookSupport.java	97.56% <100.00%> (ø)
...in/java/dev/openfeature/sdk/OpenFeatureClient.java	97.64% <100.00%> (+0.02%)	⬆️

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

[toddbaert]

@justinabrahms I've marked this ready. I've tested this locally in the flagd providers and it works as expected. Please review the names and interfaces, especially.

[justinabrahms]

I'm not entirely clear why we're passing around MutableContexts so much. I'd guess that it would be only used by users and providers / hooks would all use the interface?

[justinabrahms]

Why did this change?

[toddbaert]

Merge was previously a static method. I don't think it can be any longer, since we have to instantiate a new instance of whatever implementation we are dealing with.

Previously, merge could easily handle if once of the context passed were null. With the new instance method, we have to make sure the the context is not null before calling it. In many of the tests, this.getEvaluationContext() and openfeatureApi.getEvaluationContext() were null.

I could change this snippet to not create the empty MutableContexts to save some GC, but it would be fairly ugly I think.

[toddbaert]

I'm not entirely clear why we're passing around MutableContexts so much. I'd guess that it would be only used by users and providers / hooks would all use the interface?

There may be a few cases where I use it unnecessarily, but in some cases we need to create an instance within the client/API. I'll see what I can do to minimize these.

[toddbaert]

I'm not entirely clear why we're passing around MutableContexts so much. I'd guess that it would be only used by users and providers / hooks would all use the interface?

There may be a few cases where I use it unnecessarily, but in some cases we need to create an instance within the client/API. I'll see what I can do to minimize these.

@justinabrahms I've eliminated quite a few of these in this commit. Thanks for catching that. There are a few more cases that can't quite as easily be eliminated, which I've pointed out in comments.

[kinyoklion]

It is possible that I am maybe not completely following this, but I think if the idea is to put contexts behind an interface, and then to later be able to swap them with minimal breakage, would you not want both mutable, and readonly access, to be interfaces, and for the instances to be created with factories (not builders, but just make me a new mutable instance)?

This would be especially important if the thought is that different implementations might exist to cater to different sensitives.

[toddbaert]

@justinabrahms I think this is a case we need to instantiate something. We need to pass some kind of context to the hooks (I don't think we want it to be null if none happens to have been set).

I don't think this is a problem, since in the hook it will be an EvaluationContext interface which (unless unsafely casted) can only be used for reads.

[justinabrahms]

Yep. I'm all for this idea. We should be able to swap this in the future in a backwards compatible way, provided that the underlying context is of the EvaluationContext type.

[toddbaert]

It is possible that I am maybe not completely following this, but I think if the idea is to put contexts behind an interface, and then to later be able to swap them with minimal breakage, would you not want both mutable, and readonly access, to be interfaces, and for the instances to be created with factories (not builders, but just make me a new mutable instance)?

This would be especially important if the thought is that different implementations might exist to cater to different sensitives.

I could be missing something, but the SDK itself never needs to mutate a context. That only occurs in application author code. For this reason, I think only the read operations are relevant to the interface. I think with the current scheme, we could implement an ImmutableContext that takes a map in it's constructor, or can be built with a builder, but is immutable after instantiation. It could be used everywhere a MutableContext is.

[kinyoklion]

It is possible that I am maybe not completely following this, but I think if the idea is to put contexts behind an interface, and then to later be able to swap them with minimal breakage, would you not want both mutable, and readonly access, to be interfaces, and for the instances to be created with factories (not builders, but just make me a new mutable instance)?
This would be especially important if the thought is that different implementations might exist to cater to different sensitives.

I could be missing something, but the SDK itself never needs to mutate a context. That only occurs in application author code. For this reason, I think only the read operations are relevant to the interface. I think with the current scheme, we could implement an ImmutableContext that takes a map in it's constructor, or can be built with a builder, but is immutable after instantiation. It could be used everywhere a MutableContext is.

I guess I mostly concerned with before hooks. They could come from a number of places and would need to be able to create some level of mutable context. A provider hook specifically. I guess they could make anything that complies with the interface, and the considerations are more constrained in that case. (If I were implementing one though, I would just want to make whatever instance type was canonical, or whatever mutable type the application is using.)

[toddbaert]

I guess they could make anything that complies with the interface, and the considerations are more constrained in that case.

The considerations more constrained, and additionally, the before hook also allows authors to return an instance of a context, which is merged with the ambient context as per the spec. This is the idiomatic way to alter the context from a before hook.

I think I see your point though, that hook authors have to make a choice here, and it's somewhat disconnected from whatever instances the application author is choosing to instantiate. I will think about that some more. I'm not sure how much of a problem it is.

[kinyoklion]

I think I see your point though, that hook authors have to make a choice here, and it's somewhat disconnected from whatever instances the application author is choosing to instantiate. I will think about that some more. I'm not sure how much of a problem it is.

It probably isn't too much of a concern. I can imagine a provider making a context, keeping that in the provider, and then always returning it in the before hook. As long as they don't ever change the content it would likely be fine. If you make a context within the hook and then don't re-use it, then it isn't really a problem.

[justinabrahms]

Yep. I'm all for this idea. We should be able to swap this in the future in a backwards compatible way, provided that the underlying context is of the EvaluationContext type.