diff --git a/CHANGELOG.md b/CHANGELOG.md index 882468db10..5a98838051 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ - Add `--stdin-filepath` flag to `buf format`, which reads a single `.proto` file from stdin and writes the formatted result to stdout. The path is not read from disk, and is only used to report parse errors and diffs. +- Fix `buf convert` panic when converting images that contain files without a `syntax` + declaration or with unused imports. - Fix `buf generate` with `strategy: directory` and `include_imports: true` adding imports to the `CodeGeneratorRequest` of the directory that imports them. Imports are now split by directory into their own requests, the same as non-imports. diff --git a/cmd/buf/buf_test.go b/cmd/buf/buf_test.go index 6cb0078bd6..92d97fa3d4 100644 --- a/cmd/buf/buf_test.go +++ b/cmd/buf/buf_test.go @@ -3921,6 +3921,30 @@ func TestConvertInvalidTypeName(t *testing.T) { ) } +func TestConvertImageWithSourceCodeInfoExtension(t *testing.T) { + t.Parallel() + // The built image carries Buf's SourceCodeInfo extension, which is not part + // of the schema and so must not be resolved when converting. + tempDir := t.TempDir() + schemaDir := filepath.Join(convertTestDataDir, "source_code_info_extension") + imagePath := filepath.Join(tempDir, "image.binpb") + testRunStdout(t, nil, 0, ``, "build", schemaDir, "-o", imagePath) + testRun( + t, + 0, + nil, + nil, + "convert", + schemaDir, + "--type", + "google.protobuf.FileDescriptorSet", + "--from", + imagePath, + "--to", + "-#format=json", + ) +} + func TestConvert(t *testing.T) { t.Parallel() t.Run("binpb-to-json-file-proto", func(t *testing.T) { diff --git a/cmd/buf/internal/command/convert/testdata/convert/source_code_info_extension/a.proto b/cmd/buf/internal/command/convert/testdata/convert/source_code_info_extension/a.proto new file mode 100644 index 0000000000..3fdb28f621 --- /dev/null +++ b/cmd/buf/internal/command/convert/testdata/convert/source_code_info_extension/a.proto @@ -0,0 +1,7 @@ +// No syntax and an unused import, so that buf build attaches its +// SourceCodeInfo extension to this file. +package a; + +import "google/protobuf/descriptor.proto"; + +message A {} diff --git a/cmd/buf/internal/command/convert/testdata/convert/source_code_info_extension/buf.yaml b/cmd/buf/internal/command/convert/testdata/convert/source_code_info_extension/buf.yaml new file mode 100644 index 0000000000..b4d4e478b7 --- /dev/null +++ b/cmd/buf/internal/command/convert/testdata/convert/source_code_info_extension/buf.yaml @@ -0,0 +1 @@ +version: v2 diff --git a/private/buf/bufconvert/bufconvert.go b/private/buf/bufconvert/bufconvert.go index 43e4998b51..acc38b97eb 100644 --- a/private/buf/bufconvert/bufconvert.go +++ b/private/buf/bufconvert/bufconvert.go @@ -138,6 +138,12 @@ func checkNoMessageSetWireFormat(descriptor protoreflect.MessageDescriptor, imag // In order to create a protoreflect.MessageDescriptor, we may have already stripped // any message_set_wire_format option. So we must examine the file descriptor proto // in the image to see if the option is set. If it is, we cannot support this message. + if descriptor.IsPlaceholder() { + // The type is not in the image, such as when it was built with + // --exclude-imports, so there is nothing to check. Its fields are + // unknown and so are left as unknown bytes when unmarshaling. + return nil + } name := descriptor.FullName() if slices.Contains(checked, name) { return nil diff --git a/private/buf/bufconvert/bufconvert_test.go b/private/buf/bufconvert/bufconvert_test.go index 0e416032a7..803477c551 100644 --- a/private/buf/bufconvert/bufconvert_test.go +++ b/private/buf/bufconvert/bufconvert_test.go @@ -84,6 +84,63 @@ func TestImageWithoutMessageSetWireFormatResolution(t *testing.T) { checker.succeeds(noResolveImage.Resolver().FindEnumByName("foo.bar.Enum")) } +func TestImageWithoutMessageSetWireFormatResolutionPlaceholder(t *testing.T) { + t.Parallel() + // Types missing from the image, such as when it was built with + // --exclude-imports, resolve to placeholders that cannot be checked. + file := &descriptorpb.FileDescriptorProto{ + Name: new("test.proto"), + Package: new("foo.bar"), + Dependency: []string{"missing.proto"}, + MessageType: []*descriptorpb.DescriptorProto{ + { + Name: new("Baz"), + Field: []*descriptorpb.FieldDescriptorProto{ + { + Name: new("missing"), + Number: new(int32(1)), + Label: descriptorpb.FieldDescriptorProto_LABEL_OPTIONAL.Enum(), + Type: descriptorpb.FieldDescriptorProto_TYPE_MESSAGE.Enum(), + TypeName: new(".missing.Missing"), + }, + }, + ExtensionRange: []*descriptorpb.DescriptorProto_ExtensionRange{ + {Start: new(int32(100)), End: new(int32(200))}, + }, + }, + }, + Extension: []*descriptorpb.FieldDescriptorProto{ + { + Name: new("missing_ext"), + Number: new(int32(100)), + Label: descriptorpb.FieldDescriptorProto_LABEL_OPTIONAL.Enum(), + Type: descriptorpb.FieldDescriptorProto_TYPE_MESSAGE.Enum(), + TypeName: new(".missing.Missing"), + Extendee: new(".foo.bar.Baz"), + }, + }, + } + imageFile, err := bufimage.NewImageFile( + file, + nil, + uuid.UUID{}, + file.GetName(), + file.GetName(), + false, + false, + nil, + ) + require.NoError(t, err) + image, err := bufimage.NewImage([]bufimage.ImageFile{imageFile}) + require.NoError(t, err) + resolver := ImageWithoutMessageSetWireFormatResolution(image).Resolver() + + checker := resultChecker{t} + checker.succeeds(resolver.FindMessageByName("foo.bar.Baz")) + checker.succeeds(resolver.FindExtensionByName("foo.bar.missing_ext")) + checker.succeeds(resolver.FindExtensionByNumber("foo.bar.Baz", 100)) +} + func TestFindMessageInFile(t *testing.T) { t.Parallel() t.Run("no-package", func(t *testing.T) { diff --git a/private/buf/buflsp/testdata/organize_imports/oneline_import_used_test.proto b/private/buf/buflsp/testdata/organize_imports/oneline_import_used_test.proto new file mode 100644 index 0000000000..d4ac06d1e0 --- /dev/null +++ b/private/buf/buflsp/testdata/organize_imports/oneline_import_used_test.proto @@ -0,0 +1 @@ +syntax = "proto3"; import "types/existing_field.proto"; message M { example.types.ExistingField f = 1; } \ No newline at end of file diff --git a/private/bufpkg/bufimage/build_image.go b/private/bufpkg/bufimage/build_image.go index 1c30660d29..05c52af818 100644 --- a/private/bufpkg/bufimage/build_image.go +++ b/private/bufpkg/bufimage/build_image.go @@ -281,8 +281,9 @@ func BuildImageFromOpener( } // resolverForFDS normalizes the given [*descriptorpb.FileDescriptorSet] by -// marshaling and re-parsing it, then builds a [protoencoding.Resolver] with -// Buf's custom descriptor extensions recognised, ready for [fileDescriptorSetToImage]. +// marshaling and re-parsing it with Buf's custom descriptor extensions +// recognised, then returns it with a [protoencoding.Resolver] for its files, +// ready for [fileDescriptorSetToImage]. // // The marshal/unmarshal round-trip is necessary because [fdp.DescriptorProto] // stores option values as unknown bytes via SetUnknown. Re-parsing converts those @@ -301,15 +302,16 @@ func resolverForFDS(fds *descriptorpb.FileDescriptorSet) (*descriptorpb.FileDesc // input exactly (including any vendored google/protobuf/descriptor.proto). // Buf's descriptor extensions are supplied by a fallback resolver consulted // only when the file graph cannot resolve a descriptor, so ReparseExtensions - // can recognise them without altering the graph. + // can recognise them without altering the graph. The fallback is only used + // for reparsing: the returned resolver must resolve only the image's files. fileResolver := protoencoding.NewLazyResolver(normalizedFDS.File...) - resolver := protoencoding.CombineResolvers(fileResolver, bufDescriptorResolver) + reparseResolver := protoencoding.CombineResolvers(fileResolver, bufDescriptorResolver) for _, fileDescriptor := range normalizedFDS.File { - if err := protoencoding.ReparseExtensions(resolver, fileDescriptor.ProtoReflect()); err != nil { + if err := protoencoding.ReparseExtensions(reparseResolver, fileDescriptor.ProtoReflect()); err != nil { return nil, nil, err } } - return normalizedFDS, resolver, nil + return normalizedFDS, fileResolver, nil } // fileDescriptorSetToImage is a helper function that converts a [descriptorpb.FileDescriptorSet]