Fix untestable extension methods

[davidfowl]

• https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNetCore.Http.Extensions/SendFileResponseExtensions.cs
• https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNetCore.Http.Extensions/FormFileExtensions.cs

These methods demand that files be on disk. What makes it worse is that these are extension methods so they cannot be stubbed.

Maybe we should make these virtual on HttpResponse and IFormFile (interface method).

/cc @Eilon @Tratcher @lodejard

[glennc]

If we are going to do this we should do it now, moving methods around after RC2 should be avoided.

[davidfowl]

another alternative might be to add an overload that takes IFileInfo

[davidfowl]

Ughm IFileInfo is read only...

[khellang]

another alternative might be to add an overload that takes IFileInfo

Add an overload where? For SendFile?

What about the FormFileExtensions? Promote them to IFormFile?

[davidfowl]

@khellang Those are the 2 options. Either promote to interface or use some overload that takes a writable file abstraction.

[davidfowl]

For SendFile, we should allow passing IFileInfo

[muratg]

Moving this to Backlog as we will be in RC2 ask mode very soon. If you feel strongly about this issue, please ping me.

[lodejard]

On the Form file extensions, why not Task SaveAsync(Stream target)?

The IFileInfo for SendFile is good though - because if it's has a non-null-or-empty PhysicalFilePath some servers can accelerate that.

[khellang]

I think Task SaveAsync(Stream target) makes sense, then the Task SaveAsync(string path) overloads could be extension methods again 😛

[davidfowl]

Only problem with extension methods is that it reintroduces the original problem if you ever use them... Which is what the original problem and why this bug was filed 😄

[khellang]

Only problem with extension methods is that it reintroduces the original problem if you ever use them

Yes and no. You can still mock the SaveAsmethod on the interface, which is an improvement.

[khellang]

Also, if it's changed to a Stream, does the SaveAs name make sense?

[davidfowl]

If we add Save(Stream target) then there's no point adding an untestable extension method.

[khellang]

So no point in adding a convenience method for saving to a path? Like

public static void SaveAs(this IFormFile file, string path)
{
    using (var fileStream = File.Create(path, DefaultBufferSize))
    {
        file.Save(fileStream);
    }
}

[davidfowl]

It just means you can't test it is all.

[lodejard]

It's better in general for devs to call File.Create for themselves. Otherwise you'll end up with some (but not all) of the APIs that take Stream having a string overload for convenience, but you don't know what flags they pass. What file sharing for example? Fail if file exists? Those details may not be consistent if you took a look at how the various convenience methods create files on your behalf...

[Tratcher]

Wouldn't that be CopyTo[Async](Stream)? Save implies the destination is a file, which is no longer necessary.