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

Suggestion: Invert stream ownership for PUT requests #72

Open
faiteanu opened this issue Sep 27, 2022 · 0 comments
Open

Suggestion: Invert stream ownership for PUT requests #72

faiteanu opened this issue Sep 27, 2022 · 0 comments
Assignees

Comments

@faiteanu
Copy link

Hi,
first of all: thanks for the nice library!

I have one concern regarding the stream handling in PutHandler.cs

using (var fileStream = await document.CreateAsync(cancellationToken).ConfigureAwait(false))
{
var contentLength = _context.RequestHeaders.ContentLength;
if (contentLength == null)
{
_logger.LogInformation("Writing data without content length");
await data.CopyToAsync(fileStream).ConfigureAwait(false);
}
else
{
_logger.LogInformation("Writing data with content length {0}", contentLength.Value);
await Copy(data, fileStream, contentLength.Value, cancellationToken).ConfigureAwait(false);
}
}

Since you require document to provide a writeable stream, that makes usage in some scenarios unnecessarily difficult. Though it works well for the implemented file storage and memory storage, which both possess writeable streams, SQLiteBlobWriteStream.cs already looks cumbersome and needs to buffer the whole stream, defeating the purpose of streaming.

My suggestion is thus to pass the input stream to the document and let it handle the further processing. For the lines mentioned above, I used

var contentLength = context.RequestHeaders.ContentLength;
if (operation == PutOperation.Create)
{
    await document.CreateAsync(data, contentLength, cancellationToken).ConfigureAwait(false);
}
else
{
    await document.WriteAsync(data, startPosition ?? 0, contentLength, cancellationToken).ConfigureAwait(false);
}

This allows to write the following code (in shortened form). Note that the input stream is passed directly to the database, without requiring to buffer the whole file.

public async Task WriteAsync(Stream stream, int startPosition, int contentLength, CancellationToken ct)
{
    using (var connection = context.Database.GetDbConnection())
    {
        await connection.OpenAsync();
        using (var command = connection.CreateCommand())
        {
            command.CommandText = "UPDATE file_data SET file=@file WHERE id=@id";
            command.Parameters.AddWithValue("id", id);
            command.Parameters.AddWithValue("file", stream);
            await command.ExecuteNonQueryAsync(ct);
        }
    }
}

Since changing the method signatures of CreateAsync and WriteAsync is significant, it would break all previously implemented clients. I leave it up to you to decide whether to go this route.

Cheers, Fabian

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants