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

Future definition of the Tus-Resumable header #165

Open
Acconut opened this issue Sep 18, 2020 · 10 comments
Open

Future definition of the Tus-Resumable header #165

Acconut opened this issue Sep 18, 2020 · 10 comments

Comments

@Acconut
Copy link
Member

Acconut commented Sep 18, 2020

Right now, the specification says following about the Tus-Resumable header:

The Tus-Resumable header MUST be included in every request and response except for OPTIONS requests. The value MUST be the version of the protocol used by the Client or the Server.
If the version specified by the Client is not supported by the Server, it MUST respond with the 412 Precondition Failed status and MUST include the Tus-Version header into the response. In addition, the Server MUST NOT process the request.

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 or HTTP/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 accepts Tus-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:

Do we want to keep the current definition of Tus-Resumable and cause compatibility issues between tus 1.1 clients and tus 1.0 servers? Or do we want to change the definition and avoid these issues?

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 set Tus-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

@smatsson
Copy link

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:

  1. Is a client using 1.0 able to talk to a server with 1.1?
  2. Is a client using 1.1 able to talk to a server with 1.0?

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:

  1. Use semver properly, each change (even fixed misspellings) is a version bump and force all servers and clients to update to this behavior. The downside here is that we have not done this from the beginning so there is some fragmentation between different servers supporting 1.0. This is somewhat protected by the extensions concept as most additions have been added to extesions. A real life example on when this could have helped is How to handle change with empty values for metadata? #149 which caused a semi-breaking change in tusdotnet while not being reflected in the protocol version.
  2. Leave it as is. This has the same problem with fragmentations as 1 but from exeperience I think this one will get worse and worse over time until no-one knows what is supported by 1.0 anymore.
  3. Only use version bumps for majors. This might not be desirable as it gives the impression that all changes are breaking causing slow adaptation.

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.

@Acconut
Copy link
Member Author

Acconut commented Sep 28, 2020

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?

@smatsson
Copy link

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?

@Acconut
Copy link
Member Author

Acconut commented Sep 28, 2020

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?

@Acconut
Copy link
Member Author

Acconut commented Sep 29, 2020

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.

@smatsson
Copy link

smatsson commented Sep 30, 2020

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".

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 👍

@smatsson
Copy link

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. :)

@Acconut
Copy link
Member Author

Acconut commented Oct 1, 2020

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 👍
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.

@kvz
Copy link
Member

kvz commented Oct 1, 2020

Seems fair to me 👌

@Acconut
Copy link
Member Author

Acconut commented Oct 14, 2020

Just a quick update: I didn't come around to write the draft yet. I will do so next week!

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

No branches or pull requests

3 participants