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.

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

Jump to bottom

Add FromFile to BinaryData #107231

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add FromFile to BinaryData #107231

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0f71345
Add FromFile to BinaryData
manandre Sep 1, 2024
96c090b
Cleanup temporary files
manandre Sep 2, 2024
c31e500
Use FileStream async mode
manandre Sep 4, 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
4 changes: 4 additions & 0 deletions src/libraries/System.Memory.Data/ref/System.Memory.Data.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ public BinaryData(string data, string? mediaType) { }
public static System.BinaryData FromBytes(byte[] data, string? mediaType) { throw null; }
public static System.BinaryData FromBytes(System.ReadOnlyMemory<byte> data) { throw null; }
public static System.BinaryData FromBytes(System.ReadOnlyMemory<byte> data, string? mediaType) { throw null; }
public static System.BinaryData FromFile(string path) { throw null; }
public static System.BinaryData FromFile(string path, string? mediaType) { throw null; }
public static System.Threading.Tasks.Task<System.BinaryData> FromFileAsync(string path, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public static System.Threading.Tasks.Task<System.BinaryData> FromFileAsync(string path, string? mediaType, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresDynamicCodeAttribute("JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation.")]
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("JSON serialization and deserialization might require types that cannot be statically analyzed.")]
public static System.BinaryData FromObjectAsJson<T>(T jsonSerializable, System.Text.Json.JsonSerializerOptions? options = null) { throw null; }
Expand Down
76 changes: 76 additions & 0 deletions src/libraries/System.Memory.Data/src/System/BinaryData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,82 @@ private static async Task<BinaryData> FromStreamAsync(Stream stream, bool async,
}
}

/// <summary>
/// Creates a <see cref="BinaryData"/> instance from the specified file.
/// </summary>
/// <param name="path">The path to the file.</param>
/// <returns>A value representing all of the data from the file.</returns>
public static BinaryData FromFile(string path)
{
if (path is null)
{
throw new ArgumentNullException(nameof(path));
}

return FromFileAsync(path, async: false).GetAwaiter().GetResult();
}

/// <summary>
/// Creates a <see cref="BinaryData"/> instance from the specified file
/// and sets <see cref="MediaType"/> to <see pref="mediaType"/> value.
/// </summary>
/// <param name="path">The path to the file.</param>
/// <param name="mediaType">MIME type of this data, e.g. <see cref="MediaTypeNames.Application.Octet"/>.</param>
/// <returns>A value representing all of the data from the file.</returns>
/// <seealso cref="MediaTypeNames"/>
public static BinaryData FromFile(string path, string? mediaType)
{
if (path is null)
{
throw new ArgumentNullException(nameof(path));
}

return FromFileAsync(path, async: false, mediaType).GetAwaiter().GetResult();
}

/// <summary>
/// Creates a <see cref="BinaryData"/> instance from the specified file.
/// </summary>
/// <param name="path">The path to the file.</param>
/// <param name="cancellationToken">A token that may be used to cancel the operation.</param>
/// <returns>A value representing all of the data from the file.</returns>
public static Task<BinaryData> FromFileAsync(string path, CancellationToken cancellationToken = default)
{
if (path is null)
{
throw new ArgumentNullException(nameof(path));
}

return FromFileAsync(path, async: true, cancellationToken: cancellationToken);
}

/// <summary>
/// Creates a <see cref="BinaryData"/> instance from the specified file
/// and sets <see cref="MediaType"/> to <see pref="mediaType"/> value.
/// </summary>
/// <param name="path">The path to the file.</param>
/// <param name="mediaType">MIME type of this data, e.g. <see cref="MediaTypeNames.Application.Octet"/>.</param>
/// <param name="cancellationToken">A token that may be used to cancel the operation.</param>
/// <returns>A value representing all of the data from the file.</returns>
/// <seealso cref="MediaTypeNames"/>
public static Task<BinaryData> FromFileAsync(string path, string? mediaType,
CancellationToken cancellationToken = default)
{
if (path is null)
{
throw new ArgumentNullException(nameof(path));
}

return FromFileAsync(path, async: true, mediaType, cancellationToken);
}

