Skip to content

When no mail providers are registered, fall back to no-op behavior #714

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
+57 −84
Closed

When no mail providers are registered, fall back to no-op behavior #714

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a398da0
When no mail providers are registered, fall back to no-op behavior
Tyrrrz Aug 30, 2024
b106d59
tests
Tyrrrz Aug 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 12 additions & 33 deletions src/Common/Services/Mail/AggregateMailProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,53 +5,32 @@ namespace Passwordless.Common.Services.Mail;
/// <summary>
/// Wraps multiple mail providers and tries to send the message using them in order.
/// </summary>
public class AggregateMailProvider : IMailProvider
public class AggregateMailProvider(
IOptionsSnapshot<MailConfiguration> options,
IMailProviderFactory factory,
ILogger<AggregateMailProvider> logger)
: IMailProvider
{
private readonly IOptionsSnapshot<MailConfiguration> _options;
private readonly IMailProviderFactory _factory;
private readonly ILogger<AggregateMailProvider> _logger;

public const string FallBackFailedMessage = "No registered mail provider was able to send the message";

public AggregateMailProvider(
IOptionsSnapshot<MailConfiguration> options,
IMailProviderFactory factory,
ILogger<AggregateMailProvider> logger)
{
_options = options;
_factory = factory;
_logger = logger;
}

public async Task SendAsync(MailMessage message)
{
message.From ??= options.Value.From;
message.FromDisplayName ??= options.Value.FromName;

if (message.From == null)
{
message.From = _options.Value.From;
}
if (message.FromDisplayName == null)
{
message.FromDisplayName = _options.Value.FromName;
}
foreach (var providerConfiguration in _options.Value.Providers)
foreach (var providerConfiguration in options.Value.Providers)
{
try
{
_logger.LogDebug("Attempting to send message using provider '{Provider}'", providerConfiguration.Name);
var provider = _factory.Create(providerConfiguration.Name, providerConfiguration);
logger.LogDebug("Attempting to send message using provider '{Provider}'", providerConfiguration.Name);
var provider = factory.Create(providerConfiguration.Name, providerConfiguration);

await provider.SendAsync(message);
_logger.LogInformation("Sent message using provider '{Provider}'", providerConfiguration.Name);
logger.LogInformation("Sent message using provider '{Provider}'", providerConfiguration.Name);
return;
}
catch (Exception e)
{
_logger.LogError(e, "Failed to send message using provider '{Provider}'", providerConfiguration.Name);
Copy link
Contributor

Choose a reason for hiding this comment

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

This one was just to let us know when we fail to send using a specific provider, and log accordingly if it is not being logged by the provider itself specifically. It is to help us know when there is a specific issue with either a template or credentials of a specific provider.

logger.LogError(e, "Failed to send message using provider '{Provider}'", providerConfiguration.Name);
}
}

_logger.LogCritical(FallBackFailedMessage);
Copy link
Contributor

Choose a reason for hiding this comment

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

If none of the registered e-mail providers can send, we need to know all of them fail. Because at that point the customers could be critically impacted being unable to log in.

throw new InvalidOperationException(FallBackFailedMessage);
}
}
19 changes: 3 additions & 16 deletions src/Common/Services/Mail/MailBootstrap.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,26 +17,12 @@ public static void AddMail(this WebApplicationBuilder builder)
{
var section = builder.Configuration.GetSection("Mail:Providers");

if (!section.GetChildren().Any())
{
o.Providers = new List<BaseMailProviderOptions>
{
new FileMailProviderOptions()
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing it might not be a good idea, we could define a new mail provider to log to the console using a warning or info log level for testing. Or output to Memorycache or some other in-memory option.

For self-hosting keeping the file is better by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think all you need to do to fix the API testing image is the following:

#716

Copy link
Contributor

Choose a reason for hiding this comment

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

It is ready, the tests are passing

};
return;
}

// Iterate over all configured providers and add them to the list with binding.

var providers = new List<BaseMailProviderOptions>();
foreach (var child in section.GetChildren())
{
var type = child.GetValue<string>("Name");

if (type == null)
{
throw new ConfigurationErrorsException("Provider type is missing");
}
var type = child.GetValue<string>("Name")
?? throw new ConfigurationErrorsException("Provider type is missing");

BaseMailProviderOptions mailProviderOptions = type.ToLowerInvariant() switch
{
Expand All @@ -52,6 +38,7 @@ public static void AddMail(this WebApplicationBuilder builder)

providers.Add(mailProviderOptions);
}

o.Providers = providers;
});
}
Expand Down
2 changes: 1 addition & 1 deletion src/Common/Services/Mail/MailConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@ public class MailConfiguration
/// <summary>
/// The ordered list of mail providers to use.
/// </summary>
public IReadOnlyCollection<BaseMailProviderOptions> Providers { get; set; } = new List<BaseMailProviderOptions>();
public IReadOnlyCollection<BaseMailProviderOptions> Providers { get; set; } = [];
}
75 changes: 41 additions & 34 deletions tests/Common.Tests/Services/Mail/AggregateMailProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,23 +27,15 @@ public AggregateMailProviderTests()
}

