Remove support for deprecated protovalidate options #3610
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
jhump
left a comment
jhump
left a comment
There was a problem hiding this comment.
Wow, this is complicated.
I imagine this would be way simpler and easier to understand (and less potentially brittle) to just skip the synthetic message and validate the violations solely using a filter on the violation field paths.
The only problem I foresee with that is that if any other field has a configuration error (like a bad CEL expression) then none of the examples can be validated. But we'd still issue a lint warning for that configuration error, right? So seems like it's probably an okay trade-off.
private/bufpkg/bufcheck/bufcheckserver/internal/buflintvalidate/field.go
Show resolved
Hide resolved
| newFileDescriptorProto := &descriptorpb.FileDescriptorProto{ | ||
| Name: proto.String(":synthetic:"), | ||
| Package: proto.String("__SYNTHETIC__"), | ||
| Syntax: proto.String("proto3"), |
There was a problem hiding this comment.
This file must have the same syntax (and possibly edition) as the original file. Otherwise, the new field descriptor may have properties that are incompatible with the enclosing file's syntax/edition. For example, if the field has features, those aren't allowed outside of editions. If it has a required label, that's not allowed outside of proto2.
Also, for editions, you need to copy over the original file's features -- to make sure the new synthetic file has all of the same file-wide default feature values as the original. And the new synthetic containing message will need to mirror that of the original. But this step gets more complicated: if the original message was not a top-level message, you need to actually combine the features of the original message and all of its ancestors/enclosing messages, so that the effective set of message-level features (from which a field might inherit) are identical to those in the original message.
| if !parentMapFieldDescriptor.IsMap() { | ||
| return nil, nil, errors.New("parentMapFieldDescriptor is not a map") | ||
| } | ||
| newMapEntryMessageDescriptor := &descriptorpb.DescriptorProto{ |
There was a problem hiding this comment.
I think this would be much safer (and less code) to instead use protodesc to mechanically translate the entire map entry message to a *descriptorpb.DescriptorProto, to make sure you don't miss anything here (such as features in an editions file).
There was a problem hiding this comment.
You'll also need to handle proto2 groups: in those cases, it should be fine to treat like any other message field, and let it refer to the group definition in the original file. But you'd need to change the field's type from TYPE_GROUP to TYPE_MESSAGE.
| newFieldDescriptorProto.Type = descriptorpb.FieldDescriptorProto_TYPE_MESSAGE.Enum() | ||
| newFieldDescriptorProto.TypeName = proto.String(newMapEntryMessageDescriptor.GetName()) |
There was a problem hiding this comment.
This isn't necessary. If it was originally a map field, these are already set this way, from the call to protodesc.ToFieldDescriptorProto.
There was a problem hiding this comment.
Oof, never mind. That is only true of the first one, the type enum. This is correct that it must re-write the type name.
|
Closing this since we are going to update this with the new |
ghost
mentioned this pull request
2 participants
This is related to bufbuild/protovalidate#297, so it should wait until that is merged first.