Skip to content

fix: Validate privateAttributes in evaluation context conversion - #70

Open
kinyoklion wants to merge 2 commits into
mainfrom
devin/1788966481-of-dotnet-private-attrs
Open

kinyoklion wants to merge 2 commits into
mainfrom
devin/1788966481-of-dotnet-private-attrs

Conversation

@kinyoklion

@kinyoklion kinyoklion commented Sep 9, 2026 •

Copy link
Copy Markdown
Member

privateAttributes was applied without any type validation, so invalid values were silently dropped instead of being reported, as required by the accepted OpenFeature provider behavior spec (OFP in launchdarkly/sdk-specs).

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Related issues

None.

Describe the solution you've provided

  • A non-array privateAttributes now logs The attribute 'privateAttributes' must be of type array, matching how name and anonymous type errors are already reported in this converter
  • Non-string entries are omitted and log 'privateAttributes' must be an array of only string values, matching the Python, Ruby, and JavaScript providers
  • Valid string entries are still applied, so one bad entry no longer risks the whole list

Describe 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

ProcessValue called builder.Private(ldValue.AsList(LdValue.Convert.String).ToArray()). For a non-array value AsList yields an empty list, so privateAttributes: "myCustomAttribute" silently marked nothing private; non-string entries converted to null attribute references. The new ExtractPrivateAttributes filters 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.ServerProvider builds 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 privateAttributes on evaluation contexts is now validated when converting to LaunchDarkly Context, matching the OpenFeature provider behavior spec and other LaunchDarkly providers.

EvalContextConverter routes privateAttributes through new ExtractPrivateAttributes instead of blindly calling AsList(LdValue.Convert.String). A non-array value logs the same InvalidTypeMessage pattern used for name and anonymous; 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.

Co-Authored-By: rlamb@launchdarkly.com <4955475+kinyoklion@users.noreply.github.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration

Copy link
Copy Markdown
Contributor

@cursor review

@kinyoklion
kinyoklion marked this pull request as ready for review September 29, 2026 22:41
@kinyoklion
kinyoklion requested a review from a team as a code owner September 29, 2026 22:41

if (privateAttributes.Length != items.Count)
{
_log.Error("'privateAttributes' must be an array of only string values");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention that non-strings were dropped, while the others retained? Just to document the behavior more clearly for customers.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@devin-ai-integration

Copy link
Copy Markdown
Contributor

CI note: ci-build (macos-latest) failed on its net6.0 leg in ClientIntegrationTests.ItHandlesValidInitializationWhenClientIsReadyAfterADelay with Test execution timed out after 5000 milliseconds after 1 ms of test body time. The identical test passed on the net8.0 leg of the same job, this branch touches only EvalContextConverter.cs and its tests, and the same-area #69 was green on macOS.

Root cause is the test project running collections in parallel while those initialization tests drive the process-global Api.Instance with sleeps and timers. Fix is in #75; rebasing or merging that will clear this run.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants