Open Acconut opened 3 years ago
This is a complex issue and I need some time to think things through. tusdotnet does the same thing as tusd (https://github.com/tusdotnet/tusdotnet/blob/master/Source/tusdotnet/TusProtocolHandlerIntentBased.cs#L96) as this is the way I interpret the current spec. I don't think it's clear from the current spec that the version is semver.
I think it all boils down to the following questions:
Looking at the 1.1 milestone it seems that some of the features have already been added to 1.0 so I guess the answer is "yes" to both of the questions above?
The way I see it we have a couple of options moving forward:
From the ones above I think number 1 is the best approach (without doing all to much thinking on the subject). There are two downsides as I see it. First that we haven't done this to begin with. If this was implemented from the get to we should have increased minor for each new extension added and minor/patch for all modifications to the spec, including changes to extensions. This is a problem now but might not be an issue once we start doing this and servers/clients adapt. Secondly, a minor, we would end up with versions similar to 1.24.152
which might be ugly/hard to understand
I think that we have a good chance of implementing option 1 above when we move to 1.1 as most (all?) servers and clients need to update the version check anyway. After this we could start updating version with each change, be it spelling fixes or new extensions. If we also tag each change in this repo with the new version it would be simple to check the exact version being used and what was included in that version.
I definitely think that the header has its value and should be kept as it's an easy way to determine if an incoming request is a tus request or not.
I don't think it's clear from the current spec that the version is semver.
That's a good point to improve if that's the case. The spec mentions SemVer but maybe we should improve the wording:
Version: 1.0.0 (SemVer) [..] Following SemVer, as of 1.0.0 tus is ready for general adoption.
Looking at the 1.1 milestone it seems that some of the features have already been added to 1.0
That's my fault. There have already been a few new features published on tus.io under the version 1.0 which should should actually have been in the 1.1 version. I always hoped to address them by a later release but due to the problem discussed in this issue, I postponed it for too long.
The way I see it we have a couple of options moving forward:
You are right with this list of options. There is also the possibility of increasing the tus version according to SemVer but report a different value in the Tus-Resumable header. For example, the tus protocol could be at v1.2.3 but the Tus-Resumable header contains the value v1.0.0 (as I mentioned in answer 2 in my original comment). Were you talking about this approach in your option 3?
From the ones above I think number 1 is the best approach
I agree that this would definitely be the correct approach since it solves the underlying issues instead of just "patching" the symptoms.
I definitely think that the header has its value and should be kept as it's an easy way to determine if an incoming request is a tus request or not.
Don't worry. The header is not going anywhere :)
@nigoroll Do you have an opinion on this topic?
I don't think it's clear from the current spec that the version is semver.
That's a good point to improve if that's the case. The spec mentions SemVer but maybe we should improve the wording:
Version: 1.0.0 (SemVer) [..] Following SemVer, as of 1.0.0 tus is ready for general adoption.
Seems like I completely missed that. You are absolutely correct! Given this we should just be able to increment minor/patch when new things are added. Maybe start with 1.1.x and update tusd, tusdotnet, ts-js-client etc?
The way I see it we have a couple of options moving forward:
You are right with this list of options. There is also the possibility of increasing the tus version according to SemVer but report a different value in the Tus-Resumable header. For example, the tus protocol could be at v1.2.3 but the Tus-Resumable header contains the value v1.0.0 (as I mentioned in answer 2 in my original comment). Were you talking about this approach in your option 3?
Yes pretty much. Another approach we could do is including both the "real" version (e.g. 1.2.3) and 1.0.0 for backwards compatibility. This way client's that aren't aware of later versions would still work as they find the 1.0.0 version in OPTIONS and then the server would correctly parse both 1.2.3 and 1.0.0. What do you think?
Given this we should just be able to increment minor/patch when new things are added. Maybe start with 1.1.x and update tusd, tusdotnet, ts-js-client etc?
Yes, we could do that.
Another approach we could do is including both the "real" version (e.g. 1.2.3) and 1.0.0 for backwards compatibility. This way client's that aren't aware of later versions would still work as they find the 1.0.0 version in OPTIONS and then the server would correctly parse both 1.2.3 and 1.0.0. What do you think?
So you mean we should have two headers? Tus-Resumable: 1.0.0
to make the tus 1.0 servers happy and Tus-Resumable2: 1.2.3
to allow tus 1.1+ servers to actually infer the tus version? Yes, that would be a way but I feel like this is "too patchy" because we add this new header to the protocol just to hide the mistakes take we made in the early implementations. When looking at it from that argument, fixating the Tus-Resumable
header to always be 1.0.0 is also "very patchy".
Maybe we should just stick with SemVer and fix our implementations, which would keep the protocol itself clean. Once all the fixed implementations have been widely adopted, the problem is gone and not relevant anymore. However, when we add a workaround to the protocol, we have to live with it forever. When having to decide between a limited, tough way and an unlimited, ugly way, I would choose the first one. When looking at the problem from this point of view, I think I changed my opinion and I would advocate leaving Tus-Resumable as it is and instead fix the implementation (of course, we should improve the wording in the specification to make version handling easier to understand).
Does that make sense?
I really like how the OpenAPI specification describes version handling: https://swagger.io/specification/#versions It very elaborate and detailed, making it easy for implementations to follow the specification. We could use that as an inspiration for tus.
Given this we should just be able to increment minor/patch when new things are added. Maybe start with 1.1.x and update tusd, tusdotnet, ts-js-client etc?
Yes, we could do that.
Another approach we could do is including both the "real" version (e.g. 1.2.3) and 1.0.0 for backwards compatibility. This way client's that aren't aware of later versions would still work as they find the 1.0.0 version in OPTIONS and then the server would correctly parse both 1.2.3 and 1.0.0. What do you think?
So you mean we should have two headers?
Tus-Resumable: 1.0.0
to make the tus 1.0 servers happy andTus-Resumable2: 1.2.3
to allow tus 1.1+ servers to actually infer the tus version? Yes, that would be a way but I feel like this is "too patchy" because we add this new header to the protocol just to hide the mistakes take we made in the early implementations. When looking at it from that argument, fixating theTus-Resumable
header to always be 1.0.0 is also "very patchy".
It's an interesting idea but I agree that it is very patchy. What I meant was that we could still include 1.0.0
in the OPTIONS response so that older clients would still work. Let's say that we have a server that is using proper semver and that the latest version is 1.2.3
. Any client providing Tus-Resumable: 1.0.0
would work as per semver. If the client does a equal check for version (e.g TusVersionHeader.Contains('1.0.0')
) it would still fail to communicate with the server if the server only sends back Tus-Version: 1.2.3
in the OPTIONS response. A way of circumventing this would be to always include the latest "major version" a swell as the real version:
HTTP/1.1 204 No Content
Tus-Resumable: 1.2.3
Tus-Version: 1.0.0,1.2.3
Maybe we should just stick with SemVer and fix our implementations, which would keep the protocol itself clean. Once all the fixed implementations have been widely adopted, the problem is gone and not relevant anymore. However, when we add a workaround to the protocol, we have to live with it forever. When having to decide between a limited, tough way and an unlimited, ugly way, I would choose the first one. When looking at the problem from this point of view, I think I changed my opinion and I would advocate leaving Tus-Resumable as it is and instead fix the implementation (of course, we should improve the wording in the specification to make version handling easier to understand).
Does that make sense?
We're on the same page here 👍
I really like how the OpenAPI specification describes version handling: https://swagger.io/specification/#versions It very elaborate and detailed, making it easy for implementations to follow the specification. We could use that as an inspiration for tus.
This is pretty much how I see how we should move forward with the version in tus. :)
What I meant was that we could still include 1.0.0 in the OPTIONS response so that older clients would still work.
Interesting idea but we both agree that fixing the implementations is likely the best solution.
We're on the same page here :+1: This is pretty much how I see how we should move forward with the version in tus. :)
Great! Unless @kvz (who I also asked for feedback) has an opposing view, I will start working on a draft to explain the version handling better.
Seems fair to me :ok_hand:
Just a quick update: I didn't come around to write the draft yet. I will do so next week!
Right now, the specification says following about the Tus-Resumable header:
The idea was that Tus-Resumable is used by the Server to know what version of the protocol the Client is using, similar to how
HTTP/1.0
orHTTP/1.1
is included in HTTP requests.However, most Servers implement the version check by simply testing if the Tus-Resumable header matches exactly
1.0.0
since this is the only published version of tus 1.x (I hope to release 1.1 soon but the issue I am describing here has prevented us from doing so). I am also guilty of this, since tusd implements this naive check as well: https://github.com/tus/tusd/blob/26b84bcb1c818f95ea236fbfe1c43e47c32574ab/pkg/handler/unrouted_handler.go#L264. This check has not been an issue in the past but will be if we publish a new version of the protocol:So, let's assume tus 1.1 is available and a new client supporting tus 1.1 wants to contact a tus 1.0 server. According to the current specification, the client must set
Tus-Resumable: 1.1.0
but the server only acceptsTus-Resumable: 1.0.0
and will reject the request. In turn, this makes it impossible for the tus 1.1 client to communicate with the tus 1.0 server which should be possible since tus 1.1 must not be a breaking change. In theory, you could argue that the specification implies that the server must parse the Tus-Resumable header as a SemVer declaration (https://semver.org/) and then check if the client's version is compatible with the server's version. With this understanding we could say that servers, such as tusd, do not correctly implement the specification and must be changed.However, we must also acknowledge that there is a vast collection of tus servers in production that use this incorrect version check. These servers may not be updated to use a correct version check in the near future making it impossible to use them with tus 1.1 clients once they are available. We should avoid such a situation if possible. I hope this problem is understandable. If not, please let me know.
This leads to following question:
I see following possible answers: 1) Keep the current definition and adjust the implementations. The implementations have a wrong version check and so they must be addressed instead of changing the protocol. 2) Change the specification to only include the major version in Tus-Resumable, i.e. only
1.0.0
,2.0.0
and so on. This makes the current naive version check compatible with the specification. Clients always setTus-Resumable: 1.0.0
and Servers always accept that. Now, what if a client wants to use a new feature in tus 1.1? How does a server know that it should handle the request according to tus 1.1? I would argue that the server does not need the Tus-Resumable header to determine that. Since tus 1.1 is compatible with tus 1.0, a client must always use a specific header or a specific extension to use a new feature. A tus 1.0 server will simply ignore these headers or extensions since it does not know them. A tus 1.1 on the other hand, recognizes these headers or extension and therefore knows what the client is talking about. The Tus-Resumable header is not needed in this scenario.Are there any other possible solution that I forgot? Or is my argumentation flawed? I am happy about any feedback!
This issue has been brought up before (see https://github.com/tus/tus-resumable-upload-protocol/issues/96; I laid out similar reasons in the original comment there) but the discussion never really ended in an agreement (that's my fault) so I wanted to bring it back to hopefully resolve it this time.
/cc @nigoroll @smatsson