[Fact]
public async Task SendAsync_Throws_InvalidOperationException_WhenNoProvidersAreRegistered()
public async Task SendAsync_NoOp_WhenNoProvidersAreRegistered()
{
// Arrange
var mailMessage = _fixture.Create<MailMessage>();
var mailOptions = new MailConfiguration
{
From = "[email protected]",
Providers = new List<BaseMailProviderOptions>()
};
var mailOptions = new MailConfiguration { From = "[email protected]", Providers = [] };
_optionsMock.SetupGet(x => x.Value).Returns(mailOptions);

// Act
var actual = await Assert.ThrowsAsync<InvalidOperationException>(async () =>
await _sut.SendAsync(mailMessage));

// Assert
Assert.Equal(AggregateMailProvider.FallBackFailedMessage, actual.Message);
// Act & assert
await _sut.SendAsync(mailMessage);
}

[Fact]
Expand All @@ -54,27 +46,30 @@ public async Task SendAsync_Overrides_MailMessageSenderFromConfiguration_WhenMai
var mailOptions = new MailConfiguration
{
From = "[email protected]",
Providers = new List<BaseMailProviderOptions>
{
Providers =
[
_fixture.Build<AwsMailProviderOptions>()
.With(x => x.Name, AwsMailProviderOptions.Provider)
.Create(),
_fixture.Build<SendGridMailProviderOptions>()
.With(x => x.Name, SendGridMailProviderOptions.Provider)
.Create()
}
]
};
_optionsMock.SetupGet(x => x.Value).Returns(mailOptions);

var awsProviderMock = new Mock<IMailProvider>();
_factoryMock.Setup(x => x.Create(It.Is<string>(y => y == AwsMailProviderOptions.Provider), It.IsAny<BaseMailProviderOptions>()))
_factoryMock.Setup(x => x.Create(It.Is<string>(y => y == AwsMailProviderOptions.Provider),
It.IsAny<BaseMailProviderOptions>()))
.Returns(awsProviderMock.Object);

// Act
await _sut.SendAsync(mailMessage);

// Assert
_factoryMock.Verify(x => x.Create(It.Is<string>(y => y == AwsMailProviderOptions.Provider), It.IsAny<AwsMailProviderOptions>()), Times.Once);
_factoryMock.Verify(
x => x.Create(It.Is<string>(y => y == AwsMailProviderOptions.Provider), It.IsAny<AwsMailProviderOptions>()),
Times.Once);
awsProviderMock.Verify(x => x.SendAsync(It.Is<MailMessage>(p => p.From == mailOptions.From)), Times.Once);
}

Expand All @@ -86,27 +81,30 @@ public async Task SendAsync_DoesNotOverride_MailMessageSenderFromConfiguration_W
var mailOptions = new MailConfiguration
{
From = "[email protected]",
Providers = new List<BaseMailProviderOptions>
{
Providers =
[
_fixture.Build<AwsMailProviderOptions>()
.With(x => x.Name, AwsMailProviderOptions.Provider)
.Create(),
_fixture.Build<SendGridMailProviderOptions>()
.With(x => x.Name, SendGridMailProviderOptions.Provider)
.Create()
}
]
};
_optionsMock.SetupGet(x => x.Value).Returns(mailOptions);

var awsProviderMock = new Mock<IMailProvider>();
_factoryMock.Setup(x => x.Create(It.Is<string>(y => y == AwsMailProviderOptions.Provider), It.IsAny<BaseMailProviderOptions>()))
_factoryMock.Setup(x => x.Create(It.Is<string>(y => y == AwsMailProviderOptions.Provider),
It.IsAny<BaseMailProviderOptions>()))
.Returns(awsProviderMock.Object);

// Act
await _sut.SendAsync(mailMessage);

