TheThingsIndustries / protoc-gen-fieldmask

Generate field mask utilities from proto files
Apache License 2.0
11 stars 3 forks source link

Relax oneof set fields validation #43

Closed adriansmares closed 2 years ago

adriansmares commented 2 years ago

Summary

References https://github.com/TheThingsNetwork/lorawan-stack/pull/5326

Changes

Notes for Reviewers

This is motivated by the ADR settings PR. Before this PR, it would be impossible to change the oneof field without clearing the field in between uses. This 'clear' operation (setting to nil) was required because the generated code would complain that the source and the destination oneof type are mismatching.

The effect on TTS can be seen at https://github.com/TheThingsNetwork/lorawan-stack/commit/453b8350169237574036694622a80dc155bc9d20

adriansmares commented 2 years ago

Just to make sure that I understood this: partial patches (so the ones which do have subfields) should require that the source and destination types match, but full replaces may change the type freely ?

I understand the reasoning to a certain extent but keep in mind that the client does not know what is stored on the server side - It is hard to know when I need to do a replace and when I need to do a patch as a client, so requiring that I use the top level field complicates things. We require that the client fully retrieves the entity before the update if we require that the top level field is used, which is inconsistent with the rest of the API design.

Note that this PR does not allow you to provide two oneof branches at once - that one is still banned - you cannot give both oneof_field.oneof_branch_a and oneof_field.oneof_branch_b at the same time for an update.

htdvisser commented 2 years ago

Just to make sure that I understood this: partial patches (so the ones which do have subfields) should require that the source and destination types match, but full replaces may change the type freely?

Exactly. And to give a clear example, let's look at the ApplicationPubSub message. If the stored PubSub has nats in the provider oneof, I should not be able to set only the mqtt.password. Instead, I need to replace the entire provider by setting the entire mqtt object, so that we can also validate the entire thing.

keep in mind that the client does not know what is stored on the server side

I think it's not unreasonable to expect that clients know the state of the entity (or at least the relevant fields) before they can make changes to those fields.

adriansmares commented 2 years ago

I've implemented the requested change and added two test cases that should cover this.