feat(ofrep): AOT compatibility#651
Conversation
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>
74a698c to
f7f84de
Compare
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.
| - name: OFREP AOT compatibility check | ||
| if: matrix.os == 'ubuntu-latest' | ||
| run: dotnet publish test/OpenFeature.Providers.Ofrep.AotCompatibility/OpenFeature.Providers.Ofrep.AotCompatibility.csproj -c Release -r linux-x64 --self-contained true -p:TreatWarningsAsErrors=true |
There was a problem hiding this comment.
Have a look at: https://github.com/open-feature/dotnet-sdk/blob/main/.github/workflows/aot-compatibility.yml
We would prefer to have compatibility checks to all releases and machine types.
There was a problem hiding this comment.
This is done, though I'm not confident on enabling it for any other provider than Ofrep.
There was a problem hiding this comment.
Can you consider moving this file to a single file app?
There was a problem hiding this comment.
I don't have much experience on this. I thought you meant .csx file, then I found this post: https://learn.microsoft.com/en-us/dotnet/core/sdk/file-based-apps
Might take a while for this.
Signed-off-by: Reinaldy Rafli <github@reinaldyrafli.com>
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
There was a problem hiding this comment.
Added a couple of comments. Thanks for making the changes.
The single file can wait.
| EvaluationContext? context, CancellationToken cancellationToken = default) | ||
| { | ||
| #if NET8_0_OR_GREATER | ||
| #if NET8_0_OR_GREATER |
There was a problem hiding this comment.
| #if NET8_0_OR_GREATER | |
| #if NET8_0_OR_GREATER |
|
|
||
| var response = await this._client.EvaluateFlag(flagKey, defaultValue, | ||
| context, cancellationToken).ConfigureAwait(false); | ||
| var response = await evaluate(flagKey, defaultValue, context, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Why are you passing a function here instead of calling directly the method?
| /// <remarks> | ||
| /// This generic method is provided for backward compatibility. | ||
| /// For Native AOT scenarios, use the typed methods (EvaluateBooleanFlag, EvaluateStringFlag, etc.) instead. | ||
| /// </remarks> |
| /// <summary> | ||
| /// Evaluates a flag value using the OFREP API. This generic method is provided for backward compatibility. | ||
| /// For Native AOT scenarios, use the typed methods (EvaluateBooleanFlag, EvaluateStringFlag, etc.) instead. | ||
| /// </summary> | ||
| /// <typeparam name="T">The type of the flag value.</typeparam> | ||
| /// <param name="flagKey">The key of the flag to evaluate.</param> | ||
| /// <param name="defaultValue">The default value to return if evaluation fails.</param> | ||
| /// <param name="context">The evaluation context.</param> | ||
| /// <param name="cancellationToken">A token to cancel the operation.</param> | ||
| /// <returns>The evaluated flag response.</returns> | ||
| /// <remarks> | ||
| /// This method delegates to the AOT-safe typed implementation for OFREP-supported value types. | ||
| /// Other generic types use the legacy reflection-based JSON path and are not AOT-safe. | ||
| /// </remarks> |
There was a problem hiding this comment.
This seems to be now fully native AOT compliant. This needs to be removed.
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