Feedback on tus2 setup and configuration

[smatsson]

Hello my fellow community members! :)

As you might be aware there is currently a lot of effort put into creating tus2, the next major version of tus, and present it to IEFT to make it a "real" HTTP standard.

NOTE: This is the configuration to use when the new "tus2" protocol, currently being discussed on the IETF httpwg Github. Until tus2 has been standardized, this issue won't be merged. Link to draft and discussions:
https://datatracker.ietf.org/doc/draft-tus-httpbis-resumable-uploads-protocol/
https://mailarchive.ietf.org/arch/msg/httpbisa/tqUWc48_DArsMUf5xH6rLW8SAVY/

Since the tus2 protocol is a completely new protocol I have created a POC for it that includes many of the changes requested to tusdotnet over the years (e.g. using BodyReader as default, the controller pattern, endpoint routing etc). This requires the new tusdotnet version to have a completely new setup system and handling and I would love some feedback on the new way of doing things!

Readme with examples on how to use the new code can be found in the POC branch: https://github.com/tusdotnet/tusdotnet/tree/POC/tus2

Feel free to post any comments/ideas here or if you're shy, just drop me an email with your comments.

[oluatte]

Hey Stefan,

As always, thanks for doing this. I'm going to look at this some more, but capturing initial thoughts below:

• This seems a lot simpler, that can be hard to achieve, so well done!

• When using named configurations like so, I think the creation of the configuration should happen in the ConfigureServices method and not in Configure. Some reasons why:

  • Doing it in ConfigureServices might be the idiomatic way of doing this in dotnet (most other libraries including framework ones such as Authorization act this way).
  • This gives good access to configuration that is bound / registered in configure services and which may also be used elsewhere

• I like the storage and upload manager interfaces, they seems to combine many of the tus1 ixstore interfaces into one. Should be easier to use a blob store for the raw bytes and a db for the upload manager. I think this setup makes the idea of some kind of request handler pattern for the upload manager (get data on request start --> process --> save) more possible (would still love explicit notifications though).

• Is AllowClientToDeleteFile related to the explicit configuration of Tus protocol extensions that we have discussed elsewhere? If so, do we need to configure explicitly per endpoint again?

• Is it possible to use constructor injection for the TusHandler? When creating a custom handler that extends this one, it is difficult for someone looking at the code to know what dependencies it needs. It would be nice to be able to pass them into the constructor of the subclass and then: base(store, options, uploadmanager etc.).

  • Maybe this has to do with the storage factory pattern? Saw a mention of that in the readme, curious as to what benefit it brings over standard DI

Awesome stuff. Thanks for all your hard work.

[smatsson]

Thanks for the feedback and the kind words @mayoatte! I can't take all the credit though, most of the ideas for tus2 has come from the community in different forms.

When using named configurations like so, I think the creation of the configuration should happen in the ConfigureServices method and not in Configure. Some reasons why:
Doing it in ConfigureServices might be the idiomatic way of doing this in dotnet (most other libraries including framework ones such as Authorization act this way).

I agree that this config object is not very idiomatic. My idea was not to put configuration in it but rather use it as a "glue" object to map profile names to a specific endpoint. There have been some different ideas on how to do this, e.g in #148 (long read but fantastic discussions) including using attributes on the handler (called controller in #148). The downside to this is that each handler can only be used with a single profile as attributes are more or less "static". My gut feeling says that there are use cases where one wishes to reuse a handler between different endpoints but wishes to store data differently or allow deletes for one endpoint but not another. What do you think? Another pro with using a glue object is that we can extend the object in the future without adding breaking changes for everyone. Another idea that has been floating around is to remove the object and instead specify the data directly on the MapTus2 method, e.g.

endpoints.MapTus2<MyTusHandler>("/files-tus-2");
endpoints.MapTus2<MyTusHandler>("/files-tus-2-2", "MyStorageProfile");
endpoints.MapTus2<MyTusHandler>("/files-tus-2-3", "MyOtherStorageProfile", "MyUploadManagerProfile");

The above would simplify the configuration a bit. AllowClientToDeleteFile could be a property/method on the handler which would allow more dynamic configuration similar to how this is done in tus1. For instance, one could disallow deletes while the file is being processed or similar. What do you think?

