Changes in 2.3.0 break API

[claq2]

The changes in #61 added some optional parameters (e.g. Payload, Decode), which breaks compatibility with existing calling code. Calls to these methods generate MissingMethodExceptions. If you're following semantic versioning, the major version should be incremented. I suspect the intent wasn't to break compatibility, though, so perhaps overloads should be added and the 2.3.0 package should be pulled.

[dvsekhvalnov]

Hi @claq2 ,

sorry about that. Would you mind providing minimal test code to show issue?

[claq2]

I can give you a recreate scenario because you can't easily write a unit test for this:

1. Create a DLL that has a public call that calls into one of the 2.2 JWT methods that had an optional parameter added in 2.3, like Encode.
2. Create another bit of code, like an exe, that calls the public method in step 1. Add the reference to the step 1 DLL as a DLL reference, not a project reference.
3. Without recompiling the code in step 1, upgrade to jose-jwt 2.3 and try running the code in step 2.

That's how I found this. I was using a NuGet library called IdentityModel.Owin.PopAuthentication that calls JWT.Payload(System.String). When I upgraded my project to jose-jwt 2.3, it could no longer find a Payload method that takes only 1 parameter that is a string. Step 1 above simulates IdentityModel.Owin.PopAuthentication and step 2 simulates my app.

I suggest removing the optional parameters and adding overloads for the methods to restore compatibility, e.g.

    public static string Payload(string token)
    {
        return Payload(token, null);
    }

    public static string Payload(string token, JwtSettings settings)
    {
        byte[][] parts = Compact.Parse(token);

        if(parts.Length > 3)
        {
            throw new JoseException(
                "Getting payload for encrypted tokens is not supported. Please use Jose.JWT.Decode() method instead.");
        }

        return Encoding.UTF8.GetString(parts[1]);
    }

I might have some time to do this myself and submit a pull request, but if you'd rather keep the optional parameters and move the version to 3.0, then I won't try.

[dvsekhvalnov]

Thanks @claq2 , i'll play around to reproduce.

You specifically complaining about Payload method only? Other methods had optional parameters before.

Also as far as i can see IdentityModel.Owin.PopAuthentication depends on v1.9.1. Why do you want to update jose-jwt dependency without upgrading IdentityModel.Owin.PopAuthentication first?

[claq2]

I think Payload is the only one that IdentityModel.Owin.PopAuthentication uses. From a semantic versioning perspective, any change in the existing public calls of a library should bump the major version. All of the JWT and other public methods should look the same at they did in 2.0. If they've had optional parameters added since then, that's a breaking change. New overloads are non-breaking.

There is no update for IdentityModel.Owin.PopAuthentication available. It just happened that it worked with jose-jwt 2.2. By semver rules, it should then work with any 2.x version.

[dvsekhvalnov]

Ok, so you basically talking about binary compatibility where you can swap .dll without recompiling everything else?

I've never thought about binary compatibility, only trying to preserve source compatibility where you can recompile your project without changes to API calls.

On the other hand i don't mind if we can restore status quo here, i'll try to reproduce and see if it's possible to release 2.3.1 which brings binary compatibility back (probably just restore oldPayload method?).

[claq2]

It's not just the Payload method. Any public method that has changed since 2.0 needs to be adjusted, and anything that's been changed after 2.0 until now.
I'm going to try myself, too.

[claq2]

I tried to make 2 calls out of every call in JWT by removing the options settings values, but then the compiler doesn't know which call to use. E.g. if you have

public string Foo(string input)

and

public string Foo(string input, string input2 = null)

then this call is ambiguous:

Foo("blah");

If I make the settings value mandatory, then it must move to be before all of the optional parameters, which breaks both binary and source compatibility. So, there's no way to restore binary compatibility.

Perhaps the right approach is to add a disclaimer that the library does not maintain binary compatibility. Or declare a 3.0 version on the next change in the API and then maintain both compatibilities.

I'll close this because there's no way to get both compatibilities with the current code.

[dvsekhvalnov]

Thanks for input.

Still want to play with it myself. Probably make sense to start incorporating something like http://apichange.codeplex.com/ to get some tooling help?