BREAKING CHANGE: Add generic structure type, use context/getObject

[toddbaert]

As described in #50, I think that expected a T back from object flag resolvers is not sustainable. I believe it pushes too much of a burden on providers to instantiate a new T (whatever it may be) and map the attributes received from the backend into it.

Instead, I've added a more generic Structure (language used in the OpenFeature spec) with methods similar to the existing EvaluationContext. I've also made the EvaluationContext inherit from this to reduce duplication (the EvaluationContext is also referred to as a "structure" in the spec.

After this change, providers will create a new Structure and add the corresponding fields from the json/xml/protobuf they are concerned with.

In summary:

• Adds Structure type which represents JSON data
• EvaluationContext inherits encapsultes this using lombok's @Delegate.
• Consolidated addIntegerAttribute, addStringAttribute, etc, in EvaluationContext to a single overloaded add method.
• Removed ability to get multiple attributes of a single type from EvaluationContext (I'm not convinced these are that valuable, but I'll take feedback).
• introduces a Value wrapper class that houses one of: Structure, List, Boolean, etc (representations of all the possible JSON node values).

[beeme1mr]

FYI @rgrassian-split. I would like to get your thoughts on this. How would this change impact the provider you're working on?

[rgrassian-split]

FYI @rgrassian-split. I would like to get your thoughts on this. How would this change impact the provider you're working on?

@beeme1mr Hey! This is a fantastic change, I was actually running into some issues with the T implementation so I'm happy to see this. It shouldn't be too crazy to change our provider to support this.

[toddbaert]

@rgrassian-split it would be good if you could put some feedback in #50. Really glad to have your input.

[rgrassian-split]

@toddbaert Just did, that is actually the bigger deal for us that I very much support switching it from the current approach

[justinabrahms]

Lots of formatting churn makes the testing stuff (especially spec definitions) a bit hard to read.

[codecov-commenter]

Codecov Report

Merging #51 (6eed1a3) into main (465d113) will decrease coverage by 2.21%.
The diff coverage is 80.90%.

@@             Coverage Diff              @@
##               main      #51      +/-   ##
============================================
- Coverage     93.01%   90.79%   -2.22%     
- Complexity      124      146      +22     
============================================
  Files            17       19       +2     
  Lines           272      326      +54     
  Branches         12        7       -5     
============================================
+ Hits            253      296      +43     
- Misses           11       20       +9     
- Partials          8       10       +2     

Flag	Coverage Δ
unittests	90.79% <80.90%> (-2.22%)	⬇️

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

Impacted Files	Coverage Δ
src/main/java/dev/openfeature/javasdk/Value.java	77.96% <77.96%> (ø)
...c/main/java/dev/openfeature/javasdk/Structure.java	82.60% <82.60%> (ø)
...ava/dev/openfeature/javasdk/EvaluationContext.java	87.87% <84.00%> (+4.27%)	⬆️
...ava/dev/openfeature/javasdk/OpenFeatureClient.java	97.53% <100.00%> (ø)

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

[toddbaert]

@justinabrahms I think I've cleaned up a lot of the whitespace changes. A few are possibly improvements (missing newlines in some places, etc). I've left those.

I also added array support in Structure.

I'm going to test this locally in my provider and see if it makes things nicer. Let me know what you think.

I had to rebase/squash to fix the whitespace issue, so previous commits are gone... I hope that's not a problem 😬

[toddbaert]

@justinabrahms I made some more changes. Our abstraction was leaky because any node in a JSON structure can be a JSON structure, an array or a primitive. That wasn't reflected the implementation leading to some issues, particularly with type safety around arrays.

What I've done in this commit, is introduce a Value wrapper class that houses one of: Structure, List<Value>, Boolean, etc (representations of all the possible JSON node values). The actual value can be retrieved like value.asBoolean() or value.asStructure()... etc, which return null if the inner value is not of the supposed type. I believe any JSON structure can be represented safely this way.

This is similar to how java-grpc represents their profobuf, which is similar in structure to JSON, and something like a simplified version of LDValue.

I've maintained most of the previous work. EvaluationContext still encapsulates a Structure, etc.

I think this is something along the lines of what @kinyoklion was recommending as well.

[toddbaert]

Lists must be of type Value.

[justinabrahms]

I think I need to play around with constructing a few of these objects to figure out what the API feels like.

[toddbaert]

I think I need to play around with constructing a few of these objects to figure out what the API feels like.

OK @justinabrahms, I'll leave it up to you to merge it then (or message with more feedback). @kinyoklion has also approved. I think we'll need to do something similar in dotnet cc @benjiro

[s-sen]

Are we OK with a shallow copy of the values? The application developer will use this to obtain all the values hence a shallow copy is OK in that scenario. But will this structure also be used during building and mutating the EvaluationContext?

[toddbaert]

I think this is a good question...

I suspect that for mutating, users will be using the merge and add functions in the EvaluationContext. I don't have any strong opinions about if this should be a shallow copy, deep copy, or just expose the inner map (though I think in most cases, that's a bad choice). I guess whatever we choose it should be well-documented.

Does anyone have a strong argument for a particular direction on this?

@justinabrahms @kinyoklion @benjiro

[justinabrahms]

#53 Moved this discussion here.