Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve unknown fields when converting between FileDescriptorProto and ImageFile #1855

Merged
merged 5 commits into from
Feb 23, 2023
+295 −34
Merged

Preserve unknown fields when converting between FileDescriptorProto and ImageFile #1855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
adc24a0
preserve unknown
jhump Feb 22, 2023
d23d351
must use protocmp to correctly compare protos
jhump Feb 22, 2023
d390f86
strip buf extension from file descriptor proto unrecognized fields
jhump Feb 23, 2023
9d5d48f
add file copyright
jhump Feb 23, 2023
9d059d3
review feedback
jhump Feb 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 38 additions & 33 deletions private/bufpkg/bufimage/bufimagetesting/bufimagetesting_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,22 +455,19 @@ func TestBasic(t *testing.T) {
)
diff := cmp.Diff(protoImage, bufimage.ImageToProtoImage(newImage), protocmp.Transform())
require.Equal(t, "", diff)
// TODO: all of the below proto equality checks should use cmp.Diff
Copy link
Member Author

@jhump jhump Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to address this TODO because now that the ProtoReflect method might be called (to query/set unknown fields), the internal, unexported atomicMessageState pointer field of the message can differ between the two messages being compared (previously, by coincidence, was always nil on both sides).

require.Equal(
t,
&descriptorpb.FileDescriptorSet{
File: []*descriptorpb.FileDescriptorProto{
testProtoImageFileToFileDescriptorProto(protoImageFileAA),
testProtoImageFileToFileDescriptorProto(protoImageFileImport),
testProtoImageFileToFileDescriptorProto(protoImageFileWellKnownTypeImport),
testProtoImageFileToFileDescriptorProto(protoImageFileAB),
testProtoImageFileToFileDescriptorProto(protoImageFileBA),
testProtoImageFileToFileDescriptorProto(protoImageFileBB),
testProtoImageFileToFileDescriptorProto(protoImageFileOutlandishDirectoryName),
},
fileDescriptorSet := &descriptorpb.FileDescriptorSet{
File: []*descriptorpb.FileDescriptorProto{
testProtoImageFileToFileDescriptorProto(protoImageFileAA),
testProtoImageFileToFileDescriptorProto(protoImageFileImport),
testProtoImageFileToFileDescriptorProto(protoImageFileWellKnownTypeImport),
testProtoImageFileToFileDescriptorProto(protoImageFileAB),
testProtoImageFileToFileDescriptorProto(protoImageFileBA),
testProtoImageFileToFileDescriptorProto(protoImageFileBB),
testProtoImageFileToFileDescriptorProto(protoImageFileOutlandishDirectoryName),
},
bufimage.ImageToFileDescriptorSet(image),
)
}
diff = cmp.Diff(fileDescriptorSet, bufimage.ImageToFileDescriptorSet(image), protocmp.Transform())
require.Equal(t, "", diff)
codeGeneratorRequest := &pluginpb.CodeGeneratorRequest{
ProtoFile: []*descriptorpb.FileDescriptorProto{
testProtoImageFileToFileDescriptorProto(protoImageFileAA),
Expand All @@ -490,17 +487,21 @@ func TestBasic(t *testing.T) {
"d/d.proto/d.proto",
},
}
require.Equal(
t,
diff = cmp.Diff(
codeGeneratorRequest,
bufimage.ImageToCodeGeneratorRequest(image, "foo", nil, false, false),
protocmp.Transform(),
)
require.Equal(t, "", diff)

// verify that includeWellKnownTypes is a no-op if includeImports is false
require.Equal(
t,
diff = cmp.Diff(
codeGeneratorRequest,
bufimage.ImageToCodeGeneratorRequest(image, "foo", nil, false, true),
protocmp.Transform(),
)
require.Equal(t, "", diff)

codeGeneratorRequestIncludeImports := &pluginpb.CodeGeneratorRequest{
ProtoFile: []*descriptorpb.FileDescriptorProto{
testProtoImageFileToFileDescriptorProto(protoImageFileAA),
Expand All @@ -522,11 +523,12 @@ func TestBasic(t *testing.T) {
"d/d.proto/d.proto",
},
}
require.Equal(
t,
diff = cmp.Diff(
codeGeneratorRequestIncludeImports,
bufimage.ImageToCodeGeneratorRequest(image, "foo", nil, true, false),
protocmp.Transform(),
)
require.Equal(t, "", diff)
newImage, err = bufimage.NewImageForCodeGeneratorRequest(codeGeneratorRequest)
require.NoError(t, err)
AssertImageFilesEqual(
Expand Down Expand Up @@ -563,11 +565,12 @@ func TestBasic(t *testing.T) {
"d/d.proto/d.proto",
},
}
require.Equal(
t,
diff = cmp.Diff(
codeGeneratorRequestIncludeImportsAndWellKnownTypes,
bufimage.ImageToCodeGeneratorRequest(image, "foo", nil, true, true),
protocmp.Transform(),
)
require.Equal(t, "", diff)
// imagesByDir and multiple Image tests
imagesByDir, err := bufimage.ImageByDir(image)
require.NoError(t, err)
Expand Down Expand Up @@ -642,11 +645,12 @@ func TestBasic(t *testing.T) {
},
},
}
require.Equal(
t,
codeGeneratorRequests,
bufimage.ImagesToCodeGeneratorRequests(imagesByDir, "foo", nil, false, false),
)
requestsFromImages := bufimage.ImagesToCodeGeneratorRequests(imagesByDir, "foo", nil, false, false)
require.Equal(t, len(codeGeneratorRequests), len(requestsFromImages))
for i := range codeGeneratorRequests {
diff = cmp.Diff(codeGeneratorRequests[i], requestsFromImages[i], protocmp.Transform())
require.Equal(t, "", diff)
}
codeGeneratorRequestsIncludeImports := []*pluginpb.CodeGeneratorRequest{
{
ProtoFile: []*descriptorpb.FileDescriptorProto{
Expand Down Expand Up @@ -688,11 +692,12 @@ func TestBasic(t *testing.T) {
},
},
}
require.Equal(
t,
codeGeneratorRequestsIncludeImports,
bufimage.ImagesToCodeGeneratorRequests(imagesByDir, "foo", nil, true, false),
)
requestsFromImages = bufimage.ImagesToCodeGeneratorRequests(imagesByDir, "foo", nil, true, false)
require.Equal(t, len(codeGeneratorRequestsIncludeImports), len(requestsFromImages))
for i := range codeGeneratorRequestsIncludeImports {
diff = cmp.Diff(codeGeneratorRequestsIncludeImports[i], requestsFromImages[i], protocmp.Transform())
require.Equal(t, "", diff)
}
}

func testProtoImageFileToFileDescriptorProto(imageFile *imagev1.ImageFile) *descriptorpb.FileDescriptorProto {
Expand Down
54 changes: 53 additions & 1 deletion private/bufpkg/bufimage/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,16 @@ import (
"github.com/bufbuild/buf/private/pkg/normalpath"
"github.com/bufbuild/buf/private/pkg/protodescriptor"
"github.com/bufbuild/buf/private/pkg/stringutil"
"google.golang.org/protobuf/encoding/protowire"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"
"google.golang.org/protobuf/types/descriptorpb"
"google.golang.org/protobuf/types/pluginpb"
)

// Must match the tag number for ImageFile.buf_extensions defined in proto/buf/alpha/image/v1/image.proto.
const bufExtensionFieldNumber = 8042

// paths can be either files (ending in .proto) or directories
// paths must be normalized and validated, and not duplicated
// if a directory, all .proto files underneath will be included
Expand Down Expand Up @@ -345,7 +350,7 @@ func fileDescriptorProtoToProtoImageFile(
if len(unusedDependencyIndexes) == 0 {
unusedDependencyIndexes = nil
}
return &imagev1.ImageFile{
resultFile := &imagev1.ImageFile{
Name: fileDescriptorProto.Name,
Package: fileDescriptorProto.Package,
Syntax: fileDescriptorProto.Syntax,
Expand All @@ -368,6 +373,53 @@ func fileDescriptorProtoToProtoImageFile(
ModuleInfo: protoModuleInfo,
},
}
resultFile.ProtoReflect().SetUnknown(stripBufExtensionField(fileDescriptorProto.ProtoReflect().GetUnknown()))
return resultFile
}

func stripBufExtensionField(unknownFields protoreflect.RawFields) protoreflect.RawFields {
// We accumulate the new bytes in result. However, for efficiency, we don't do any
// allocation/copying until we have to (i.e. until we actually see the field we're
// trying to strip). So result will be left nil and initialized lazily if-and-only-if
// we actually need to strip data from unknownFields.
var result protoreflect.RawFields
bytesRemaining := unknownFields
for len(bytesRemaining) > 0 {
num, wireType, n := protowire.ConsumeTag(bytesRemaining)
if n < 0 {
// shouldn't be possible unless explicitly set to invalid bytes via reflection
return unknownFields
}
var skip bool
if num == bufExtensionFieldNumber {
// We need to strip this field.
skip = true
if result == nil {
// Lazily initialize result to the preface that we've already examined.
result = append(
make(protoreflect.RawFields, 0, len(unknownFields)),
unknownFields[:len(unknownFields)-len(bytesRemaining)]...,
)
}
} else if result != nil {
// accumulate data in result as we go
result = append(result, bytesRemaining[:n]...)
}
bytesRemaining = bytesRemaining[n:]
n = protowire.ConsumeFieldValue(num, wireType, bytesRemaining)
if n < 0 {
return unknownFields
}
if !skip && result != nil {
result = append(result, bytesRemaining[:n]...)
}
bytesRemaining = bytesRemaining[n:]
}
if result == nil {
// we did not have to remove anything
return unknownFields
}
return result
}

func imageToCodeGeneratorRequest(
Expand Down
Loading