Protobuf: Binary compatibility broken in Google.Protobuf by introduction of Proto2 features

[jskeet]

This may be the case for other types as well, but at least FieldCodec introduced binary-incompatible changes in 3.9.0, in this commit.

Each FieldCodec static factory method gained a default parameter:

public static FieldCodec<string> ForString(uint tag)

became

public static FieldCodec<string> ForString(uint tag, string defaultValue = "")

This is a binary breaking change. Code compiled against an earlier version of Google.Protobuf fails at execution time when using the new version, as it can't find the method. We should have added overloads instead of default parameters here.

[ObsidianMinor]

This was my mistake, I'll fix it. To be 100% clear, reintroducing the previous method will make the next release binary-compatible, right?

[jskeet]

I haven't looked over all the changes - I only found that one. Ideally, you should take a copy of the public API at an earlier version, then take a copy of the public API at a later version, and diff them. Introducing a new overload and then making the currently-optional parameter required will change the compile-time behaviour (in that it will resolve to the single-parameter method rather than the two-parameter method) but that's unlikely to break anyone.

The public API generator tool at https://github.com/JakeGinnivan/ApiApprover could help with this.

[ObsidianMinor]

Alright I ran the tool and found the FieldCodec factories are the only breaking API changes (beyond methods introduced on interfaces that should only be implemented by internal types).

[jskeet]

As a thought, I wonder whether we should delist 3.9.0 from NuGet for now, then release 3.9.1 with the fix when it's ready to release. That will reduce the chances of other people accidentally taking a dependency on 3.9.0 which would cause diamond dependency issues.

@anandolee Are you happy to do that?

[anandolee]

Delisted. Thanks

Sorry for late reply, just back from vacation

[anandolee]

After a second thought, I have a question for the compatibility. Is the compatibility break that new generated code can not compile with old C# runtime? Protobuf compatibility does not gurantee such case. We only consider old generated code not work with new runtime as breaking change

[ObsidianMinor]

The break is when you use the 3.9.0 runtime with old generated code. The issue was noticed when a Google Cloud Platform lib using generated code from 3.7.0 used Google.Protobuf 3.9.0 and attempted to run (it couldn't since a parameter was added to each of the FieldCodec factory methods). @anandolee

[jtattermusch]

Because we already plan to release v3.9.1 for C#, there's one more backwards compatibility item that should be fixed:
In https://github.com/protocolbuffers/protobuf/pull/4642/files#diff-067e1b56cdf9c678137370d294a19f7d, a HasValue method was added to IFieldAccessor interface, but adding a method to a pre-existing interface is a breaking change.
A fix for that proposed in https://github.com/protocolbuffers/protobuf/pull/5936, but we should fix in the 3.9.x release branch first to avoid the breaking change from reaching the users (#4642 was only merged recently and is not present in v3.8.x)
@ObsidianMinor can you create a separate PR for removing HasValue from IFieldAccessor?

[TeBoring]

In that case, seems we are missing a backward compatibility test?
Can we update the test to catch this issue?

[jtattermusch]

Because we already plan to release v3.9.1 for C#, there's one more backwards compatibility item that should be fixed:
In https://github.com/protocolbuffers/protobuf/pull/4642/files#diff-067e1b56cdf9c678137370d294a19f7d, a HasValue method was added to IFieldAccessor interface, but adding a method to a pre-existing interface is a breaking change.
A fix for that proposed in #5936, but we should fix in the 3.9.x release branch first to avoid the breaking change from reaching the users (#4642 was only merged recently and is not present in v3.8.x)
@ObsidianMinor can you create a separate PR for removing HasValue from IFieldAccessor?

Ok, I was wrong, the HasValue addition has been there since 3.7.0 release which is a long time ago. Despite this is technically a breaking change, it has slipped through the cracks, and because we can't change the past and no one has complained about this change so far, I think we can just leave it there as is (and we won't need the IFieldPresenceAccessor proposed in #5936)

[jtattermusch]

@TeBoring testing backward API compatibility in an automated fashion is hard.
There is a set of rules for C# we should follow https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/breaking-change-rules.md, but it's hard to check for all of those in an automated fashion (you'd need to take an older version of protobuf and a newer version of protobuf and compare the APIs according to those rules).
I don't think we have any such tests for any of the protobuf languages - we just need to be more careful when reviewing newly added APIs (they are not added very often, the proto2 support for C# is an exception because it's a major addition).

[TeBoring]

https://github.com/protocolbuffers/protobuf/tree/master/csharp/compatibility_tests/v3.0.0
We already have that. We just need to update the version number in it to test against the newest release.

[anandolee]

I will add compatibility test for 3.7.0

[anandolee]

The 3.7.0 compatibility test is unable to catch the error. @ObsidianMinor are you sure the failure is when use 3.7.0 protoc with 3.9.0 runtime? I also noticed that in your PR, you mentioned "There was no way to generate extension identifiers at the time (before #5936) so rather than add tests for code that couldn't actually be used (unless the consumer decided to implement the interfaces and use the API manually)"

Is it because users implement the interfaces and used the API manually (not the generated code)? I am wondering how to reproduce the error in compatibility test or maybe we do not want to add this to compatibility test?

[ObsidianMinor]

The error comes from adding the defaultValue parameter to the FieldCodec factory methods. You can't catch it at compile time, only at runtime if you compile a library against another version of Google.Protobuf.

[jskeet]

The test steps would need to be something like:

• Generate with 3.7.0
• Compile against 3.7.0
• Run against 3.9.0