Extension methods to convert VoiceResponse and MessagingResponse to TwiMLResult

[Giorgi]

Implements #61

As there are two TwiMLResult classes (one for Minimal API and one for controller-based approach) I had to create to classes for the extension methods and put them in the respective namespaces.

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

• [X] I acknowledge that all my contributions will be made under the project's license.

[Giorgi]

I have an idea for merging the two TwiMLResult classes into one: What if it implements (with some #ifdef magic) both interfaces IActionResult and IResult? That way there will be only one TwiMLResult and no need to have to separate TwiMLExtensions classes.

[Swimburger]

This is what I was fearing:

app.MapGet("/sms",()
    => new MessagingResponse()
        .Message($"Ahoy!")
        .ToTwiMLResult()
);

This code results in the following error:

Program.cs(47, 10): [CS0121] The call is ambiguous between the following methods or properties: 'Twilio.AspNet.Core.TwiMLExtensions.ToTwiMLResult(Twilio.TwiML.MessagingResponse)' and 'Twilio.AspNet.Core.MinimalApi.TwiMLExtensions.ToTwiMLResult(Twilio.TwiML.MessagingResponse)'

This is because namespace Twilio.AspNet.Core and Twilio.AspNet.Core.MinimalApi is imported in my case, and probably many applications.

Even when explicitly telling minimal APIs the return type is Twilio.AspNet.Core.MinimalApi.TwiMLResult, the compiler will still find it ambiguous.

[Giorgi]

What do you think about merging those two classes? In that case there will be only one extension method.

[Swimburger]

One way to circumvent this, which maybe something we should do anyway, is to merge the TwiMLResult for Minimal APIs and MVC.

This would be the MVC version implementing IActionResult:

namespace Twilio.AspNet.Core
{
    // ReSharper disable once InconsistentNaming
    public partial class TwiMLResult : IActionResult
    {
        ...

        public Task ExecuteResultAsync(ActionContext actionContext)
            => WriteTwimlToResponse(actionContext.HttpContext);

        private Task WriteTwimlToResponse(HttpContext httpContext)
        {
            if (Data == null)
            {
                Data = "<Response></Response>";
            }

            httpContext.Response.ContentType = "application/xml";
            httpContext.Response.ContentLength = Encoding.UTF8.GetByteCount(Data);
            return httpContext.Response.WriteAsync(Data);
        }
    }
}

And this would be the Minimal API version implementing IResult:

namespace Twilio.AspNet.Core;

public partial class TwiMLResult : IResult
{
    /// <summary>
    /// Writes the TwiML to the HTTP response body
    /// </summary>
    /// <param name="httpContext">The HttpContext containing the Response to write the TwiML to</param>
    public Task ExecuteAsync(HttpContext httpContext) => WriteTwimlToResponse(httpContext);
}

The latter would be conditionally compiled only for .NET 6 and up.

[Swimburger]

@Giorgi Yes, this is where my head is going too. I'm a little annoyed that we didn't do it in the first place. Doing it now will be a potentially breaking change.

[Giorgi]

Agreed, but it's better late than never. If you decide to go that way I can send a PR.

[Swimburger]

I think it's the way to go. @dprothero What do you think? We should probably wait for some more changes to come in before releasing v7, since we just release v6.

[dprothero]

I'm not following why this would be a breaking change?

[Swimburger]

It depends on what folks are doing in their code. If they explicitly use the type with namespace like this Twilio.AspNet.Core.MinimalApi.TwiMLResult, it'd be a breaking change since the type doesn't exist anymore.

I guess we could keep a class at Twilio.AspNet.Core.MinimalApi.TwiMLResult which inherits from Twilio.AspNet.Core.TwiMLResult but marked as [Obsolete] and remove it in the next major release, while keeping it for the minor release. However, that will also lead to ambiguous references.

There would also be some signature changes of methods to use the Twilio.AspNet.Core.TwiMLResult type.

All in all, minor and unlikely breaking changes, but definitely possible.

[Swimburger]

Also, the namespace Twilio.AspNet.Core.MinimalApi wouldn't be necessary anymore. The Minimal API extension methods should also be moved to Twilio.AspNet.Core.

[Giorgi]

However, that will also lead to ambiguous references.

It won't if the extension method will be available only on the main TwiMLResult class

[Swimburger]

@Giorgi I meant an ambiguous reference to the TwiMLResult class if there's one in both namespaces, not the extension methods.

[Swimburger]

@dprothero I added a branch releases/v7 which we can use to park breaking changes, while we can keep using main for non-breaking changes. Whenever a changes goes into main, we should be able to easily pull it into releases/v7 too. What do you think?

@Giorgi I think the best course of action is to do the right thing even if it's breaking change. So I'd merge the TwiMLResult class using partial classes and conditional compilation. Then I'd also move anything else out of the Twilio.AspNet.Core.MinimalApi namespace into the Twilio.AspNet.Core namespace. Then the new extension methods shouldn't be ambiguous anymore as well.

[Swimburger]

@Giorgi would you like me to move this forward?

[Giorgi]

Yes, I'll find time to update it.

[Swimburger]

@Giorgi I'd be happy to move this forward if you can't find the time. All good!

[Giorgi]

@Swimburger I'll try to do it this week.