feat: report prerequisite relations in AllFlagState #19
Conversation
| @@ -295,7 +332,10 @@ public override FeatureFlagsState Read(ref Utf8JsonReader reader, Type typeToCon | |||
| for (var flagsObj = RequireObject(ref reader); flagsObj.Next(ref reader);) | |||
| { | |||
| var subKey = flagsObj.Name; | |||
| var flag = flags.ContainsKey(subKey) ? flags[subKey] : new FlagState(); | |||
There was a problem hiding this comment.
Slightly awkward: have to new the prerequisite list here in the deserializer, and also on line 375. I'd rather just have this in the constructor so it's never a question, but looks like structs in our language version can't define constructors.
Another option is to make a null list be a possibility. Logically that'd be fine, but.. I'd rather the consumers never need to question if it's null or not. I could be convinced otherwise.
There was a problem hiding this comment.
I agree with the constructor idea or something similar. Faced a similar issue in Java wrt to nullity, but luckily the array constructor with capacity 0 uses a static instance under the hood for performance.
There was a problem hiding this comment.
Theoretically we should have struct constructors, but not parameter-less constructors. Ideally we would be creating a fully populated readonly struct instead of poking stuff into it. (containing an immutable list of prerequisites, or null)
There was a problem hiding this comment.
With the way we are deserializing JSON, we'd need to have some kind of builder pattern (or temp variables) to achieve that (creating the readonly struct in one go.)
There was a problem hiding this comment.
If the temp variables are references, then they aren't really temps, the struct holds a reference. Though it maybe it problematic to adjust. It isn't at an assignment level though. Assign to a shared list and then assign to the populated list.
| @@ -371,8 +372,7 @@ public FeatureFlagsState AllFlagsState(Context context, params FlagsStateOption[ | |||
|
|
|||
| var builder = new FeatureFlagsStateBuilder(options); | |||
There was a problem hiding this comment.
These weren't used at all in this function. They are used internal to the FeatureFlagsStateBuilder, presumably they didn't get cleaned up when that was introduced.
|
|
||
| var directPrerequisites = result.PrerequisiteEvals.Where( | ||
| e => e.FlagKey == flag.Key) | ||
| .Select(p => p.PrerequisiteFlag.Key).ToList(); |
There was a problem hiding this comment.
Converting to a list here, rather than leaving as an IEnumerable, because the intent is that order is preserved.
| @@ -295,7 +332,10 @@ public override FeatureFlagsState Read(ref Utf8JsonReader reader, Type typeToCon | |||
| for (var flagsObj = RequireObject(ref reader); flagsObj.Next(ref reader);) | |||
| { | |||
| var subKey = flagsObj.Name; | |||
| var flag = flags.ContainsKey(subKey) ? flags[subKey] : new FlagState(); | |||
There was a problem hiding this comment.
I agree with the constructor idea or something similar. Faced a similar issue in Java wrt to nullity, but luckily the array constructor with capacity 0 uses a static instance under the hood for performance.
| }; | ||
| _flags[flagKey] = flag; | ||
| return this; | ||
| } | ||
| } | ||
|
|
||
| internal struct FlagState | ||
| internal struct FlagState : IEquatable<FlagState> |
There was a problem hiding this comment.
Non-nullable prerequisites feel a bit strange to me.
There was a problem hiding this comment.
Fair. Is that from a "what a library consumer would expect from a iterate-able thing in C#" kind of way, or a "how structs work" way?
There was a problem hiding this comment.
So, it feels strange in the language to me. We have null-state analysis, so we should be able to safely mark things null-able and use them. https://learn.microsoft.com/en-us/dotnet/csharp/nullable-references#null-state-analysis
From a struct perspective, ideally structs only contain other value types, but that isn't always possible. Next is for structs to only contain immutable reference types in addition to value types. Least favorable is this mix, but also not always avoidable. A struct with mostly not references, with value semantics, is better than a class here probably. But it doesn't behave like a struct. In that if you alias it and mutate the list, then both lists mutate.
There was a problem hiding this comment.
If we make an immutable list, and then assign the entire list, then it at least makes it more toward the normal side. It could also be exposed as an IReadOnlyList<T>.
There was a problem hiding this comment.
I don't think this is blocking though, as it isn't part of the external API.
…g merging behavior
🤖 I have created a release *beep* *boop* --- ## [8.6.0](LaunchDarkly.ServerSdk-v8.5.2...LaunchDarkly.ServerSdk-v8.6.0) (2024-10-24) ### Features * report prerequisite relations in AllFlagState ([#19](#19)) ([43da95c](43da95c)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR updates
AllFlagsStateto track prerequisite evaluations.This didn't require modifying the Evaluator interface, as it already returns prerequisites as part of the
EvalResult.When the returned
FlagStateis marshaled to JSON, it will now contain the prerequisite info for each flag.