[FEATURE] #563 Add implementation identifier to compatibility check

[tobous]

trafficstars

Adds the implementation identifier as a field to the Saros version transferred and checked before the session negotiation. This identifier can be used to check whether the IDE the Saros instance is running on matches to prohibit cross-IDE sessions, even if the version number matches.

This logic will have to be adjusted once IDE cross-compatibility has been added. At that point, it would probably be best to create a static list of known compatible Saros implementations.

Adds the new Saros plugin context value 'SarosImplementation'. It is expected to be injected by IDE-specific implementations, providing the corresponding implementation identifier. Subsequently adjusted the Eclipse and IntelliJ IDEA implementation as well as the Saros Server to inject such an identifier.

Adjusted the version compatibility test to include cases for differing implementation identifiers. Also adjusted the existing tests to use the new version format.

Resolves #563.

[srossbach]

Maybe it was not that smart to get rid of the compability chart.

So here is Saros: Eclipse X.Y.Z And here is Saros: IntelliJ A.B.C

All checks are basically hardcoded so if i want to establish cross compability I have to use the same "implementation" (what implementation ?!) qualifier.

[tobous]

Maybe it was not that smart to get rid of the compability chart.

So here is Saros: Eclipse X.Y.Z And here is Saros: IntelliJ A.B.C

All checks are basically hardcoded so if i want to establish cross compability I have to use the same "implementation" (what implementation ?!) qualifier.

The implementation qualifier is just what I called the shorthand used to identify the implementation. For saros/I, it would be "I", for saros/E "E", etc. This part is appended to the front of the Version number, separated using "/", e.g. E/16.1.0

Yes, once we want to allow ide cross compatibility, adding a compatibility list would be best. There are two possible approaches for this: specifying explicit versions that are compatible or specifying ide implementations that are compatible. With the first option, it would only allow compatibility for the exact versions. With the second option, it would allow for any versions compatible according to the rest of the compatibility scheme (i.e. with a matching major and minor version number).

[srossbach]

1. I do not think it is that smart to break compatibility on the logic that is responsible for checking the compatibility, i.e you should ensure that the changed data can still be correctly evaluated by older versions. In you current approach no side can that because you append the implementation/platform id to the front of the "old" version string format.

2. As this solution is not polished yet can we set this patch on hold for now until all requirements are known ?

[tobous]

1. I do not think it is that smart to break compatibility on the logic that is responsible for checking the compatibility, i.e you should ensure that the changed data can still be correctly evaluated by older versions. In you current approach no side can that because you append the implementation/platform id to the front of the "old" version string format.

I could add it to the end so that the old versions see it as part of the qualifier, but that would have the disadvantages that

• it is harder for the user to find the important information contained in the version string that way (the qualifier might be quite long),
• we would have to add an additional requirement and check that the qualifier does not contain the separator character used for the implementation identifier, and
• actual version incompatibilities might not be detected if they fall into the range of the "compatibility" (as the old version will not have restriction for the separator character in place, it could theoretically use a qualifier shaped like the new version string, leading to a false-positive compatibility result).

While this does break the parsing logic, meaning the version will be read as invalid/0.0.0.invalid or 0.0.0.invalid respectively, it does not break the compatibility check. As the version string can't be parsed, the version is detected as incompatible and the invitation is aborted. This is also displayed to the user.

While this "invalid" version might be a bit confusing, the general hint that the versions are incompatible should still be understandable. And, if the user is unsure about this message, they would hopefully look at the compatibility section on the Saros page or ask a question on Gitter/the mailing list. Additionally, this is only an issue for the transition period.

2. As this solution is not polished yet can we set this patch on hold for now until all requirements are known ?

I don't know what other requirements there could be. And in my mind, the current logic does everything it is supposed to do: Ensure that different IDE implementations are not seen as compatible, even if the version string matches.

Adding a compatibility list for different implementations is a completely different matter in my opinion.

[srossbach]

Can you tell me how to test the Saros Server against a client (Saros/E or Saros/I) after this patch ?

[tobous]

Can you tell me how to test the Saros Server against a client (Saros/E or Saros/I) after this patch ?

Good point. Did not think of that. (Currently, it doesn't work anyway since the Server still uses version 15.0.0.)

The server makes this really complicated as it is mimicking the normal client without having the same restrictions. Hard-coding that the server is always compatible seems bad practice to me, especially since the core can't access the constant that defines the Server implementation identifier. Will think about it.

[srossbach]

To keep backward compatibility I would suggest to use a new field in the map, post an invalid version in the former field to old clients will correctly see the new client as not compatible and only evaluate the new field in the new client.

[tobous]

Rebased onto current base branch without any interaction.

[tobous]

To keep backward compatibility I would suggest to use a new field in the map, post an invalid version in the former field to old clients will correctly see the new client as not compatible and only evaluate the new field in the new client.

Do you mean changing the key used to transfer the version string? I am not sure what this would accomplish. With these changes, the mismatch in the format between the old and the new client is already correctly detected as an invalid version on both sides/in both directions. As the new "old version" would also have to be invalid to always be seen as incompatible, this would not really gain us much. The incompatible version string would still be displayed for the old clients. And while I could add a specific handling in the new client displaying a more helpful error message, I am not really sure this would improve the usability by much.

Or do you mean that the implementation qualifier should not be added to the version string but should rather be transferred as a separate value? That would indeed be an option. It would also simplify the version parsing logic. The only drawback would be that this approach would not ensure that older clients see newer clients with a different implementation qualifier as incompatible in cases where the version number match.

[tobous]

Rebased onto current base branch without any interactions.

[tobous]

Rebased onto current master without any interactions.