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

Feat/mailgun entry point #94

Open
wants to merge 10 commits into
base: milestone/mailgun-support
Choose a base branch
from
22 changes: 22 additions & 0 deletions src/Sinch/Core/UrlResolver.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using Sinch.Conversation;
using Sinch.Mailgun;
using Sinch.SMS;
using Sinch.Voice;

Expand Down Expand Up @@ -83,5 +84,26 @@ public Uri ResolveSmsServicePlanIdUrl(SmsServicePlanIdRegion smsServicePlanIdReg
return new Uri(string.Format(smsApiServicePlanIdUrlTemplate,
smsServicePlanIdRegion.Value.ToLowerInvariant()));
}

public Uri ResolveMailgunUrl(MailgunRegion mailgunRegion)
{
if (!string.IsNullOrEmpty(_apiUrlOverrides?.MailgunUrl)) return new Uri(_apiUrlOverrides.MailgunUrl);

string? mailgunUrl;
switch (mailgunRegion)
{
case MailgunRegion.Us:
mailgunUrl = "https://api.mailgun.net";
break;
case MailgunRegion.Eu:
mailgunUrl = "https://api.eu.mailgun.net";
break;
default:
throw new ArgumentOutOfRangeException(nameof(mailgunRegion), mailgunRegion,
"Unexpected enum value.");
}

return new Uri(mailgunUrl);
}
}
}
18 changes: 18 additions & 0 deletions src/Sinch/Mailgun/MailgunRegion.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
namespace Sinch.Mailgun
{
/// <summary>
/// Mailgun allows the ability to send and receive email in both US and EU regions.
/// Be sure to use the appropriate region on which you have created your domain.
/// </summary>
public enum MailgunRegion
{
/// <summary>
/// United States region
/// </summary>
Us,
/// <summary>
/// Europe region
/// </summary>
Eu
}
}
23 changes: 23 additions & 0 deletions src/Sinch/Mailgun/SinchMailgunClient.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
using System;
using Sinch.Core;
using Sinch.Logger;

namespace Sinch.Mailgun
{
/// <summary>
/// The Mailgun API is part of the Sinch family and enables you to send, track, and receive email effortlessly.
/// </summary>
public interface ISinchMailgunClient
{
// TBD: add domains
}

/// <inheritdoc />
internal class SinchMailgunClient : ISinchMailgunClient
{
public SinchMailgunClient(Uri baseUrl, Http http, LoggerFactory? loggerFactory = null)
{
// TBD: implement domains
}
}
}
54 changes: 54 additions & 0 deletions src/Sinch/SinchClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Sinch.Conversation;
using Sinch.Core;
using Sinch.Logger;
using Sinch.Mailgun;
using Sinch.Numbers;
using Sinch.SMS;
using Sinch.Verification;
Expand Down Expand Up @@ -111,6 +112,17 @@ public ISinchVerificationClient Verification(string appKey, string appSecret,
/// <param name="voiceRegion">See <see cref="VoiceRegion" />. Defaults to <see cref="VoiceRegion.Global" /></param>
/// <returns></returns>
public ISinchVoiceClient Voice(string appKey, string appSecret, VoiceRegion? voiceRegion = null);

/// <summary>
/// APIs are at the heart of Mailgun.
/// Our goal is to provide developers worldwide with an accessible and straightforward way to send,
/// receive, and track emails effortlessly.
/// </summary>
/// <param name="apiKey">When you sign up for Mailgun, a primary account API key is generated.
/// This key allows you to perform all CRUD operations via our various API endpoints and for any of your sending domains. </param>
/// <param name="region"><see cref="MailgunRegion"/></param>
/// <returns></returns>
public ISinchMailgunClient Mailgun(string apiKey, MailgunRegion region);
}

public class SinchClient : ISinchClient
Expand All @@ -131,6 +143,32 @@ public class SinchClient : ISinchClient
private readonly ILoggerAdapter<ISinchClient>? _logger;
private readonly UrlResolver _urlResolver;

/// <summary>
/// Initialize a new <see cref="SinchClient" /> without providing common credentials.
/// Intended to be used for APIs which requires their own credentials:
/// <list type="bullet">
/// <item>
/// <see cref="Mailgun"/>
/// </item>
/// <item>
/// <see cref="Voice"/>
/// </item>
/// <item>
/// <see cref="Verification"/>
/// </item>
/// </list>
/// If you plan to use other APIs or not sure about what API you are planning to use, consider using main constructor
/// <see cref="SinchClient(String?, String?, String?, Action&lt;SinchOptions>)"/>
/// <example>
/// <code>
/// var mailgunClient = new SinchClient().Mailgun("apikey", MailgunRegion.Eu);
/// </code>
/// </example>
/// </summary>
/// <param name="options">Optional. See: <see cref="SinchOptions" /></param>
public SinchClient(Action<SinchOptions>? options = default) : this(null, null, null, 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.

Please, take a look here @asein-sinch regarding design of Sinch entry point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, if we agree on that, will move it to a separate PR to include it in version without mailgun

Choose a reason for hiding this comment

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

It's interesting to have a SinchClient constructor with no required parameters but it becomes ever more confusing to let the SinchOptions at this level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap, that's why I'm content with current design of having only one entry point, even with "weird" feel if you only need other API.
Nonetheless, I'm open to other suggestions, but can we, please, move this discussion as out of scope of this PR and discuss it elsewhere and for other task/PR?

Choose a reason for hiding this comment

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

I think this is precisely the goal of this PR to address the Mailgun entry point as the name suggests. Would it make sense to define the options in the Mailgun() constructor: Mailgun(key, region, 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.

As stated previously, It will require SinchOptions on the entry point of the SinchClient non the less. That's why SinchClient().Mailgun(options?) will not flawlesly here, as well as I'm lacking understanding what problem it will solve.
Two options I'm fine with:

  • As was before SinchClient(key?, secret?, projectId?, options?)
  • The proposition with an empty SinchClient(options?) for speficic APIs, in separate PR as it affects whole SDK - to clarify, before merging Mailgun to main, this change will land here eventually.

Again, I'm open to other designs, if any.

Choose a reason for hiding this comment

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

SinchClient(sinchOptions).Mailgun(key, region, url)
SinchClient(sinchOptions).Mailgun(key, region, mailgunOptions)

The idea is to not pollute the SinchClient with options for a non-standard API and to not have to manage the Mailgun configuration in various places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idea makes sense, but bear with me:

  • In case you have an optional SinchOptions.UrlOverrides where you can setup all urls you need in one place and other optionals is SinchOptions, where you will look for any similar stuff.

  • In case of SinchClient(sinchOptions).Mailgun(key, region, mailgunOptions/url), and we will be talking about optional urlOverride specifically, you will be overriding near the entry point, and I like the idea. But now, what about non-specific api like Conversation? There it is still a need for UrlOverride. Should I rewrote all design to SpecificApi(Options)? to have consistency, breaking backward compatibility. Or leave SinchOptions.UrlOverride as is, now having two places where you can setup UrlOverride - in constructor of specific API AND in a global SinchOptions? A for me, having to group similar settings makes sense. . Also, Options pattern is widely used in dotnet ecosystem, and intuitively, devs gonna to look into it to see what's there.
    Also, you probably can ask: then why Mailgun(creds, region) takes place. And answer to that is I want to force here a conscious decision for a required properties. While optional SinchOptions.UrlOverrides is for a specific purpose.

In general, it comes down to personal preference.
The source of a problem is a variety of SInch products and we trying our best to provide a somewhat unified experience.
And to state again, this design applies not only to Mailgun, but affects all SDK.

Choose a reason for hiding this comment

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

In general, it comes down to personal preference.

TBH a documented team decision would be preferable.
If the decision leads to a breaking change, then we can plan for a v2 with the delivery of the mailgun support in the SDK.

Copy link
Contributor Author

@Dovchik Dovchik Dec 18, 2024

Choose a reason for hiding this comment

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

I didn't get what you mean here.
Should we include doc team to discussion?
Any more comments about design? Do you want to continue discussion?

{
}

/// <summary>
/// Initialize a new <see cref="SinchClient" />
Expand Down Expand Up @@ -274,6 +312,22 @@ public ISinchVoiceClient Voice(string appKey, string appSecret,
_urlResolver.ResolveVoiceApplicationManagementUrl());
}

/// <inheritdoc />
public ISinchMailgunClient Mailgun(string apiKey, MailgunRegion region)
{
if (string.IsNullOrEmpty(apiKey))
{
throw new ArgumentNullException(nameof(apiKey), "apiKey shouldn't be null or empty");
}

var baseUrl = _urlResolver.ResolveMailgunUrl(region);
var mailgunAuth = new BasicAuth(appKey: "api", appSecret: apiKey);
// NOTE: jsonNamingPolicy will not play a role here as property naming of mailgun is inconsistent
// meaning, all lifting will be done through JsonPropertyNamingAttribute
var http = new Http(mailgunAuth, _httpClient, _loggerFactory?.Create<IHttp>(), JsonNamingPolicy.CamelCase);
return new SinchMailgunClient(baseUrl, http, _loggerFactory);
}

private void ValidateCommonCredentials()
{
var exceptions = new List<Exception>();
Expand Down
5 changes: 5 additions & 0 deletions src/Sinch/SinchOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -137,5 +137,10 @@ public sealed class ApiUrlOverrides
/// Overrides Numbers api base url
/// </summary>
public string? NumbersUrl { get; init; }

/// <summary>
/// Overrides Mailgun api base url
/// </summary>
public string? MailgunUrl { get; init; }
}
}
50 changes: 49 additions & 1 deletion tests/Sinch.Tests/Core/UrlResolverTests.cs
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
using System;
using System.Collections.Generic;
using System.Linq;
using FluentAssertions;
using Sinch.Conversation;
using Sinch.Core;
using Sinch.Mailgun;
using Sinch.SMS;
using Sinch.Voice;
using Xunit;
Expand Down Expand Up @@ -306,13 +308,59 @@ public void ResolveSmsUrl(SmsRegion smsRegion, ApiUrlOverrides apiUrlOverrides)

[Theory]
[MemberData(nameof(SmsServicePlanIdUrlData))]
public void ResolveSmsServicePlanIdUrl(SmsServicePlanIdRegion smsServicePlanIdRegion, ApiUrlOverrides apiUrlOverrides)
public void ResolveSmsServicePlanIdUrl(SmsServicePlanIdRegion smsServicePlanIdRegion,
ApiUrlOverrides apiUrlOverrides)
{
var smsUrl = new UrlResolver(apiUrlOverrides).ResolveSmsServicePlanIdUrl(smsServicePlanIdRegion);
var expectedUrl = string.IsNullOrEmpty(apiUrlOverrides?.SmsUrl)
? new Uri($"https://{smsServicePlanIdRegion.Value}.sms.api.sinch.com/")
: new Uri(apiUrlOverrides.SmsUrl);
smsUrl.Should().BeEquivalentTo(expectedUrl);
}

public record ResolveMailgunUrlTestData(
string TestName,
MailgunRegion Region,
ApiUrlOverrides ApiUrlOverrides,
string ExpectedUrl)
{
private static readonly ResolveMailgunUrlTestData[] TestCases =
{
new("Default US Mailgun address", MailgunRegion.Us, null, "https://api.mailgun.net"),
new("Default EU Mailgun address", MailgunRegion.Eu, null, "https://api.eu.mailgun.net"),
new("Default EU region even if override set but null", MailgunRegion.Eu, new ApiUrlOverrides()
{
MailgunUrl = null
}, "https://api.eu.mailgun.net"),
new("Override url", MailgunRegion.Eu, new ApiUrlOverrides()
{
MailgunUrl = "https://my-mailgun-proxy.net"
}, "https://my-mailgun-proxy.net"),
};

public static IEnumerable<object[]> TestCasesData =>
TestCases.Select(testCase => new object[] { testCase });

public override string ToString()
{
return TestName;
}
}

[Theory]
[MemberData(nameof(ResolveMailgunUrlTestData.TestCasesData), MemberType = typeof(ResolveMailgunUrlTestData))]
public void ResolveMailgunUrl(ResolveMailgunUrlTestData data)
{
var actual = new UrlResolver(data.ApiUrlOverrides).ResolveMailgunUrl(data.Region);

actual.Should().BeEquivalentTo(new Uri(data.ExpectedUrl));
}

[Fact]
public void ThrowIfRegionIsUnexpected()
{
var op = () => new UrlResolver(new ApiUrlOverrides()).ResolveMailgunUrl((MailgunRegion)(-1));
op.Should().ThrowExactly<ArgumentOutOfRangeException>();
}
}
}
28 changes: 28 additions & 0 deletions tests/Sinch.Tests/Mailgun/MailgunClientTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
using System;
using FluentAssertions;
using Sinch.Mailgun;
using Xunit;

namespace Sinch.Tests.Mailgun
{
public class MailgunClientTests
{
[Fact]
public void InitMailgunClient()
{
var mailgun = new SinchClient().Mailgun("apikey", MailgunRegion.Eu);

mailgun.Should().NotBeNull();
}

[Theory]
[InlineData("")]
[InlineData(null)]
public void FailEmptyApiKey(string apiKey)
{
var mailgunCreation = () => new SinchClient().Mailgun(apiKey, MailgunRegion.Eu);

mailgunCreation.Should().Throw<ArgumentNullException>();
}
}
}
Loading