fix: Validate privateAttributes in evaluation context conversion - #70
kinyoklion wants to merge 2 commits into
Conversation
Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
@cursor review |
|
|
||
| if (privateAttributes.Length != items.Count) | ||
| { | ||
| _log.Error("'privateAttributes' must be an array of only string values"); |
There was a problem hiding this comment.
Should we mention that non-strings were dropped, while the others retained? Just to document the behavior more clearly for customers.
There was a problem hiding this comment.
Good call — the message now says so explicitly:
'privateAttributes' must be an array of only string values. The non-string values have been dropped and the remaining values have been applied.
Pushed in 4998d10. Note this makes the .NET wording diverge slightly from the Java/Python/Ruby/PHP/JS providers, which all log just the first sentence; happy to follow up with the same clarification in the others if you want them aligned.
Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
|
CI note: Root cause is the test project running collections in parallel while those initialization tests drive the process-global |
privateAttributeswas applied without any type validation, so invalid values were silently dropped instead of being reported, as required by the accepted OpenFeature provider behavior spec (OFPinlaunchdarkly/sdk-specs).Requirements
Related issues
None.
Describe the solution you've provided
privateAttributesnow logsThe attribute 'privateAttributes' must be of type array, matching hownameandanonymoustype errors are already reported in this converter'privateAttributes' must be an array of only string values, matching the Python, Ruby, and JavaScript providersDescribe alternatives you've considered
Rejecting the entire list when any entry is invalid was considered, but the other providers keep the valid entries, and dropping them would make more attributes public than the caller intended.
Additional context
Implementation details
ProcessValuecalledbuilder.Private(ldValue.AsList(LdValue.Convert.String).ToArray()). For a non-array valueAsListyields an empty list, soprivateAttributes: "myCustomAttribute"silently marked nothing private; non-string entries converted tonullattribute references. The newExtractPrivateAttributesfilters and reports instead. Null values are still ignored, consistent with the other built-in attributes.Testing:
dotnet test test/LaunchDarkly.OpenFeature.ServerProvider.Tests -f net8.0— 71 passed.dotnet build src/LaunchDarkly.OpenFeature.ServerProviderbuilds netstandard2.0, net471, and net6.0 with no warnings.Related but separate: #69 fixes key/targetingKey handling in the same converter.
Link to Devin session: https://app.devin.ai/sessions/9f0be899af7842e0b6a7bfc6229084f6
Open in Devin Desktop: https://app.devin.ai/desktop/session/9f0be899af7842e0b6a7bfc6229084f6?variant=devin
Requested by: @kinyoklion
Note
Overview
OpenFeature
privateAttributeson evaluation contexts is now validated when converting to LaunchDarklyContext, matching the OpenFeature provider behavior spec and other LaunchDarkly providers.EvalContextConverterroutesprivateAttributesthrough newExtractPrivateAttributesinstead of blindly callingAsList(LdValue.Convert.String). A non-array value logs the sameInvalidTypeMessagepattern used fornameandanonymous; null is ignored. Arrays keep only string elements—mixed arrays log an error, drop non-strings, and still apply the valid names so one bad entry does not discard the whole list.Tests cover wrong type (string instead of array), mixed string/non-string arrays, and empty arrays with no log noise.
Reviewed by Cursor Bugbot for commit 4998d10. Bugbot is set up for automated code reviews on this repo. Configure here.