msggen: Add UnionField support to future oneOf fields for oneOf schemas#8965
msggen: Add UnionField support to future oneOf fields for oneOf schemas#89654xvgal wants to merge 1 commit into
Conversation
| optional uint64 generation = 2; | ||
| optional bytes hex = 3; | ||
| optional string string = 4; | ||
| repeated string key = 5; |
There was a problem hiding this comment.
This is a backwards incompatible change. Never change the field numbering in protobuf.
There was a problem hiding this comment.
Fixed - the first variant of each oneof now reuses the original field number.
new numbers are only allocated for additional variants.
| optional uint64 generation = 4; | ||
| repeated string key = 5; | ||
| oneof key { | ||
| DatastoreRequestarr_stringWrapper key_arr_string = 5; |
There was a problem hiding this comment.
Pretty sure this is also not backwards compatible. Also, what is really gained by having "repeated string" and "string"? I think it would be best to keep "repeated string" here.
| string label = 2; | ||
| oneof label { | ||
| string label_string = 2; | ||
| sint64 label_int = 4; |
There was a problem hiding this comment.
If you use this and send a label_int = 4 to an old parser, it will just ignore the field i think. This makes it kind of a breaking change as well imo.
There was a problem hiding this comment.
Thanks for reviewing.
I'll look around more and fix it and Re-open when it ready.
I was in hurry when found & open it.
|
Unfortunately i think it's either breaking changes or only use the new generator code on new union fields. @cdecker what do you think? |
|
It is for sure breaking, and I'm unsure if we want to lean into generation more than truncating the hierarchy with manually managed stubs. I'd like to see the output of the pre and post change generated files as I fear it may be changing a lot. But there is a discussion to be had on the future direction of the generated bindings. Personally I am tempted to go more for a uniffi / protobuf style of nested messages that translate into binding classes directly rather than the current heavily nested system (generating names like ListPeerChannelsChannelsHtlcEntry which is just ludicrous). So three options:
Maybe it's a good time to discuss this |
|
Let me check if I understand correctly
|
|
For this PR i'd like to stick to the current overrides and only use the UnionField generation for future oneOf fields. I'll research abit more what a complete rework would look like for CLN and come back to you. But for now let's not do major breaking changes. What do you think? |
The reason I opened this PR was the missing type info from msgen. In my case, cashu-developement-kit needs to get private channel IDs from CLN, and right now it uses call_raw to bypass cln-rpc's typed API limitation. It works, but it's not a clean approach. That said, sticking to the current overrides and using UnionField generation only for future fields sounds resonable to me. I don't want to break anything right now. |
|
I've updated the PR to preserve existing overrides while keeping the UnionField logic for future oneOf fileds. Also updated the title and body to reflect this. Should i squash the commits into one? |
d1837e8 to
4bf490e
Compare
…ogic Changelog-None Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4bf490e to
60beede
Compare
Summary
msggencould not handleoneOffields in JSON schemas. EveryoneOffield was manually overridden to a single type, which dropped some variants. This PR addsUnionFieldsupport to all generators so that futureoneOffields are automatically handled without needing manual overrides. Existing overrides are preserved to avoid breaking changes.For example,
invoice.exposeprivatechannelsacceptstrue,["1x1x3"], or"1x1x3"in JSON-RPC, but cln-rpc only generatedOption<Vec<ShortChannelId>>, making it impossible to passtruethrough the typed API.Changes
model.py:
UnionField.from_js()constructor bug (missingadded/deprecatedargs)oneOfdetection inCompositeField.from_js()Generators (6 files)- ready for future oneOf fields :
rust.py: Generate#[serde(untagged)]enumsproto.py: Generate protobufoneofblocks with array wrapper messagesconvert.py/unconvert.py: Generate bidirectionalFromimplsgrpc2py.py: GenerateWhichOneof()-based conversionnotification.py: Automatically supported viarust.py'sgen_fieldTraversal:
patch.py,checks.py: Add recursion intoUnionFieldvariantsTests
cargo test -p cln-rpc— passedcargo test -p cln-grpc --features server— passed (4/4)msggen generate— runs without errorsRelated
Resolves #8961, #8964