Conversation
| return true | ||
| }) | ||
| return err | ||
| case fd.IsMap(): |
There was a problem hiding this comment.
For map fields whose value was not a map, it would previously fall-through and end up getting to line 760, where it would blow up with this:
type mismatch: cannot convert map to message
The value type is actually a map. But we end up calling the val.Message() method. Oops.
So now, if it's a map whose value is not a message, it will no longer fall-through due to the condition change here.
unmultimedio
left a comment
There was a problem hiding this comment.
lgtm, although not sure if you want another review from someone more familiar with FileDescriptors than me? 😅
LOL, Robbert was my go to reviewer for this stuff before this week. Maybe @bufdev or @srikrsna-buf? |
|
@bufdev, I merged but just realized that this might be changelog-worthy since it fixes a crash. WDYT? If you agree, I'll open another PR w/ the changelog note. And then we should probably cut a point release. (The only other change to the CLI since v1.15.0 is #1874 which doesn't seem minor-version-worthy.) |
Imagine filtering could panic with "type mismatch: cannot convert map to message" for sources that have map fields where the map value is not a message.
🤦 Caught this while on-call. It happened in production when the BSR was handling an
ImageService.GetImagerequest. I think this means use of-typeparam withbuf buildandbuf generatewill crash if the schema contains maps whose values are not message types 😢. So we'll probably want to create a patch release of the Buf CLI, too.Fixes BSR-1543