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

Add support for ASP.NET Core Dependency Injection #904

Closed
wants to merge 7 commits into from
Closed

Add support for ASP.NET Core Dependency Injection #904

wants to merge 7 commits into from

Conversation

aevitas
Copy link
Contributor

@aevitas aevitas commented Jul 4, 2019

Fixes #880

This PR adds support for ASP.NET Core applications to use SendGrid with idiomatic dependency injection. I have tried to minimize the surface area impacted in SendGridClient, but had to make some modifications (most notably InitializeClient() handling some of the client initialization now).

Included is an example ASP.NET Core API with limited functionality which documents the usage of the new APIs and demonstrates how the various components interact.

While I've tested these changes and confirmed them to be working, I couldn't get the Integration tests to actually succeed due to IntegrationFixture apparently missing a file. Having reviewed the tests in question, I don't think these changes impact these tests at all, but I'd like to get a contributor's opinion on this.

If I've made a mistake in my code or assumptions, please let me know and I'd be happy to fix it.

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the development branch.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added in line documentation to the code I modified

Short description of what this PR does:

  • Add IOptions<SendGridClientOptions> support to SendGridClient
  • Enables the use of services.AddTransient/Scoped/Singleton<ISendGridClient> and subsequent injection into controllers
  • Provides a basic example in ASP.NET Core 2.2 on how to use the client with dependency injection, and sending a basic e-mail from an API.

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Jul 4, 2019
/// <param name="options">An <see cref="IOptions{SendGridClientOptions}"/> instance specifying the configuration to be used with the client.</param>
public SendGridClient(HttpClient httpClient, IOptions<SendGridClientOptions> options)
{
if (options?.Value == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this if is the reason you need to introduce InitializeClient you could use this(httpClient, options?.Value ?? throw new ArgumentNullException(nameof(options)))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I didn't know it could be expressed like that! I've refactored the code to include your suggestion.

/// </summary>
/// <param name="options">An <see cref="IOptions{SendGridClientOptions}"/> instance specifying the configuration to be used with the client.</param>
public SendGridClient(IOptions<SendGridClientOptions> options)
: this(options.Value)
Copy link
Contributor

Choose a reason for hiding this comment

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

options?.Value ?? throw new ArgumentNullException(nameof(options)) here as well?

Copy link
Contributor Author

@aevitas aevitas Jul 5, 2019

Choose a reason for hiding this comment

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

Wouldn't it make more sense to check for null in SendGridClient(HttpClient httpClient, SendGridClientOptions options):

internal SendGridClient(HttpClient httpClient, SendGridClientOptions options)
{
    this.options = options ?? throw new ArgumentNullException(nameof(options));
    this.client = httpClient ?? CreateHttpClientWithRetryHandler();
    if (this.options.RequestHeaders != null && this.options.RequestHeaders.TryGetValue(ContentType, out var contentType))
    {
        this.MediaType = contentType;
    }
}

And simple change the other ctors to:

public SendGridClient(HttpClient httpClient, IOptions<SendGridClientOptions> options)
    : this(httpClient, options?.Value)
{
}

public SendGridClient(IOptions<SendGridClientOptions> options)
    : this(options?.Value)
{
}

That way the null check is centralized, and it also prevents the client from being constructed with null options at the place of assignment.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM.
As long as:

  • A constructor should not throw a NullRefenceException
  • Providing a null options should throw a ArgumentNullException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I've committed the changes above and I'm positive they meet your requirements. Let me know what you think.

@jnyrup
Copy link
Contributor

jnyrup commented Jul 27, 2019

I came to think of the additional unnecessary dependencies (Microsoft.Extensions.Options also has dependencies), that this would cause for users of SendGrid not relying on DI.
Especially since the additional dependency is solely for having the interface IOptions<T>.

Would the following be satisfying?

Creating a DI-aware wrapper MySendGridClient in user code that adds the two new constructors.

public class MySendGridClient : SendGridClient
{
    /// <summary>
    /// Initializes a new instance of the <see cref="SendGridClient"/> class.
    /// </summary>
    /// <param name="options">An <see cref="IOptions{SendGridClientOptions}"/> instance specifying the configuration to be used with the client.</param>
    public MySendGridClient(IOptions<SendGridClientOptions> options)
        : base(options?.Value)
    {
    }

    /// <summary>
    /// Initializes a new instance of the <see cref="SendGridClient"/> class.
    /// </summary>
    /// <param name="httpClient">The HTTP Client used to send requests to the SendGrid API.</param>
    /// <param name="options">An <see cref="IOptions{SendGridClientOptions}"/> instance specifying the configuration to be used with the client.</param>
    public MySendGridClient(HttpClient httpClient, IOptions<SendGridClientOptions> options)
        : base(httpClient, options?.Value)
    {
    }
}

And change the DI call to use MySendGridClient

public void ConfigureServices(IServiceCollection services)
{
    services.AddSingleton<ISendGridClient, MySendGridClient>();
}

@aevitas
Copy link
Contributor Author

aevitas commented Jul 27, 2019

I assume that's what a lot of people using SendGrid in ASP.NET Core applications are doing right now, myself included.

We could definitely go down this route, Sentry and a bunch of other projects do something similar by shipping a separate .AspNetCore package that allows for one-line integration along the lines of app.UseSendGrid();.

The goal of this PR was to add out of the box support for dependency injection, but you are correct in that it adds a bunch of dependencies to the project which aren't strictly needed. On the other hand, requiring user code to support it is effectively the same as not supporting it at all.

It would be useful to know how many of SendGrid's users are using ASP.NET Core and its dependency injection paradigm here.

@jnyrup
Copy link
Contributor

jnyrup commented Jul 27, 2019

We use SendGrid at work in a non-ASP application that was just recently upgraded from net461 to net472.

(FYI: I'm not affiliated with SendGrid, just a regular OSS contributor )

@akunzai
Copy link
Contributor

akunzai commented Aug 19, 2019

IMHO, we can introduce a new package (ex: SendGrid.Extensions.DependencyInjection or SendGrid.Extensions.Http) with those additional dependencies.

Even more, a SendGrid service extension method can simplify the service registration.

services.AddSendGrid(options=> options.ApiKey = Configuration["SendGrid:ApiKey"]);

we can also integrate SendGrid with HttpClientFactory directly.

I will trying to implement and send another PR for these suggestions.

@IanKemp
Copy link

IanKemp commented Sep 5, 2019

@akunzai want to close this since #922 seems to cover the use case?

@aevitas aevitas closed this Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: code review request requesting a community code review or review from Twilio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency Injection in Startup.cs
5 participants