feat(ofrep): AOT compatibility#651
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Native AOT support for the OFREP provider by implementing typed evaluation methods and transitioning to source-generated JSON serialization. It also adds a dedicated AOT compatibility test project. Feedback focuses on resolving potential compilation errors related to dictionary type mismatches with the OpenFeature SDK and the use of .NET 5+ specific HttpContent methods in multi-targeted code. Additionally, there are recommendations to refactor duplicated validation and response mapping logic to improve maintainability.
|
@aldy505 Could you please sign-off your commits? You can follow these instructions:
|
Closes open-feature#600 Provide AOT-compatible path for OFREP, maintains backward compatibility for users with dynamic code Signed-off-by: Reinaldy Rafli <github@reinaldyrafli.com>
Signed-off-by: Reinaldy Rafli <github@reinaldyrafli.com>
Signed-off-by: Reinaldy Rafli <github@reinaldyrafli.com>
…standard compatibility Signed-off-by: Reinaldy Rafli <github@reinaldyrafli.com>
aldy505
force-pushed
the
feat/aot-compatibility
branch
from
74a698c to
f7f84de
Compare
askpt
left a comment
askpt
left a comment
There was a problem hiding this comment.
I saw that you have a lot of "backward compatibility" in the OfrepClient. This client is not supposed to be used by the external consumers. I would encourage to remove the backwards compatibility and use only the AOT path.
Signed-off-by: Reinaldy Rafli <github@reinaldyrafli.com>
aldy505
force-pushed
the
feat/aot-compatibility
branch
from
05fd71d to
4019d56
Compare
Signed-off-by: Reinaldy Rafli <github@reinaldyrafli.com>
Signed-off-by: Reinaldy Rafli <github@reinaldyrafli.com>
|
@askpt Applied everything other than moving to single file app. |
askpt
left a comment
askpt
left a comment
There was a problem hiding this comment.
Added a couple of comments. Thanks for making the changes.
The single file can wait.
Signed-off-by: Reinaldy Rafli <github@reinaldyrafli.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Native AOT compatibility to ChangesOFREP AOT Compatibility
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/OpenFeature.Providers.Ofrep/Client/OfrepClient.cs (1)
388-423:⚠️ Potential issue | 🔴 CriticalNon-primitive
EvaluateFlag<T>values are now silently dropped.
DeserializeResponseValue<T>only supportsbool/string/int/double/JsonElement; every other type returnsdefaultValueinstead of attempting deserialization. The testEvaluateFlag_WithObjectType_ShouldReturnObjectValuedemonstrates this regression—it expects object deserialization but the current implementation will return the default value, causing the assertion at line 654 (Assert.Equal(expectedValue.Name, result.Value.Name)) to fail.Suggested fix
private static T DeserializeResponseValue<T>(JsonElement valueElement, T defaultValue) { if (typeof(T) == typeof(JsonElement?)) { JsonElement? nullableElement = valueElement; return (T)(object)nullableElement; } - return defaultValue; + // Keep AOT-safe behavior, but preserve non-AOT backward compatibility. +#if NET8_0_OR_GREATER + if (!System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported) + { + return defaultValue; + } +#endif + var deserialized = JsonSerializer.Deserialize<T>(valueElement.GetRawText()); + return deserialized is null ? defaultValue : deserialized; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OpenFeature.Providers.Ofrep/Client/OfrepClient.cs` around lines 388 - 423, The DeserializeResponseValue<T> method only handles specific primitive types (bool, string, int, double, JsonElement) and silently returns the default value for any other type, causing the EvaluateFlag_WithObjectType_ShouldReturnObjectValue test to fail. Add a fallback case before the final return statement that attempts to deserialize unsupported types using JsonSerializer.Deserialize, passing the valueElement and defaultValue appropriately to handle object deserialization instead of silently dropping non-primitive values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/aot-compatibility.yml:
- Around line 48-52: The Checkout step using actions/checkout is currently
persisting git credentials in the local git config, which increases the risk of
token exposure in later workflow steps or artifacts. Add `persist-credentials:
false` to the `with` section of the Checkout action (after `fetch-depth` and
`submodules`) to disable credential persistence and reduce the security risk.
- Around line 74-76: The dotnet publish command in the AOT compatibility
workflow uses target framework net10.0, but the
OpenFeature.Providers.Ofrep.AotCompatibility test project targets net8.0. Change
the -f parameter value from net10.0 to net8.0 in the publish step to match the
actual target framework of the AOT compatibility test project, which will
resolve the publish failure on the AOT validation path.
- Around line 67-69: The Test AOT compatibility project build step specifies an
incorrect project path. Update the dotnet build command in the run step to point
to the correct AOT compatibility project path
test/OpenFeature.Providers.Ofrep.AotCompatibility/OpenFeature.Providers.Ofrep.AotCompatibility.csproj
instead of the current
test/OpenFeature.AotCompatibility/OpenFeature.AotCompatibility.csproj. This
ensures the build step targets the actual AOT compatibility test project that
was added.
---
Duplicate comments:
In `@src/OpenFeature.Providers.Ofrep/Client/OfrepClient.cs`:
- Around line 388-423: The DeserializeResponseValue<T> method only handles
specific primitive types (bool, string, int, double, JsonElement) and silently
returns the default value for any other type, causing the
EvaluateFlag_WithObjectType_ShouldReturnObjectValue test to fail. Add a fallback
case before the final return statement that attempts to deserialize unsupported
types using JsonSerializer.Deserialize, passing the valueElement and
defaultValue appropriately to handle object deserialization instead of silently
dropping non-primitive values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 54089c7f-bd5e-4bd8-9b82-84384e738a93
📒 Files selected for processing (12)
.github/workflows/aot-compatibility.ymlDotnetSdkContrib.slnxsrc/OpenFeature.Providers.Ofrep/Client/OfrepClient.cssrc/OpenFeature.Providers.Ofrep/Models/OfrepRequest.cssrc/OpenFeature.Providers.Ofrep/OfrepProvider.cssrc/OpenFeature.Providers.Ofrep/OpenFeature.Providers.Ofrep.csprojsrc/OpenFeature.Providers.Ofrep/Serialization/OfrepJsonSerializerContext.cstest/OpenFeature.Providers.Ofrep.AotCompatibility/Directory.Build.propstest/OpenFeature.Providers.Ofrep.AotCompatibility/OpenFeature.Providers.Ofrep.AotCompatibility.csprojtest/OpenFeature.Providers.Ofrep.AotCompatibility/Program.cstest/OpenFeature.Providers.Ofrep.Test/Client/OfrepClientTest.cstest/OpenFeature.Providers.Ofrep.Test/OfrepProviderTest.cs
Signed-off-by: Reinaldy Rafli <github@reinaldyrafli.com>
|
@askpt anything else to help move this forward? |
The GH Actions are failing at the moment. For the AOT Compatibility, from a quick look it seems it is missing a GH_TOKEN permission to access the packages in the GH Artifactory. For the format I think it is just do |
…e registry Signed-off-by: Reinaldy Rafli <github@reinaldyrafli.com>
I'm not sure with the current solution, I only follow this guide: https://docs.github.com/en/packages/working-with-a-github-packages-registry/working-with-the-nuget-registry#authenticating-in-a-github-actions-workflow -- let's see how it goes. |
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/aot-compatibility.yml (1)
73-79: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPublish target framework still mismatched with the AOT project.
Line 77 publishes
-f net9.0, but per theOpenFeature.Providers.Ofrep.AotCompatibility.csprojcontract (test/OpenFeature.Providers.Ofrep.AotCompatibility/OpenFeature.Providers.Ofrep.AotCompatibility.csproj), the project's<TargetFramework>isnet8.0. This is the same class of issue flagged in a prior review round (previouslynet10.0, nownet9.0), still not aligned with the actual project TFM — the publish step will fail.🐛 Proposed fix
dotnet publish test/OpenFeature.Providers.Ofrep.AotCompatibility/OpenFeature.Providers.Ofrep.AotCompatibility.csproj ` - -f net9.0 ` + -f net8.0 ` -r ${{ matrix.runtime }} ` -o ./aot-output🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/aot-compatibility.yml around lines 73 - 79, The AOT publish step is still using the wrong target framework in the workflow. Update the `dotnet publish` command in the `Publish AOT compatibility test (cross-platform)` step to match the `OpenFeature.Providers.Ofrep.AotCompatibility` project’s actual `TargetFramework` (`net8.0`) instead of `net9.0`, so the `publish` command and the project contract stay aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/workflows/aot-compatibility.yml:
- Around line 73-79: The AOT publish step is still using the wrong target
framework in the workflow. Update the `dotnet publish` command in the `Publish
AOT compatibility test (cross-platform)` step to match the
`OpenFeature.Providers.Ofrep.AotCompatibility` project’s actual
`TargetFramework` (`net8.0`) instead of `net9.0`, so the `publish` command and
the project contract stay aligned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 88571187-53b7-4c70-bd6e-731acddaa3e6
📒 Files selected for processing (1)
.github/workflows/aot-compatibility.yml
askpt
force-pushed
the
feat/aot-compatibility
branch
from
5455cf3 to
a60336d
Compare
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Managed to get the restore step passing but it is still failing with other errors. |
|
Hmm, this is harder than I thought. I'll revisit this tomorrow. |
3 participants
This PR
Provide AOT-compatible path for OFREP, maintains backward compatibility for users with dynamic code
Related Issues
Closes #600
Notes
Follow-up Tasks
How to test