I like the storage and upload manager interfaces, they seems to combine many of the tus1 ixstore interfaces into one. Should be easier to use a blob store for the raw bytes and a db for the upload manager. I think this setup makes the idea of some kind of request handler pattern for the upload manager (get data on request start --> process --> save) more possible (would still love explicit notifications though).

Oh definitely! I didn't add this in the POC but I think it would be a good idea to add something like OnRequestStart and OnRequestEnd to the handler. Good catch!

Is AllowClientToDeleteFile related to the explicit configuration of Tus protocol extensions that we have discussed elsewhere? If so, do we need to configure explicitly per endpoint again?

The spec is open for the server to allow or disallow deletes however it feels like. My initial idea was that this would be per endpoint. See my comment above for more reasoning on how it could be solved.

Is it possible to use constructor injection for the TusHandler? When creating a custom handler that extends this one, it is difficult for someone looking at the code to know what dependencies it needs. It would be nice to be able to pass them into the constructor of the subclass and then: base(store, options, uploadmanager etc.).

I considered constructor injection but I feel like it has some downsides. For one it would require breaking changes each time we need to add something to the base controller which is not ideal. If you look at how Microsoft solves this in MVC, there are a lot of property injection for controllers (e.g. HttpContext, User etc) which in my opinion gives a good separation between what I as a developer want to inject and "helpers" that are just there to make stuff easier. Another possible approach would be to inject a kind of TusContext object which would contain the things needed (storage, upload manager, httpcontext etc) but I don't think that would make it any clearer what is actually used and not.

Maybe this has to do with the storage factory pattern? Saw a mention of that in the readme, curious as to what benefit it brings over standard DI

There are use cases in tus1 where one wants to set things up depending on things in the current request, e.g. who the current user is. A very simple example is this one:

options.AddStorage("MyProfile", httpContext => new Tus2DiskStore(new()
{
    DiskPath = System.IO.Path.Combine(tus2Configuration.FolderDiskPath, "MyProfile", httpContext.User.Identity.Name)
}));

A more complex solution could be to resolve the path from a database or select a connection string based on the current user's tenant id or similar. I'm not sure if this is solvable in a good way using standard DI as these would be much more static once created.

Awesome stuff. Thanks for all your hard work.

Thank you!

[thesn10]

Uh, i think you posted your reply in the wrong issue :)

Also, i have some questions about how you want to move forward:

• Should i focus on discussing tus2, or do you wish to continue the discussion and the efforts to improve tus1?
• Will tus1 and tus2 share the same architecture or be two seperate code-bases?
  • If the former, are you gonna backport the architectural changes from tus2 to tus1 or the other way around?
  • If the latter, will the architecture of tus1 stay roughly the same or still be open to changes?

I'll try to give you feedback about tus2 as soon as possible! :)

[smatsson]

@Sngmng haha I totally did. So much for "answering this real quick between meetings" :D Thanks for pointing it out! I have moved my post to the correct issue.

Should i focus on discussing tus2, or do you wish to continue the discussion and the efforts to improve tus1?

I think tus2 is the future but tus1 will still be around for years to come. The tus group just recently posted our initial draft of tus2 to IEFT and our internal work has been going on for quite some time. Once adopted by IEFT anything can happen. Maybe ASP.NET Core will get built in support for tus2 in which case tusdotnet is obsolete. Only the future can tell :)

Will tus1 and tus2 share the same architecture or be two seperate code-bases?

Same code base (same lib) but probably in different namespaces. My hopes are that a backport of the tus2 architecture is possible while still supporting the events approach in tus1 (will probably not add that to tus2). Ideally there would be some kind of hybrid mode were a single endpoint can handle both tus1 and tus2 requests but I'm not sure if that is possible as the the protocols differ quite a lot. As an example, the storage implementation will differ as the stores need to support different functionality. Another example is the upload manager in tus2 which simply does not exist in tus1.

EDIT: So basically, it we can nail the architecture for tus2 I think it would be quite easy to backport while still keeping compatibility with the event based pattern that currently exist.

[thesn10]

The tus2 poc pretty good overall, good work!
A bit late but here's my feedback:

Configuration

Very nice i like it!

Maybe we could also provide typed configuration in addition to named configuration (just as the IHttpClientFactory does) like this:

