-
Notifications
You must be signed in to change notification settings - Fork 102
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
Future definition of the Tus-Resumable header #165
Comments
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 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. |
That's a good point to improve if that's the case. The spec mentions SemVer but maybe we should improve the wording:
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.
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?
I agree that this would definitely be the correct approach since it solves the underlying issues instead of just "patching" the symptoms.
Don't worry. The header is not going anywhere :) @nigoroll Do you have an opinion on this topic? |
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?
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? |
Yes, we could do that.
So you mean we should have two headers? 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. |
It's an interesting idea but I agree that it is very patchy. What I meant was that we could still include
We're on the same page here 👍 |
This is pretty much how I see how we should move forward with the version in tus. :) |
Interesting idea but we both agree that fixing the implementations is likely the best solution.
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 👌 |
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.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 #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
The text was updated successfully, but these errors were encountered: