grpc-gateway icon indicating copy to clipboard operation
grpc-gateway copied to clipboard

registry: descriptor.Field.FieldMessage is never set

Open utrack opened this issue 4 years ago • 5 comments

Hi! While fixing a bug in github.com/utrack/clay I've noticed that bindings' Body.FieldPath[].Target.FieldMessage is always nil.

I don't see any references to FieldMessage in [email protected], so I don't believe there are any bugs related to it, so it can be dropped.

However, the field itself looks useful and is worth to be populated IMO. I'll make a PR if you decide to keep it in the registry.

utrack avatar Feb 17 '20 09:02 utrack

Looks like this was added by our very own @achew22, so I'll let him speak for its use. I personally don't think I understand the utility. Maybe you could share what your interpretation is?

johanbrandhorst avatar Feb 17 '20 09:02 johanbrandhorst

Well, since Field is a rich FieldDescriptorMessage it feels like it should do what rich Message does for FieldDescriptorProto :) It's handy to be able to get Go types for fields and access all the extended methods.

Right now I'm trying to go through the fields of a binding and scan nested types to manipulate body bindings, but that's just a single use case.

utrack avatar Feb 17 '20 11:02 utrack

I don't see why we couldn't add the support, so please, do feel free to submit a PR.

johanbrandhorst avatar Feb 17 '20 12:02 johanbrandhorst

Almost certainly, that was created in a fit of copy-pasting. If the value if of use to you, it shouldn't be too hard to plumb it, but I seem to have forgotten and no one noticed. Given the age of that line of code (4+ years) without anyone commenting on it till now, my vote is that we remove so people don't depend on it.

achew22 avatar Feb 17 '20 19:02 achew22

Sorry, just remembered about that issue.

What should be the proper semantic meaning for Field.Message? Right now if I have a binding of foo.bar.baz.qux, both foo.Message, bar.Message, ... are pointing to qux.Message - i.e. there's no way to get foo's Message when looping through []Field. Is it a bug or a feature? :)

If it's not a bug and no one else needs it then FieldMessage could be removed, I guess.

utrack avatar Mar 04 '20 12:03 utrack