services.AddTus2(options =>
{
    // Default configuration
    options.AddDiskStorage("/files");
    options.AddStorage(new Tus2DiskStore(new() { DiskPath = "/files" }));

    // Named configuration
    options.AddDiskStorage("MyProfile", "/files");
    options.AddStorage("MyProfile", new Tus2DiskStore(new() { DiskPath = "/files" }));

    // Typed configuration
    options.AddDiskStorage<MyHandler>("/files");
    options.AddStorage<MyHandler>(new Tus2DiskStore(new() { DiskPath = "/files" }));
});

AllowClientToDeleteFile could be a property/method on the handler which would allow more dynamic configuration similar to how this is done in tus1. For instance, one could disallow deletes while the file is being processed or similar. What do you think?

I think it should be a method/property on the handler. In the poc from #148 it was a method which you could override method:

public override async Task<FeatureSupportContext> GetOptions()
{
    return new FeaturSupportContext()
    {
        SupportedExtensions = new StoreExtensions() 
        {
            CreationWithUpload = true,
            Termination = false,
            ...
        }
        SupportedFeatures = ...
        SupportedChecksums = ...
    };
}

// or

public override async Task<FeatureSupportContext> GetOptions()
{
    var extensionInfo = await StorageClient.GetExtensionInfo(HttpContext.RequestAborted);
    extensionInfo.SupportedExtensions.Termination = false;

    return extensionInfo;
}

I dont think this is the best design, but its a starting point.

HttpContext callback

options.AddStorage("MyProfile", httpContext => new Tus2DiskStore(new()
{
    DiskPath = System.IO.Path.Combine(tus2Configuration.FolderDiskPath, "MyProfile", httpContext.User.Identity.Name)
}));

As you may know, i quite dislike this method of configuration, because it mixes application code with configuration and requires IHttpContextAccessor. In my opinion, this is the wrong way to do it.

Instead, if there is any dependency on HttpContext, the code should always be called from the handler/controller in my opinion. Here is how its done in #148:

// (working code example of the poc from #148)

[TusController]
public class MyTusController : TusControllerBase
{
    private readonly MyOptions _tusOptions;

    public MyTusController(IOptions<MyOptions> options)
    {
        _options = options.Value;
    }

    // For simplicity, i placed this method here, 
    // but in the real world this should be a method of some service or helper class
    public TusStorageClient GetStorage(string userName)
    {
        var store = new Tus2DiskStore(new()
        {
            DiskPath = System.IO.Path.Combine(_options.FolderDiskPath, "MyProfile", userName)
            // 
        });
        return TusStorageClient.Create(store);
    }

    public override async Task<ISimpleResult> FileCompleted(FileCompletedContext context)
    {
        var storage = GetStorage(User.Identity.Name); // "User" is a property of TusControllerBase which translates to HttpContext.User
        var file = await storage.Get(context.FileId, HttpContext.RequestAborted);

        // ...

        return Ok();
    }

    public override async Task<ISimpleResult> Delete(DeleteContext context)
    {
        var storage = GetStorage(User.Identity.Name);

        await storage.Delete(context, default, HttpContext.RequestAborted);

        return Ok();
    }
}

TusBaseHandlerEntryPoints

The biggest problem i see is that there is a dependency on TusStorage. This prevents users from creating the TusStorage on their own, each request (like in the example code above), or replacing it.

Also i think that each method should be moved to its own class (just like the IntentHandlers from tus1)

TusHandler

I think it looks good overall, but the methods look very hard to override without calling the base class, because there is too much tus-specific code:

tusdotnet/Source/tusdotnet/Tus2/TusHandler/TusHandler.cs

Lines 44 to 81 in 6ff071b

	public override async Task<UploadTransferProcedureResponse> OnWriteData(WriteDataContext writeDataContext)
	{
	try
	{
	await Storage.WriteData(Headers.UploadToken, writeDataContext);
	}
	catch (OperationCanceledException)
	{
	// Left blank. This is the case when the store does throws on cancellation instead of returning.
	}
	var uploadOffset = await Storage.GetOffset(Headers.UploadToken);
	if (writeDataContext.CancellationToken.IsCancellationRequested)
	{
	return new()
	{
	DisconnectClient = true
	};
	}
	if (Headers.UploadIncomplete == true)
	{
	return new()
	{
	UploadIncomplete = true,
	UploadOffset = uploadOffset
	};
	}
	await Storage.MarkComplete(Headers.UploadToken);
	return new()
	{
	UploadOffset = uploadOffset,
	UploadIncomplete = false,
	};
	}

