search

TheThingsIndustries / protoc-gen-fieldmask

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

Adapt to updated specification, performance, error handling #8

Closed rvolosatovs closed 5 years ago

rvolosatovs commented 5 years ago

Summary:

References TheThingsIndustries/lorawan-stack#1212 https://github.com/TheThingsIndustries/lorawan-stack/pull/1329 Closes #9 See output for lorawan-stack here: https://github.com/TheThingsIndustries/lorawan-stack/pull/1329/commits/42fccc2c7eaf17d3f3c3b5db44d75cb785e1d277

Changes:

johanstokking commented 5 years ago

Why still going through sub-fields? Why does this poc not suffice?

rvolosatovs commented 5 years ago

Changing that behavior is out of scope of this PR - it will be addressed in https://github.com/TheThingsIndustries/protoc-gen-fieldmask/issues/7

htdvisser commented 5 years ago

I think we're diverging a bit proto spec with how we handle nil. I also think we don't actually need to split here, and a SetFields is sufficient.

Let's first consider the "formal" definition of SetFields:

Definition:

(destination * Message).SetFields(source *Message, paths ...string) error

Behavior:

For each path in paths, take the value of that field in source and set it in destination. Return an error if you can't do that.

Next, let's talk about the meaning of "the value of that field in source". The protobuf docs read:

When a message is parsed, if the encoded message does not contain a particular singular element, the corresponding field in the parsed object is set to the default value for that field. These defaults are type-specific: [...] For message fields, the field is not set. Its exact value is language-dependent. See the generated code guide for details.

If we look at what protoc-gen-go generated code returns for getting the value of a (deeply nested) field session.keys.app_s_key.kek_label of EndDevice:

(*EndDevice)(example).GetSession().GetKeys().GetAppSKey().GetKEKLabel()

This returns "", so I think that technically, this is what we should consider to be "the value of that field in source" that needs to be "set in destination".

Note that this would currently panic with SetField.

After re-reading the discussion in https://github.com/TheThingsIndustries/lorawan-stack/issues/1212, I think there are a couple of situations where this doesn't give the desired results:

  1. A SetEndDevice request containing path session.dev_addr, but session=nil. I think this should be caught during request validation, and that SetFields should not care about this.
  2. A GetEndDevice request containing path session.dev_addr, but the device in the database has session=nil. In this case the "spec-compliant" behavior would cause the returned device to have a non-nil session (in Go code), which isn't the desired result, but is correct according to the protobuf spec.

Another important thing to note is that the protobuf docs say:

there's no way of telling whether a field was explicitly set to the default value (for example whether a boolean was set to false) or just not set at all: you should bear this in mind when defining your message types [...] Also note that if a scalar message field is set to its default, the value will not be serialized on the wire.

That last note is important here, I think, because it implies that the returned device from above, will actually be omitted from the protobuf response (so there won't actually be a session with a zero DevAddr)

rvolosatovs commented 5 years ago

So what you're saying is that GetFields is enough here, not SetFields, right? So we would simply have 1 method(SetFields), which only errors if the path specified is unknown and sets fields to default values if a nil is encountered in the path.

rvolosatovs commented 5 years ago

Closing for now

rvolosatovs commented 5 years ago

@johanstokking I still have to test this, but would be good to receive some feedback/initial thoughts on the end result(see https://github.com/TheThingsIndustries/lorawan-stack/pull/1329/commits/7cfc02fe1e0a0cffe55ef7ad68712b658e286a2f) Please leave comments in https://github.com/TheThingsIndustries/lorawan-stack/pull/1329 I will remove the in progress label once NS and JS tests in https://github.com/TheThingsIndustries/lorawan-stack/pull/1329 pass I will work on adding more tests in this project also