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

rs: Backwards and Forwards compatibility story for cln-rpc and cln-grpc #5988

Closed
cdecker opened this issue Feb 9, 2023 · 3 comments · Fixed by #6142
Closed

rs: Backwards and Forwards compatibility story for cln-rpc and cln-grpc #5988

cdecker opened this issue Feb 9, 2023 · 3 comments · Fixed by #6142
Assignees
Labels

Comments

@cdecker
Copy link
Member

cdecker commented Feb 9, 2023

cln-rpc bindings are generated directly from the JSON schemas in doc/schemas and should therefore always be up-to-date with the version currently being developed (and match the release if we cut a cln-rpc release in sync with the release).

This means however that there is a tight coupling between the CLN version and any client/app/plugin built using cln-rpc and to a lesser extent cln-grpc. For example when the schema adds a new required field, the corresponding cln-rpc code will expect that field to be present and populated, since there is no default null value that could be used (Option<> should be used instead. The same goes for a required field getting dropped in a later version. In either of these cases, having cln-rpc expect a value that is then not present causes parsing of the entire JSON document to fail, and not being able to recover.

For this purpose I'd like to introduce inferred optional fields, i.e., fields that are mandatory in the schema, but due to them having been recently introduced or removed they are to be treated as optional, so we remain compatible with either the old version pre-removal or newer version post-addition. This can leverage the added and deprecated annotations we recently started adding and enforcing on schema changes.

The idea is to explicitly have cln-rpc and others support a range of releases (in line with the deprecation cycle). If a field is required, but has been added or deprecated in the range of supported versions, we infer that the field may not be present before the release that introduced it, or may have been removed after the release that enacted the deprecation, hence we need to make it an inferred optional. In cln-rpc that'd be translated into an Option<>, fixing the parsing. Sadly that means that updating cln-rpc in the client/app/plugin can cause fields to flip-flop between required and optional, but that's a compile-time concern, rather than a runtime concern as is the case for the current crates.

This issue is mainly intended to brainstorm this idea, and if we don't find a hole in the logic, I'll start implementing it in msggen.

@cdecker cdecker self-assigned this Feb 9, 2023
@cdecker
Copy link
Member Author

cdecker commented Feb 9, 2023

Let's see if I can visualize the proposal a bit.

Addition

This is the transition for a required property in the schema. In cln-rpc we'd infer it to be optional until the version which did not have that property drops out of the support window. Before adding there is no way for us to know there will be a property eventually. Adding a required property will artificially be set to Optional as long as versions pre-addition are still in the supported range, and we make it required as soon as the last version predating the addition drops out of the support window.

gantt
    title Adding a new property to a schema
    dateFormat  YYYY-MM-DD
    Pre-addition     :a1, 2000-01-01, 10d
    Addition         :a2, after a1  , 20d
    Post-Addition    :a3, after a2, 10d
    section Optionality
    Option<> :b1, after a1, 20d
    Required  :after b1, 10d
Loading

Removal

The following shows how we remove a required property from a schema. Pre-removal the property is required, as soon as we mark it as deprecated we mark it as optional, and once it gets removed from the schema we no longer parse it.

gantt
    title Removing a property from the schema
    dateFormat YYYY-MM-DD
    Pre-removal  :a1, 2000-01-01, 10d
    Removal      :a2, after a1, 20d
    Post-removal :a3, after a2, 10d
    section Optionality
    Required  :b1, 2000-01-01, 10d
    Optional<> :b2, after b1, 20d
Loading

Don't bother with the dates, I chose a gantt chart just to be able to show the swimlanes, and how parts match up, the real intervals are likely 2 releases / 6 months in reality.

@rustyrussell
Copy link
Contributor

You should be able to get most of this from the "added" and "deprecated" options (which now all take a version number)?

@cdecker
Copy link
Member Author

cdecker commented Feb 17, 2023

Correct, that's the plan. I just want to make sure that a) this idea is sound and minimizes changes to users of the library while the schema evolves, and b) we maintain as much compatibility as possible across library versions, wwithout sacrificing the type-based guarantees. Specifically for languages such as Rust a expecting a field and it isn't there (can't be represented as NULL) causes the entire parser to bail, and the entire message being forwarded to the big handler in the sky.

I haven't found an issue with this so far, except maybe that adding or removing a field now causes 2 changes: add as optional followed by add mandatory, and the inverse for removals. That means library users will have to sprinkle case statements throughout their code if they use these fields, and have to adapt their code, but they'd likely have to do that anyway, and I prefer causing trouble at compile-time rather than at runtime (which would be the case if we just didn't care and let the parsers crash).

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

Successfully merging a pull request may close this issue.

2 participants