I think the code (especially the extension specific code) should be inside a "bridge" class which bridges between the TusHandler and the TusStorage and is user friendly and DI friendly. (In #148, this bridge class was called TusStorageClient)

TusConfigurationManager

Very nicely done!

...except for the IHttpContextAcessor dependency which is needed for the httpContext callbacks :)

The problem with that is that the httpcontextaccessor dependency is always there, even if the user doesnt even use it.
I would just remove the httpcontext callbacks to solve this.

TusStorage

tusdotnet/Source/tusdotnet/Tus2/Storage/ITus2Storage.cs

Line 28 in 6ff071b

// TODO: Replace FileExist, GetOffset and IsComplete with something like "GetFileInfo"

Agreed

tusdotnet/Source/tusdotnet/Tus2/Storage/Tus2DiskStorage.cs

Line 13 in 6ff071b

// TODO: Use RandomAccess if possible

I actually tried to implement this 4 months ago :) You can view it here

Some additional thoughts

Make tus extensions open for extension (pun intended :))

In tus1, we have a lot of "if (extension supported)" kind of code:

public Task OnWrite()
{
    if (_storeAdapter.Extensions.Expiration)
    {
        // [extension code for write operation]
    }
    
    if (_storeAdapter.Extensions.Concatenation)
    {
        // ... 
    }
}

This violates the open-closed principle (open for extension, closed for modification) because we would have to modify the existing methods to add or change the behaivior of tus extensions.
What if we replace this with an more extendable approach for example:

public Task OnWrite()
{
    foreach (var extension in configuration.Extensions)
    {
        extension.OnWrite(tusHeaders, _storeAdapter);
    }
}

// and

public class ExpirationExtension : TusExtension
{
    public override Task OnWrite(TusHeaders headers, StoreAdapter store)
    {
        // [extension code for write operation]
    }
}

public class ConcatenationExtension : TusExtension { //... }

This would allow the easy addition of new extensions without changing any existing code.
What do you think of this?

[smatsson]

The tus2 poc pretty good overall, good work!
A bit late but here's my feedback:

Thank you! And thanks for the feedback. Your feedback is always appreciated even if we don't always agree ;)

Configuration
Very nice i like it!

🥳

Maybe we could also provide typed configuration in addition to named configuration (just as the IHttpClientFactory does) like this:

Interesting idea! Could you give an example on how this would look? Basically how the implementation and usage of options.AddDiskStorage<MyHandler>("/files"); would look like?

AllowClientToDeleteFile could be a property/method on the handler which would allow more dynamic configuration similar to how this is done in tus1. For instance, one could disallow deletes while the file is being processed or similar. What do you think?

Yeah it's a fair point. In tus2 there are no extensions but getting some kind of options object from the handler is probably a better architecture than using the "glue" object when running MapTus2.

As you may know, i quite dislike this method of configuration, because it mixes application code with configuration and requires IHttpContextAccessor. In my opinion, this is the wrong way to do it.

I know but don't agree :D Problem is that this is a real world use case where one want to fetch something from a database before setting everything up. Could be something like a tenant connection string to where to save data or similar.

Instead, if there is any dependency on HttpContext, the code should always be called from the handler/controller in my opinion.

I don't think this is a good approach as it requires the consumer to setup storage and upload manager. I would like the handler to be as easy to use as possible, a.k.a everything is already setup for you when your code is executed. A better approach might be to remove the dependency on the HttpContext and instead introduce a factory that can create both storage and upload manager. This way one could still implement it the same way if needed or just ignore it otherwise. I.e. someone needing the HttpContext to set things up could implement their own factory which takes a HttpContextAccessor as a constructor parameter. What do you think?

TusBaseHandlerEntryPoints
The biggest problem i see is that there is a dependency on TusStorage. This prevents users from creating the >TusStorage on their own, each request (like in the example code above), or replacing it.

I don't think that the user should be required to create their own storage instance. This should just work out of the box while still allowing the for a custom implementation to be used. I don't really see this as a problem. In my point of view the handler is the smallest part that you can either run using MapTus2 or inject into some other service. You can either use the handler or inject the storage and upload manager directly (and thus handle everything like disconnecting other uploads etc).

Also i think that each method should be moved to its own class (just like the IntentHandlers from tus1)

I think this would make it harder for the user to share state between methods and would work more like the events in tus1. Also it would have the same issue as the multiple interface implementation of ITusStore in tus1 where one needs to inject multiple interfaces in the same service to use it properly. I might be misunderstanding something though?

TusHandler
I think it looks good overall, but the methods look very hard to override without calling the base class, because > there is too much tus-specific code:

I took inspiration from your code but decided on to not include the extra abstraction between the handler and the StorageClient/"processor". Running base.WriteData instead of TusProcess.WriteData seems like a small diff. If you need to inject something into another service I think the handler is what should be injected as this is the container for all the tus2 code that should run. In regards to the "DI friendly" part - I agree. I think it would make more sense to move this code to here. This would require us to remove the "glue" object for MapTus2, which we want to do anyway, and replace it with something else. Maybe an attribute like in #186.

TusConfigurationManager
Very nicely done!

Thanks :) I know you hate the dependency on HttpContextAccessor but as previously said it's a vital part of real production user cases. I don't see the problem being dependent on an http context. Your are after all in an ASP.NET Core environment where the HttpContext is a vital part. Moving the factory part into its own implementation might solve this better. What do you think?

