Evaluation context `merge` implementations should be recursive

[toddbaert]

If there is a property on this called myKey, and it has subkeys, and there's also a property called myKey on overridingContext, then the subkeys are lost, instead of merged.

This is un-intuitive and should be correct.

see: https://github.com/open-feature/java-sdk/pull/210/files#r1071367173

[javicv]

If both myKey are Structure they can be deep merged, but if myKey is an Structure in this but it isn't in overridingContext, what should be done? Should we simply override it with the myKey in overridingContext?

[toddbaert]

@javicv yes, I think overridingContext should always win in cases of conflict, but I think we should attempt a recursive object merge first. That's just my opinion though. It's beyond the scope of the OpenFeature spec, so it's really just an implementation detail of this SDK.

What do you think?

Also interested in opinions of @justinabrahms @Kavindu-Dodan

[beeme1mr]

It probably won't hurt to clarify in the spec. That would ensure consistently across SDKs and likely lead to a specific test for merging context.

[toddbaert]

It probably won't hurt to clarify in the spec. That would ensure consistently across SDKs and likely lead to a specific test for merging context.

My feelings about specifying things are more conservative. "Won't hurt" isn't a good enough justification from my perspective. Different languages might have different patterns and utilities for merging objects and I worry that we push more work onto SDKs that might not have huge ROI, or even deviate from language idioms.

To put it another way, I think it's usually a good idea to "default to unspecified" and only specify if we really think it's needed.

[thiyagu06]

@toddbaert, How do we handle for the below scenario?

// current context
{
 "mykey":  [1,2,3,4]
}

// overridding context
{
 "mykey":  [1,2,3,5,6]
}

what should be the resulting context? Is it [1,2,3,4,5,6] or [1,2,3,5,6]?

[Kavindu-Dodan]

@toddbaert, How do we handle for the below scenario?

// current context
{
 "mykey":  [1,2,3,4]
}

// overridding context
{
 "mykey":  [1,2,3,5,6]
}

what should be the resulting context? Is it [1,2,3,4,5,6] or [1,2,3,5,6]?

It should be [1,2,3,4,5,6] where values 1,2,3,5,6 are coming from the overriding structure

[javicv]

It should be [1,2,3,4,5,6] where values 1,2,3,5,6 are coming from the overriding structure

If it's a structure, yes, but for lists, this is not true with current implementation.

[toddbaert]

Personally, I don't think lists should be "merged" and any way. That behavior would surprise me quite a bit.