private static async Task<BinaryData> FromFileAsync(string path, bool async,
string? mediaType = default, CancellationToken cancellationToken = default)
{
using FileStream fileStream = File.OpenRead(path);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to use File.ReadAllBytes? Since it has that tries to minimize allocations of the byte array if the file-size is determinable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question!

  • The file-size optimization is also supported in the FromStream flow.

  • The ReadAllBytesAsync method is not available for the net462 and netstandard2.0 TFMs. Can we then fallback to the synchronous implementation for these TFMs?

  • The ReadAllBytes{Async} flow may be more efficient to avoid the intermediate copy buffer of the CopyTo{Async} flow.

Here would be the corresponding code:

        private static
#if NET6_0_OR_GREATER
        async
#endif
        Task<BinaryData> FromFileAsync(string path, bool async, string? mediaType = default, CancellationToken cancellationToken = default)
        {
#if NET6_0_OR_GREATER
            byte[] bytes;

            if (async)
            {
                bytes = await File.ReadAllBytesAsync(path, cancellationToken).ConfigureAwait(false);
            }
            else
            {
                bytes = File.ReadAllBytes(path);
            }

            return new BinaryData(bytes, mediaType);
#else
            byte[] bytes = File.ReadAllBytes(path);

            return Task.FromResult(new BinaryData(bytes, mediaType));
#endif
        }

Better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see. Thanks for looking into it. Personally I don't think the potential benefit outweighs the added complexity, I would keep it as it is then.

Copy link
Member

Choose a reason for hiding this comment

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

The FileStream should be opened for asynchronous use when async: true. The way this is written, it'll always use synchronous I/O, which means async operations on it will end up being sync calls queued to the pool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with the FileStream constructor enabling async calls.

return await FromStreamAsync(fileStream, async, mediaType, cancellationToken).ConfigureAwait(false);
}

/// <summary>
/// Creates a <see cref="BinaryData"/> instance by serializing the provided object using
/// the <see cref="JsonSerializer"/>
Expand Down
85 changes: 84 additions & 1 deletion src/libraries/System.Memory.Data/tests/BinaryDataTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ public async Task CanCreateBinaryDataFromNonSeekableStream()
public async Task CanCreateBinaryDataFromFileStream()
{
byte[] buffer = "some data"u8.ToArray();
using FileStream stream = new FileStream(Path.GetTempFileName(), FileMode.Open);
using FileStream stream = new FileStream(Path.GetTempFileName(), FileMode.Open, FileAccess.ReadWrite, FileShare.None, 4096, FileOptions.DeleteOnClose);
stream.Write(buffer, 0, buffer.Length);
stream.Position = 0;
BinaryData data = BinaryData.FromStream(stream);
Expand Down Expand Up @@ -431,6 +431,73 @@ public void MaxStreamLengthRespected()
var data = BinaryData.FromStream(new OverFlowStream(offset: int.MaxValue - 1000));
}

[Fact]
public async Task CanCreateBinaryDataFromFile()
{
byte[] buffer = "some data"u8.ToArray();
string path = Path.GetTempFileName();
try
{
File.WriteAllBytes(path, buffer);
BinaryData data = BinaryData.FromFile(path);
Assert.Equal(buffer, data.ToArray());

byte[] output = new byte[buffer.Length];
var outputStream = data.ToStream();
outputStream.Read(output, 0, (int)outputStream.Length);
Assert.Equal(buffer, output);

data = await BinaryData.FromFileAsync(path);
Assert.Equal(buffer, data.ToArray());

outputStream = data.ToStream();
outputStream.Read(output, 0, (int)outputStream.Length);
Assert.Equal(buffer, output);
}
finally
{
File.Delete(path);
}
}

[Theory]
[InlineData(null)]
[InlineData("")]
[InlineData(MediaTypeNames.Application.Soap)]
public async Task CanCreateBinaryDataFromFileWithMediaType(string? mediaType)
{
byte[] buffer = "some data"u8.ToArray();
string path = Path.GetTempFileName();
try
{
File.WriteAllBytes(path, buffer);
BinaryData data = BinaryData.FromFile(path, mediaType);
Assert.Equal(buffer, data.ToArray());
Assert.Equal(mediaType, data.MediaType);

byte[] output = new byte[buffer.Length];
var outputStream = data.ToStream();
outputStream.Read(output, 0, (int)outputStream.Length);
Assert.Equal(buffer, output);

data = await BinaryData.FromFileAsync(path, mediaType);
Assert.Equal(buffer, data.ToArray());
Assert.Equal(mediaType, data.MediaType);

outputStream = data.ToStream();
outputStream.Read(output, 0, (int)outputStream.Length);
Assert.Equal(buffer, output);

//changing the backing buffer should not affect the BD instance
buffer[3] = (byte)'z';
Assert.NotEqual(buffer, data.ToMemory().ToArray());
}
finally
{
File.Delete(path);
}
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBuiltWithAggressiveTrimming))]
public void CanCreateBinaryDataFromCustomType()
{
Expand Down Expand Up @@ -510,6 +577,22 @@ public async Task CreateThrowsOnNullStream()
Assert.Contains("stream", ex.Message);
}

[Fact]
public async Task CreateFromFileThrowsOnNullPath()
{
var ex = Assert.Throws<ArgumentNullException>(() => BinaryData.FromFile(null));
Assert.Contains("path", ex.Message);

ex = Assert.Throws<ArgumentNullException>(() => BinaryData.FromFile(null, null));
Assert.Contains("path", ex.Message);

ex = await Assert.ThrowsAsync<ArgumentNullException>(() => BinaryData.FromFileAsync(null));
Assert.Contains("path", ex.Message);

ex = await Assert.ThrowsAsync<ArgumentNullException>(() => BinaryData.FromFileAsync(null, null));
Assert.Contains("path", ex.Message);
}

[Fact]
public void CreateThrowsOnNullString()
{
Expand Down
Loading