//TODO: Use RandomAccess if possible
I actually tried to implement this 4 months ago :) You can view it here

Thanks for the example! I implemented something similar as a POC but didn't find it that much faster than FileStreams on .NET6 (but honestly only tested it on Windows).

Some additional thoughts

Make tus extensions open for extension (pun intended :))

I think discussion would be better in #186 as there are no extensions in tus2. Would love to hear more about the idea so maybe we continue in #186?

---

I think some of my comments above are a bit hard to grasp out of context of the code. I'll see if I can get some time next week to implement some of the changes to better show what I mean. Once again, thanks for the feedback!

[smatsson]

Me: "I will totally get some time this week to work on tusdotnet"
Narrator: He didn't.

Changes made. Please note that I have not yet updated the readme in the POC branch.

TusBaseHandlerEntryPoints

This code has been moved to an internal class, Tus2HandlerInvoker, and TusBaseHandlerEntryPoints has been removed.

As pointed out the base class does not need to know about so much tus specific stuff. The code for each action has been moved into the new Tus2StorageFacade class.

TusHandler

Code has been changed to be possible to integrate better into DI. Constructor can take either a Tus2StorageFacade or a ITus2ConfigurationManager. The reason this is still passed to the TusHandler is to reduce the number of methods that needs to be overriden in a derived class (see OnlyCompleteTusHandler). Will possibly add an empty constructor to TusHandler that can be used in cases where one wants to override every single method and not rely on the base class.

EndpointConfiguration has been removed and replaced with a call to the TusHandler constructor (or can just be ignored if one wishes). AllowClientToDeleteFile has been moved to a virtual property in the TusHandler class

TusServiceBuilder

Named storage + ongoing upload manager has been removed. Possibility to add a StorageFactory instead which supports this behavior. Example storage factory has been added to the test site project (a.k.a in user code): SimpleTus2StorageFactory

OngoingUploadManager can no longer be split between different names. I don't see this being a major feature as the ongoing upload manager is responsible for synchronizing "locks" between requests and is thus very dependent on infrastructure. Thus I don't see there being multiple managers for the same cluster/machine. We probably want to add a way of adding a type of storage, e.g. builder.AddStorage<MyStorageType>() for it to integrate better with DI.

[smatsson]

I have updated the readme with new examples.

[edvinkuric]

Hello,
thank you for your contributions!

Are there any updates about adding this to the master-branch / releasing?
I would love to use tus2 with the endpoint-configuration.

BR, Edvin

[smatsson]

@edvinkuric This is related to tus2, the new protocol. It is currently being discussed on the IETF httpwg Github and won't be released until it's been standardized which can take some time :) I'll update the original post to make it clearer. https://github.com/httpwg/http-extensions/issues?q=is%3Aissue+is%3Aopen+resumable-upload

Maybe #147 is what you are looking for?