// Assert
_factoryMock.Verify(x => x.Create(It.Is<string>(y => y == AwsMailProviderOptions.Provider), It.IsAny<AwsMailProviderOptions>()), Times.Once);
_factoryMock.Verify(
x => x.Create(It.Is<string>(y => y == AwsMailProviderOptions.Provider), It.IsAny<AwsMailProviderOptions>()),
Times.Once);
awsProviderMock.Verify(x => x.SendAsync(It.Is<MailMessage>(p => p.From == mailOptions.From)), Times.Never);
awsProviderMock.Verify(x => x.SendAsync(It.Is<MailMessage>(p => p.From == mailMessage.From)), Times.Once);
}
Expand All @@ -120,27 +118,30 @@ public async Task SendAsync_Executes_FirstRegisteredProviderFirst_WhenMultipleAr
var mailOptions = new MailConfiguration
{
From = "[email protected]",
Providers = new List<BaseMailProviderOptions>
{
Providers =
[
_fixture.Build<AwsMailProviderOptions>()
.With(x => x.Name, AwsMailProviderOptions.Provider)
.Create(),
_fixture.Build<SendGridMailProviderOptions>()
.With(x => x.Name, SendGridMailProviderOptions.Provider)
.Create()
}
]
};
_optionsMock.SetupGet(x => x.Value).Returns(mailOptions);

var awsProviderMock = new Mock<IMailProvider>();
_factoryMock.Setup(x => x.Create(It.Is<string>(y => y == AwsMailProviderOptions.Provider), It.IsAny<BaseMailProviderOptions>()))
_factoryMock.Setup(x => x.Create(It.Is<string>(y => y == AwsMailProviderOptions.Provider),
It.IsAny<BaseMailProviderOptions>()))
.Returns(awsProviderMock.Object);

// Act
await _sut.SendAsync(mailMessage);

// Assert
_factoryMock.Verify(x => x.Create(It.Is<string>(y => y == AwsMailProviderOptions.Provider), It.IsAny<AwsMailProviderOptions>()), Times.Once);
_factoryMock.Verify(
x => x.Create(It.Is<string>(y => y == AwsMailProviderOptions.Provider), It.IsAny<AwsMailProviderOptions>()),
Times.Once);
}

[Fact]
Expand All @@ -151,34 +152,40 @@ public async Task SendAsync_Attempts_ToUseSecondProvider_WhenFirstProviderFails(
var mailOptions = new MailConfiguration
{
From = "[email protected]",
Providers = new List<BaseMailProviderOptions>
{
Providers =
[
_fixture.Build<AwsMailProviderOptions>()
.With(x => x.Name, AwsMailProviderOptions.Provider)
.Create(),
_fixture.Build<SendGridMailProviderOptions>()
.With(x => x.Name, SendGridMailProviderOptions.Provider)
.Create()
}
]
};
_optionsMock.SetupGet(x => x.Value).Returns(mailOptions);

var awsProviderMock = new Mock<IMailProvider>();
awsProviderMock.Setup(x => x.SendAsync(It.IsAny<MailMessage>()))
.Throws<Exception>();
_factoryMock.Setup(x => x.Create(It.Is<string>(y => y == AwsMailProviderOptions.Provider), It.IsAny<BaseMailProviderOptions>()))
_factoryMock.Setup(x => x.Create(It.Is<string>(y => y == AwsMailProviderOptions.Provider),
It.IsAny<BaseMailProviderOptions>()))
.Returns(awsProviderMock.Object);

var sendGridProviderMock = new Mock<IMailProvider>();
_factoryMock.Setup(x => x.Create(It.Is<string>(y => y == SendGridMailProviderOptions.Provider), It.IsAny<BaseMailProviderOptions>()))
_factoryMock.Setup(x => x.Create(It.Is<string>(y => y == SendGridMailProviderOptions.Provider),
It.IsAny<BaseMailProviderOptions>()))
.Returns(sendGridProviderMock.Object);

// Act
await _sut.SendAsync(mailMessage);

// Assert
_factoryMock.Verify(x => x.Create(It.Is<string>(y => y == AwsMailProviderOptions.Provider), It.IsAny<AwsMailProviderOptions>()), Times.Once);
_factoryMock.Verify(x => x.Create(It.Is<string>(y => y == SendGridMailProviderOptions.Provider), It.IsAny<SendGridMailProviderOptions>()), Times.Once);
_factoryMock.Verify(
x => x.Create(It.Is<string>(y => y == AwsMailProviderOptions.Provider), It.IsAny<AwsMailProviderOptions>()),
Times.Once);
_factoryMock.Verify(
x => x.Create(It.Is<string>(y => y == SendGridMailProviderOptions.Provider),
It.IsAny<SendGridMailProviderOptions>()), Times.Once);

}
}
Loading