fix: improve targetingKey handling in the context

[Kavindu-Dodan]

This PR

Fixes #801.

What's changed?

I have changed how we store the targetingKey internally (in both mutable and immutable contexts). Instead of storing in a field, we now store targetingKey as a property of the delegated Structure implemementation.

Why

Both mutable and immutable contexts use a Structure implementation instance and delegate operations onto this instance. For example, asObjectMap and asMap() are exposed through this delegation. With this change, we keep the same delegation and expose targetingKey from the above-mentioned attribute exporters.

Impact

There are no behaviour change other than for avoiding storing of empty string ("") for targetingKey.

[Kavindu-Dodan]

This is the only major change - we do not accept empty value or spaces for targeting key. Eaerlier this was handled in the merge stage where we filtered for null or empty values.

[toddbaert]

Makes sense.

[codecov]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (ef56006) 94.89% compared to head (b9f4469) 95.39%.

❗ Current head b9f4469 differs from pull request most recent head 1c18a9e. Consider uploading reports for the commit 1c18a9e to get more accurate results

Files	Patch %	Lines
.../main/java/dev/openfeature/sdk/MutableContext.java	85.71%	0 Missing and 2 partials ⚠️
...ain/java/dev/openfeature/sdk/ImmutableContext.java	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #805      +/-   ##
============================================
+ Coverage     94.89%   95.39%   +0.49%     
- Complexity      365      368       +3     
============================================
  Files            34       34              
  Lines           862      847      -15     
  Branches         53       50       -3     
============================================
- Hits            818      808      -10     
+ Misses           23       21       -2     
+ Partials         21       18       -3     

Flag	Coverage Δ
unittests	95.39% <86.95%> (+0.49%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[toddbaert]

@Kavindu-Dodan This seems like a good idea to me. We recently had a bug in flagd-java due to this. I think the risk is low but I'm interested in what others think.

Do you think we should do the same thing in dotnet? cc @kinyoklion @austindrenski

[kinyoklion]

@Kavindu-Dodan This seems like a good idea to me. We recently had a bug in flagd-java due to this. I think the risk is low but I'm interested in what others think.

Do you think we should do the same thing in dotnet? cc @kinyoklion @austindrenski

I like this idea. Prior to it being formalized into the API of the context itself I had been targeting the "targetingKey" based on a string constant.

A snippet from the documentation for the LaunchDarkly dotnet provider.

var evaluationContext = EvaluationContext.Builder()
  .Set("kind", "organization")
  .Set("targetingKey", "my-org-key") // Could also use "key" instead of "targetingKey".
  .Build();

By storing it in an attribute it would theoretically maintain compatibility with those assumptions.

[lukas-reining]

Makes sense and the change looks good to me!

[toddbaert]

@Kavindu-Dodan would you mind creating a similar .NET issue if you agree?

[Kavindu-Dodan]

@Kavindu-Dodan would you mind creating a similar .NET issue if you agree?

Created a .Net issue - open-feature/dotnet-sdk#235

I will merge this as we have an agreement on the change :)

[sonarcloud]

Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud