Skip to content

Conversation

@toddbaert
Copy link
Member

@toddbaert toddbaert commented Apr 17, 2025

Migrates from JsonLogic.Net to JsonLogic and from NewtonSoft Json to System.Text.Json. This also addresses a CVE in our NewtonSoft transitive dep.

I've tried to keep changes minimal, but there's obviously API changes included in both the migrations above. There's certainly some additional optimizations possible but I'd like to do them separately.

Let me know if you see anything odd in terms of conversions or potential performance issues. Obviously all the e2e and unit tests pass.

Resolves: #316

@toddbaert toddbaert requested review from a team as code owners April 17, 2025 19:35
@github-actions github-actions bot requested a review from bacherfl April 17, 2025 19:35
@toddbaert toddbaert force-pushed the fix/json-logic-migration branch from 8a02428 to 8d8c05c Compare April 17, 2025 19:36
@toddbaert toddbaert marked this pull request as draft April 17, 2025 19:37
@toddbaert toddbaert changed the title wip fix: migrate to System.Text.Json and JsonLogic Apr 17, 2025
@toddbaert toddbaert force-pushed the fix/json-logic-migration branch 2 times, most recently from af34ce1 to f1c900e Compare April 21, 2025 18:39
@toddbaert toddbaert marked this pull request as ready for review April 21, 2025 18:40
@toddbaert toddbaert force-pushed the fix/json-logic-migration branch from f1c900e to 8ad2105 Compare April 21, 2025 18:45
@@ -0,0 +1,68 @@
using System;
Copy link
Member Author

Choose a reason for hiding this comment

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

This file was renamed, but it was changed enough that git sees it as a delete/add.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates the Flagd provider from NewtonSoft.Json and JsonLogic.Net to System.Text.Json and Json.Logic while addressing a CVE in a transitive NewtonSoft dependency. Key changes include refactoring JSON parsing and rule evaluation throughout tests and production code, updating custom evaluators to use the new libraries, and modifying test assertion logic to align with the updated JSON API.

Reviewed Changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/OpenFeature.Contrib.Providers.Flagd.Test/Utils.cs Updates JSON string formatting and reference syntax for compatibility with System.Text.Json.
test/OpenFeature.Contrib.Providers.Flagd.Test/StringEvaluatorTest.cs Refactors tests to use JsonLogic.Apply and RuleRegistry.AddRule.
test/OpenFeature.Contrib.Providers.Flagd.Test/SemVerEvaluatorTest.cs Adjusts semver evaluations to work with System.Text.Json and the new semver rule.
test/OpenFeature.Contrib.Providers.Flagd.Test/JsonEvaluatorTest.cs Updates metadata assertions by switching from GetInt to GetDouble for numeric values.
test/OpenFeature.Contrib.Providers.Flagd.Test/FractionalEvaluatorTest.cs Migrates fractional evaluator tests to leverage System.Text.Json and JsonLogic.
test/OpenFeature.Contrib.Providers.Flagd.E2e.Test/Steps/FlagdStepDefinitionBase.cs Comments out the test-skip condition to allow for E2E tests to run, impacting test environment control.
src/OpenFeature.Contrib.Providers.Flagd/Resolver/InProcess/JsonEvaluator.cs Replaces Newtonsoft.Json with System.Text.Json, updating metadata extraction and rule evaluation logic.
src/OpenFeature.Contrib.Providers.Flagd/Resolver/InProcess/CustomEvaluators/StringRule.cs Introduces new implementations for string rules using the updated Json.Logic API.
src/OpenFeature.Contrib.Providers.Flagd/Resolver/InProcess/CustomEvaluators/SemVerRule.cs Updates semver evaluation to integrate with JsonLogic and the new JSON API.
src/OpenFeature.Contrib.Providers.Flagd/Resolver/InProcess/CustomEvaluators/FractionalRule.cs Refactors fractional flag calculations to use System.Text.Json and newer JsonLogic patterns.
src/OpenFeature.Contrib.Providers.Flagd/Resolver/InProcess/CustomEvaluators/FlagdProperties.cs Adjusts extraction of flag properties from the EvaluationContext via System.Text.Json.
Files not reviewed (1)
  • src/OpenFeature.Contrib.Providers.Flagd/OpenFeature.Contrib.Providers.Flagd.csproj: Language not supported
Comments suppressed due to low confidence (1)

test/OpenFeature.Contrib.Providers.Flagd.Test/JsonEvaluatorTest.cs:346

  • The metadata integer value is now being retrieved as a double instead of an int; please confirm that this conversion is intentional and that downstream code correctly handles numeric values as doubles.
Assert.Equal(2, result.FlagMetadata.GetDouble("integer"));

Copy link
Member

@askpt askpt left a comment

Choose a reason for hiding this comment

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

Added a couple of comments. In general looks good to me, but I would like to not have some classes public and seal them.

@open-feature open-feature deleted a comment from Copilot AI Apr 21, 2025
Assert.NotNull(result.FlagMetadata);
Assert.Equal("1.0.2", result.FlagMetadata.GetString("string"));
Assert.Equal(2, result.FlagMetadata.GetInt("integer"));
Assert.Equal(2, result.FlagMetadata.GetDouble("integer"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we able to resolve integers?

Copy link
Member Author

@toddbaert toddbaert Apr 21, 2025

Choose a reason for hiding this comment

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

Sort of - this is one thing I meant to make a note of, and the single behavioral change in this PR.

I think, as a solution, we may need to make some improvements to our SDK itself to allow us to interpret numeric metadata as either doubles OR integers, as desired. I think with what we have currently, there's no way to do this. This shows up here because of the way the new JSON API works - it only specifies whether the JSON node is numeric or not (obviously JSON doesn't distinguish between ints and decimals).

I think we need to add some features here to allow a numeric value to be interpreted either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @askpt

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make an issue for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

:shipit:

Thank you!

.NET/NuGet offers great settings to prevent similar issues in the future. You can add

  <PropertyGroup>
    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
    <NuGetAudit>true</NuGetAudit>
    <NuGetAuditMode>all</NuGetAuditMode>
    <NuGetAuditLevel>low</NuGetAuditLevel>
  </PropertyGroup>

to your global props file.

This was referenced Dec 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[flagd] Brings vulnerable version of Newtonsfot.Json

7 participants