search

hashgraph / pbj

A performance optimized Google Protocol Buffers code generator, parser, and Gradle module.
Apache License 2.0
13 stars 6 forks source link

Support resetting `oneof` fields to `UNSET` via `copyBuilder()` #160

Open tinker-michaelj opened 8 months ago

tinker-michaelj commented 8 months ago

Problem

If a oneof field like this Account#staked_id field is ever set to a non-default value, there is no way to reset its value to com.hedera.hapi.node.state.token.codec.AccountProtoCodec.STAKED_ID_UNSET via a copy builder.

This imposes considerable burden on client code when e.g. an account wants to stop staking.

Solution

Generate a clearStakingId() method on the Account.Builder that lets us reset stakedId to com.hedera.hapi.node.state.token.codec.AccountProtoCodec.STAKED_ID_UNSET via a copy builder.

Alternatives

No response

Neeharika-Sompalli commented 3 months ago

@imalygin @anthony-swirldslabs This is needed in one of the logics for services. Could you please let us know your thoughts and if this can be fixed. Thanks

anthony-swirldslabs commented 3 months ago

Please note that PBJ model objects are immutable per both the design and their implementation, so you wouldn't be able to clear a OneOf field in an already constructed model object.

However, the proposal to allow clearing a OneOf in a copy builder makes total sense to me.

You'd probably need to modify the code around here:

https://github.com/hashgraph/pbj/blob/79ac1de7b8b706b583f89d2fe1c9d4e60743e480/pbj-core/pbj-compiler/src/main/java/com/hedera/pbj/compiler/impl/generators/ModelGenerator.java#L828-L833

to allow it to generate an additional clear method.

Neeharika-Sompalli commented 3 months ago

Please note that PBJ model objects are immutable per both the design and their implementation, so you wouldn't be able to clear a OneOf field in an already constructed model object.

However, the proposal to allow clearing a OneOf in a copy builder makes total sense to me.

You'd probably need to modify the code around here:

https://github.com/hashgraph/pbj/blob/79ac1de7b8b706b583f89d2fe1c9d4e60743e480/pbj-core/pbj-compiler/src/main/java/com/hedera/pbj/compiler/impl/generators/ModelGenerator.java#L828-L833

to allow it to generate an additional clear method.

@anthony-swirldslabs Do you recommend we open a PR for this fix, or anyone owning all the issues in PBJ?

anthony-swirldslabs commented 3 months ago

@Neeharika-Sompalli : you're welcome to open a PR, and I'll be happy to review it!

Alternatively, you can check with @poulok if you don't have cycles to implement this change. Depending on urgency, prioritization, and ownership she may decide to assign it to someone else.