Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature request]: Add AddStrategy API with no options #1985

Closed
eerhardt opened this issue Feb 22, 2024 · 3 comments · Fixed by #2068
Closed

[Feature request]: Add AddStrategy API with no options #1985

eerhardt opened this issue Feb 22, 2024 · 3 comments · Fixed by #2068

Comments

@eerhardt
Copy link

Is your feature request related to a specific problem? Or an existing feature?

The AddStrategy extension methods take a ResilienceStrategyOptions options argument, which gets validated using DataAnnotations. This makes the method incompatible with trimming and native AOT, and it is annotated as such:

[RequiresUnreferencedCode(Constants.OptionsValidation)]
public static TBuilder AddStrategy<TBuilder>(this TBuilder builder, Func<StrategyBuilderContext, ResilienceStrategy> factory, ResilienceStrategyOptions options)
where TBuilder : ResiliencePipelineBuilderBase

[RequiresUnreferencedCode(Constants.OptionsValidation)]
public static ResiliencePipelineBuilder AddStrategy(
this ResiliencePipelineBuilder builder, Func<StrategyBuilderContext, ResilienceStrategy<object>> factory,
ResilienceStrategyOptions options)

[RequiresUnreferencedCode(Constants.OptionsValidation)]
public static ResiliencePipelineBuilder<TResult> AddStrategy<TResult>(
this ResiliencePipelineBuilder<TResult> builder, Func<StrategyBuilderContext, ResilienceStrategy<TResult>> factory,
ResilienceStrategyOptions options)

Callers who want to use this API in a trim compatible way need to define an empty class derived from ResilienceStrategyOptions and suppress the warning from calling this API.

Describe the solution you'd like

We should add overloads to these AddStrategy methods that are trim compatible, and don't take an options argument. Similar to the AddPipeline methods.

 [RequiresUnreferencedCode(Constants.OptionsValidation)] 
 public static TBuilder AddStrategy<TBuilder>(this TBuilder builder, Func<StrategyBuilderContext, ResilienceStrategy> factory, ResilienceStrategyOptions options) 
     where TBuilder : ResiliencePipelineBuilderBase  {}

+public static TBuilder AddStrategy<TBuilder>(this TBuilder builder, Func<StrategyBuilderContext, ResilienceStrategy> factory) 
+    where TBuilder : ResiliencePipelineBuilderBase  {}

 [RequiresUnreferencedCode(Constants.OptionsValidation)] 
 public static ResiliencePipelineBuilder AddStrategy( 
     this ResiliencePipelineBuilder builder, Func<StrategyBuilderContext, ResilienceStrategy<object>> factory, 
     ResilienceStrategyOptions options) {}

+public static ResiliencePipelineBuilder AddStrategy(
+    this ResiliencePipelineBuilder builder, Func<StrategyBuilderContext, ResilienceStrategy<object>> factory) {}

 [RequiresUnreferencedCode(Constants.OptionsValidation)] 
 public static ResiliencePipelineBuilder<TResult> AddStrategy<TResult>( 
     this ResiliencePipelineBuilder<TResult> builder, Func<StrategyBuilderContext, ResilienceStrategy<TResult>> factory, 
     ResilienceStrategyOptions options) {}

+ public static ResiliencePipelineBuilder<TResult> AddStrategy<TResult>(
+    this ResiliencePipelineBuilder<TResult> builder, Func<StrategyBuilderContext, ResilienceStrategy<TResult>> factory) {}

Additional context

This is being used by Microsoft.Extensions.Http.Resilience here:
dotnet/extensions#4962 (comment)

Adding this API would make the caller simpler. It wouldn't need to declare an empty class, and suppress the trim warning.

@martincostello
Copy link
Member

Seems reasonable to me, plus would make the API easier to use even without wanting to use AoT.

@chrbkr
Copy link

chrbkr commented Mar 27, 2024

This seems to be actually a bug, at least in the documentation. The options parameter in the ResiliencePipelineBuilderExtensions.AddStrategy methods is described like this:

The options associated with the strategy. If none are provided the default instance of ResilienceStrategyOptions is created.

So one would expect the parameter already to be optional. There is the ´EmptyOptions´ class, but it's internal, so I need to copy it if I don't need any options for my strategy.

@martintmk
Copy link
Contributor

This seems to be actually a bug, at least in the documentation. The options parameter in the ResiliencePipelineBuilderExtensions.AddStrategy methods is described like this:

The options associated with the strategy. If none are provided the default instance of ResilienceStrategyOptions is created.

So one would expect the parameter already to be optional. There is the ´EmptyOptions´ class, but it's internal, so I need to copy it if I don't need any options for my strategy.

These are incorrect docs, thanks for noticing. Both new API and fixed docs are addressed in #2068.

cc @eerhardt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants