🐛 [firebase_core] Generated Pigeon code is spanning packages

[stuartmorgan]

I came across this code today, where Pigeon is being configured to generate Dart into firebase_core_platform_interface, and native code into firebase_core. This is extremely dangerous, particularly given that Pigeon isn't being pinned.

The channel details are considered internal implementation details of Pigeon, and subject to change at any time, without breaking version changes. This is not theoretical; for example 4.2.12 completely changed the serialization format of objects, and 10.1.4 changed the names of the channels.

The way this package has been set up, any change to the version of Pigeon used to generate the code for these two packages would have to be versioned as a breaking change to firebase_core_platform_interface. If that's really the behavior you want, I would very strongly recommend that you pin pigeon in your pubspec.yaml and put a very clear warning in a comment on that line. But bigger picture, I would strongly urge you not to use this structure in the first place; putting two halves of a protocol that must exactly match and that you don't directly control in two different packages is very fragile, and likely to cause bugs in the future.

[darshankawar]

/cc @Lyokone and @russellwheatley

[russellwheatley]

Hey @stuartmorgan, thank you for the heads up. I think at a minimum, we will pin the version as you've recommended. We're going to discuss the implications in respect to the Pigeon implementations across packages. The firebase_core_platform_interface is a dependency of firebase_core. We have a fairly robust versioning system that ensures their versions are in sync once code generation updates are made. We also have integration tests that would likely fail if the Dart API does not match its native counterpart.

[stuartmorgan]

The firebase_core_platform_interface is a dependency of firebase_core.

But not a pinned one, which means that:

We have a fairly robust versioning system that ensures their versions are in sync once code generation updates are made.

unless that versioning system includes a guarantee that you make a major version change when updating Pigeon-generated code, you cannot ensure that.

We also have integration tests that would likely fail if the Dart API does not match its native counterpart.

Do your integration tests run with every possible combination of versions that the resolver allows? In my experience, most CI systems just test the latest version of everything. That means that you would notice if you updated the Dart generated code without updating the native generated code, but not if you published that as a non-breaking change, at which point your plugin's clients are not protected from the scenario where they use the latest version of firebase_core_platform_interface (containing updated Pigeon-generated Dart code) with an existing version of firebase_core (containing old, incompatible Pigeon-generated host code).

To give a concrete example since this can be hard to reason about in the abstract:

• firebase_core_platform_interface is currently 4.8.0
  • With Pigeon 9.x-generated code.
• firebase_core is currently 2.17.0
  • Depending on firebase_core_platform_interface ^4.8.0
  • Also with Pigeon 9.x-generated code.

Imagine someone updates Pigeon to 11.x—which is not compatible with 9.x—and then regenerates everything, releasing it as a new minor version, instead of major version. There would be:

• firebase_core_platform_interface 4.9.0
  • With Pigeon 11.x-generated code.
• firebase_core 2.18.0
  • Depending on firebase_core_platform_interface ^4.9.0
  • Also with Pigeon 11.x-generated code.

That will pass any CI that doesn't do very specific things designed for edge cases like this (such as pub downgrade before running all tests). It will correctly prevent someone from using f_c 2.18 (new Pigeon) with f_c_p_i 4.8 (old Pigeon) because of the constraint update in the app-facing package. But it will not prevent someone from running f_c 2.17 (old Pigeon) with f_c_p_i 4.9 (new Pigeon), because the already-released-to-the-wild 2.17 says it is compatible with ^4.8.0, which includes 4.9.x.

[stuartmorgan]

(Also, presumably the plugin is federated in order to allow for other implementations that aren't maintained by this organization. By making the Dart code part of the platform interface package, the implication is that other packages could use it as well, but in practice they can't unless you only change the generated code in major version updates because they would be completely broken by f_c_p_i 4.9.0 in the scenario above. And if other implementors aren't supposed to rely on that Dart code, then having it in f_c_p_i rather than f_c doesn't help, and is just actively confusing.)

[russellwheatley]

Given the outlined dilemma above, we're going to pin the platform interface package version to the relevant packages that depend upon it.

In our case, the platform interface packages are for the use of FlutterFire. They were not designed to be consumed by other users.

[stuartmorgan]

In our case, the platform interface packages are for the use of FlutterFire. They were not designed to be consumed by other users.

The sole purpose of the federated plugin design is to allow federation. Why does the platform interface package exist if not to allow other implementations?

(I'm confused why the proposed solution here is not to simply move the Dart code for the Pigeon implementation into the other package, which would completely solve this problem without abandoning federation, and also be less complex.)