V7 Generator/Revise dependency injection extension methods#1242
V7 Generator/Revise dependency injection extension methods#1242Kwok-he-Chu wants to merge 15 commits intomainfrom
Conversation
…th a ServiceLifeTime Handles both webhook handler and services
… to api.mustache HostConfiguration.mustache also no longer adds all services as a singleton, this is moved to ServiceCollectionExtension.mustache
…esulting in no event-triggers This allows developers to initialize the Service standalone, but gives the option to override certain behavior
…n moved to service-level
…uilder HostBuilder: Configures configuration Services: initializes the implementation of services
Allow the Options to be fetched through a Provider class, rather than injection it directly.
Fix comments JsonSerializerOptionsProvider with correct references to the classes
… easy initialization
Summary of ChangesHello @Kwok-he-Chu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the dependency injection patterns within the .NET Library, making service configuration more intuitive and less error-prone. By introducing an AdyenOptionsProvider and new IHostBuilder extension methods, it automates the construction of HttpClient base URLs and offers streamlined options for registering services, either individually or as a complete set for an API. These changes aim to improve developer experience by reducing boilerplate and enhancing the clarity of service setup. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 significantly improves the dependency injection experience for the library by introducing more idiomatic .NET patterns. The new Configure{{ApiName}} and Configure{{ApiName}}Defaults extension methods, along with the individual Add{{Service}} methods, provide developers with more flexibility and reduce boilerplate code. The core change of handling BaseAddress construction within the service constructors is a great improvement.
My review includes a few suggestions to enhance the changes further:
- Addressing a test that appears to be disabled in a non-standard way.
- Refactoring duplicated code in the new tests to improve maintainability.
- Cleaning up unused parameters in one of the new extension methods.
| hostBuilder.ConfigureServices((context, services) => | ||
| { | ||
| HostConfiguration hostConfiguration = new HostConfiguration(services); | ||
|
|
||
| hostConfigurationOptions(context, services, hostConfiguration); | ||
| }); |
There was a problem hiding this comment.
The httpClientOptions and httpClientBuilderOptions parameters of the Configure{{apiName}} method are no longer used within its body. This can be misleading for developers. Consider removing them from the method signature to reflect that HttpClient configuration should now be done via the new IServiceCollection extension methods.
|
|
||
| [TestMethod] | ||
| public async Task Given_Payments_When_CardDetails_With_Empty_RequestOptions_Succeeds() | ||
| public async Task xGiven_Payments_When_CardDetails_With_Empty_RequestOptions_Succeeds() |
There was a problem hiding this comment.
The test method Given_Payments_When_CardDetails_With_Empty_RequestOptions_Succeeds has been renamed to xGiven_Payments_When_CardDetails_With_Empty_RequestOptions_Succeeds, which effectively disables it. Is this intentional? If the test needs to be temporarily disabled, it's better to use the [Ignore] attribute for clarity, along with a comment explaining why it's ignored. For example:
[TestMethod]
[Ignore("Reason for ignoring this test")]
public async Task Given_Payments_When_CardDetails_With_Empty_RequestOptions_Succeeds()
{
// ...
}If this test is no longer needed, it should be removed.
| public void Given_ConfigureCheckout_When_AddAllCheckoutServices_Is_Called_Result_Is_Not_Null() | ||
| { | ||
| // Arrange | ||
| IHost testHost = Host.CreateDefaultBuilder() | ||
| .ConfigureCheckout((context, services, config) => | ||
| { | ||
| config.ConfigureAdyenOptions(options => | ||
| { | ||
| options.Environment = AdyenEnvironment.Test; | ||
| options.AdyenApiKey = "ADYEN_API_KEY"; | ||
| }); | ||
|
|
||
| services.AddAllCheckoutServices(); | ||
| }) | ||
| .Build(); | ||
|
|
||
| // Act | ||
|
|
||
| // 1. Services | ||
| var donationsService = testHost.Services.GetRequiredService<IDonationsService>(); | ||
| var modificationsService = testHost.Services.GetRequiredService<IModificationsService>(); | ||
| var ordersService = testHost.Services.GetRequiredService<IOrdersService>(); | ||
| var paymentLinksService = testHost.Services.GetRequiredService<IPaymentLinksService>(); | ||
| var paymentsService = testHost.Services.GetRequiredService<IPaymentsService>(); | ||
| var recurringService = testHost.Services.GetRequiredService<IRecurringService>(); | ||
| var utilityService = testHost.Services.GetRequiredService<IUtilityService>(); | ||
|
|
||
| // 2. ServiceEvents | ||
| var donationsServiceEvents = testHost.Services.GetRequiredService<DonationsServiceEvents>(); | ||
| var modificationsServiceEvents = testHost.Services.GetRequiredService<ModificationsServiceEvents>(); | ||
| var ordersServiceEvents = testHost.Services.GetRequiredService<OrdersServiceEvents>(); | ||
| var paymentLinksServiceEvents = testHost.Services.GetRequiredService<PaymentLinksServiceEvents>(); | ||
| var paymentsServiceEvents = testHost.Services.GetRequiredService<PaymentsServiceEvents>(); | ||
| var recurringServiceEvents = testHost.Services.GetRequiredService<RecurringServiceEvents>(); | ||
| var utilityServiceEvents = testHost.Services.GetRequiredService<UtilityServiceEvents>(); | ||
|
|
||
| // Assert | ||
|
|
||
| // 1. Services | ||
| Assert.IsNotNull(donationsService); | ||
| Assert.IsNotNull(modificationsService); | ||
| Assert.IsNotNull(ordersService); | ||
| Assert.IsNotNull(paymentLinksService); | ||
| Assert.IsNotNull(paymentsService); | ||
| Assert.IsNotNull(recurringService); | ||
| Assert.IsNotNull(utilityService); | ||
|
|
||
| // 2. ServiceEvents | ||
| Assert.IsNotNull(donationsServiceEvents); | ||
| Assert.IsNotNull(modificationsServiceEvents); | ||
| Assert.IsNotNull(ordersServiceEvents); | ||
| Assert.IsNotNull(paymentLinksServiceEvents); | ||
| Assert.IsNotNull(paymentsServiceEvents); | ||
| Assert.IsNotNull(recurringServiceEvents); | ||
| Assert.IsNotNull(utilityServiceEvents); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void Given_ConfigureCheckoutDefaults_When_Configure_AdyenOptions_Is_Called_Result_Is_Not_Null() | ||
| { | ||
| // Arrange | ||
| IHost testHost = Host.CreateDefaultBuilder() | ||
| .ConfigureCheckoutDefaults((context, services, config) => | ||
| { | ||
| config.ConfigureAdyenOptions(options => | ||
| { | ||
| options.Environment = AdyenEnvironment.Test; | ||
| options.AdyenApiKey = "ADYEN_API_KEY"; | ||
| }); | ||
| }) | ||
| .Build(); | ||
|
|
||
| // Act | ||
|
|
||
| // 1. Services | ||
| var donationsService = testHost.Services.GetRequiredService<IDonationsService>(); | ||
| var modificationsService = testHost.Services.GetRequiredService<IModificationsService>(); | ||
| var ordersService = testHost.Services.GetRequiredService<IOrdersService>(); | ||
| var paymentLinksService = testHost.Services.GetRequiredService<IPaymentLinksService>(); | ||
| var paymentsService = testHost.Services.GetRequiredService<IPaymentsService>(); | ||
| var recurringService = testHost.Services.GetRequiredService<IRecurringService>(); | ||
| var utilityService = testHost.Services.GetRequiredService<IUtilityService>(); | ||
|
|
||
| // 2. ServiceEvents | ||
| var donationsServiceEvents = testHost.Services.GetRequiredService<DonationsServiceEvents>(); | ||
| var modificationsServiceEvents = testHost.Services.GetRequiredService<ModificationsServiceEvents>(); | ||
| var ordersServiceEvents = testHost.Services.GetRequiredService<OrdersServiceEvents>(); | ||
| var paymentLinksServiceEvents = testHost.Services.GetRequiredService<PaymentLinksServiceEvents>(); | ||
| var paymentsServiceEvents = testHost.Services.GetRequiredService<PaymentsServiceEvents>(); | ||
| var recurringServiceEvents = testHost.Services.GetRequiredService<RecurringServiceEvents>(); | ||
| var utilityServiceEvents = testHost.Services.GetRequiredService<UtilityServiceEvents>(); | ||
|
|
||
| // Assert | ||
|
|
||
| // 1. Services | ||
| Assert.IsNotNull(donationsService); | ||
| Assert.IsNotNull(modificationsService); | ||
| Assert.IsNotNull(ordersService); | ||
| Assert.IsNotNull(paymentLinksService); | ||
| Assert.IsNotNull(paymentsService); | ||
| Assert.IsNotNull(recurringService); | ||
| Assert.IsNotNull(utilityService); | ||
|
|
||
| // 2. ServiceEvents | ||
| Assert.IsNotNull(donationsServiceEvents); | ||
| Assert.IsNotNull(modificationsServiceEvents); | ||
| Assert.IsNotNull(ordersServiceEvents); | ||
| Assert.IsNotNull(paymentLinksServiceEvents); | ||
| Assert.IsNotNull(paymentsServiceEvents); | ||
| Assert.IsNotNull(recurringServiceEvents); | ||
| Assert.IsNotNull(utilityServiceEvents); | ||
| } |
There was a problem hiding this comment.
The test methods Given_ConfigureCheckout_When_AddAllCheckoutServices_Is_Called_Result_Is_Not_Null and Given_ConfigureCheckoutDefaults_When_Configure_AdyenOptions_Is_Called_Result_Is_Not_Null contain a large block of identical code for resolving and asserting services. This duplication can make the tests harder to maintain.
Consider extracting the common assertion logic into a private helper method to reduce duplication and improve readability.
For example, you could create a method like this:
private void AssertAllCheckoutServicesAreRegistered(IHost host)
{
// 1. Services
Assert.IsNotNull(host.Services.GetRequiredService<IDonationsService>());
Assert.IsNotNull(host.Services.GetRequiredService<IModificationsService>());
Assert.IsNotNull(host.Services.GetRequiredService<IOrdersService>());
Assert.IsNotNull(host.Services.GetRequiredService<IPaymentLinksService>());
Assert.IsNotNull(host.Services.GetRequiredService<IPaymentsService>());
Assert.IsNotNull(host.Services.GetRequiredService<IRecurringService>());
Assert.IsNotNull(host.Services.GetRequiredService<IUtilityService>());
// 2. ServiceEvents
Assert.IsNotNull(host.Services.GetRequiredService<DonationsServiceEvents>());
Assert.IsNotNull(host.Services.GetRequiredService<ModificationsServiceEvents>());
Assert.IsNotNull(host.Services.GetRequiredService<OrdersServiceEvents>());
Assert.IsNotNull(host.Services.GetRequiredService<PaymentLinksServiceEvents>());
Assert.IsNotNull(host.Services.GetRequiredService<PaymentsServiceEvents>());
Assert.IsNotNull(host.Services.GetRequiredService<RecurringServiceEvents>());
Assert.IsNotNull(host.Services.GetRequiredService<UtilityServiceEvents>());
}And then call it from both test methods.
1 participant
Description
This PR addresses Dependency Injection for the .NET Library. Developers can now configure and inject the service they want to use.
Before:
It wasn't possible to initialize any
Servicewith the correct URL by injecting them as follows:the underlying HttpClient wouldn't know the URL at this point. The work-around for developers (on the consuming-side) would be:
This is not ideal. This PR addresses this:
After:
The
BaseURLis now constructed once in the constructor of theService. For this to work, we introducedAdyenOptionsProviderto make this work for all services.Improvements
ConstructUrl(AdyenOptions, "example https://checkout-test.adyen.com/v71")is called inside the Service-constructorAdyenOptionsProviderto retrieve theAdyenOptionsused to Construct the Url based on theEnvironmentandLiveEndpointUrlPrefixServiceLifeTime(singleton, scoped, transients), default: SingletonConfigure{{apiName}}to allow developers to inject any service (see tested scenarios), example where:{{apiName}}=CheckoutConfigure{{apiName}}Defaultsto inject all services automatically (see tested scenarios), example where:{{apiName}}=Checkoutapi_doc.mustachewith correct snippetsJsonSerializerOptionsProviderTested scenarios
Extending the tests in [1] the CheckoutAPI (covering APIs) and [2] AcsWebhooks (covering the webhooks).
Dependency Injection using
Configure{{ApiName}}ConfigureCheckout-method and allow the developer to dependency inject individual services from the Checkout API:Dependency Injection using
Configure{{ApiName}}DefaultsConfigureCheckoutDefaults-method and automatically injects all services from the Checkout API:Roll-out plan