From 80b43a0262e783ed4db71258a8e5ce300337905b Mon Sep 17 00:00:00 2001 From: bufdev Date: Mon, 22 Jun 2020 12:41:04 -0400 Subject: [PATCH] Marshal/unmarshal custom options in JSON and add image convert command --- go.mod | 2 +- go.sum | 4 +- internal/buf/bufcli/bufcli.go | 14 +- internal/buf/bufcli/env_reader.go | 149 +++++++++--- internal/buf/cmd/buf/buf.go | 2 +- internal/buf/cmd/buf/buf_test.go | 219 ++++++++++++++---- internal/buf/cmd/buf/cmds.go | 40 +++- internal/buf/cmd/buf/flags.go | 32 +++ internal/buf/cmd/buf/run.go | 31 +++ .../cmd/buf/testdata/customoptions1/a.proto | 11 + .../protoc-gen-buf-check-breaking/breaking.go | 1 + .../pkg/proto/protoencoding/json_marshaler.go | 57 +++++ .../proto/protoencoding/json_unmarshaler.go | 2 + internal/pkg/proto/protoencoding/resolver.go | 5 +- 14 files changed, 494 insertions(+), 75 deletions(-) create mode 100644 internal/buf/cmd/buf/testdata/customoptions1/a.proto diff --git a/go.mod b/go.mod index c1b4d6f3ae..9eb32a64e0 100644 --- a/go.mod +++ b/go.mod @@ -15,7 +15,7 @@ require ( github.com/stretchr/testify v1.6.1 go.uber.org/multierr v1.5.0 go.uber.org/zap v1.15.0 - google.golang.org/genproto v0.0.0-20200617032506-f1bdc9086088 // indirect + google.golang.org/genproto v0.0.0-20200622133129-d0ee0c36e670 // indirect google.golang.org/protobuf v1.24.0 gopkg.in/yaml.v3 v3.0.0-20200615113413-eeeca48fe776 ) diff --git a/go.sum b/go.sum index 3d292a0f0d..9d049f04cc 100644 --- a/go.sum +++ b/go.sum @@ -211,8 +211,8 @@ google.golang.org/genproto v0.0.0-20180817151627-c66870c02cf8/go.mod h1:JiN7NxoA google.golang.org/genproto v0.0.0-20190819201941-24fa4b261c55/go.mod h1:DMBHOl98Agz4BDEuKkezgsaosCRResVns1a3J2ZsMNc= google.golang.org/genproto v0.0.0-20200526211855-cb27e3aa2013 h1:+kGHl1aib/qcwaRi1CbqBZ1rk19r85MNUf8HaBghugY= google.golang.org/genproto v0.0.0-20200526211855-cb27e3aa2013/go.mod h1:NbSheEEYHJ7i3ixzK3sjbqSGDJWnxyFXZblF3eUsNvo= -google.golang.org/genproto v0.0.0-20200617032506-f1bdc9086088 h1:XXo4PvhJkaWYIkwn7bX7mcdB8RdcOvn12HbaUUAwX3E= -google.golang.org/genproto v0.0.0-20200617032506-f1bdc9086088/go.mod h1:jDfRM7FcilCzHH/e9qn6dsT145K34l5v+OpcnNgKAAA= +google.golang.org/genproto v0.0.0-20200622133129-d0ee0c36e670 h1:v/N9fZIfu6jopNImrZwgx48fql5yT3k82CJvpIyGtPA= +google.golang.org/genproto v0.0.0-20200622133129-d0ee0c36e670/go.mod h1:FWY/as6DDZQgahTzZj3fqbO1CbirC29ZNUFHwi0/+no= google.golang.org/grpc v1.8.0/go.mod h1:yo6s7OP7yaDglbqo1J04qKzAhqBH6lvTonzMVmEdcZw= google.golang.org/grpc v1.19.0/go.mod h1:mqu4LbDTu4XGKhr4mRzUsmM4RtVoemTSY81AxZiDr8c= google.golang.org/grpc v1.21.0/go.mod h1:oYelfM1adQP15Ek0mdvEgi9Df8B9CZIaU1084ijfRaM= diff --git a/internal/buf/bufcli/bufcli.go b/internal/buf/bufcli/bufcli.go index 4b370083f2..627ed19951 100644 --- a/internal/buf/bufcli/bufcli.go +++ b/internal/buf/bufcli/bufcli.go @@ -44,7 +44,7 @@ type EnvReader interface { configOverride string, externalFilePaths []string, externalFileFilePathsAllowNotExist bool, - excludeSourceInfo bool, + excludeSourceCodeInfo bool, ) (Env, []bufanalysis.FileAnnotation, error) // GetImageEnv is the same as GetEnv but only allows image values and never builds. GetImageEnv( @@ -54,6 +54,7 @@ type EnvReader interface { configOverride string, externalFilePaths []string, externalFileFilePathsAllowNotExist bool, + excludeSourceCodeInfo bool, ) (Env, error) // GetSourceEnv is the same as GetEnv but only allows source values and always builds. GetSourceEnv( @@ -63,8 +64,17 @@ type EnvReader interface { configOverride string, externalFilePaths []string, externalFileFilePathsAllowNotExist bool, - excludeSourceInfo bool, + excludeSourceCodeInfo bool, ) (Env, []bufanalysis.FileAnnotation, error) + // GetImage is the same as GetImageEnv but does not get a configuration. + GetImage( + ctx context.Context, + container app.EnvStdinContainer, + value string, + externalFilePaths []string, + externalFileFilePathsAllowNotExist bool, + excludeSourceCodeInfo bool, + ) (bufimage.Image, error) // ListFiles lists the files. ListFiles( ctx context.Context, diff --git a/internal/buf/bufcli/env_reader.go b/internal/buf/bufcli/env_reader.go index 5d40713854..eac19aeb9f 100644 --- a/internal/buf/bufcli/env_reader.go +++ b/internal/buf/bufcli/env_reader.go @@ -98,6 +98,7 @@ func (e *envReader) GetEnv( configOverride, externalFilePaths, externalFilePathsAllowNotExist, + excludeSourceCodeInfo, t, ) return env, nil, err @@ -123,6 +124,7 @@ func (e *envReader) GetImageEnv( configOverride string, externalFilePaths []string, externalFilePathsAllowNotExist bool, + excludeSourceCodeInfo bool, ) (_ Env, retErr error) { defer instrument.Start(e.logger, "get_image_env").End() defer func() { @@ -141,6 +143,7 @@ func (e *envReader) GetImageEnv( configOverride, externalFilePaths, externalFilePathsAllowNotExist, + excludeSourceCodeInfo, imageRef, ) } @@ -176,6 +179,35 @@ func (e *envReader) GetSourceEnv( ) } +func (e *envReader) GetImage( + ctx context.Context, + container app.EnvStdinContainer, + value string, + externalFilePaths []string, + externalFilePathsAllowNotExist bool, + excludeSourceCodeInfo bool, +) (_ bufimage.Image, retErr error) { + defer instrument.Start(e.logger, "get_image").End() + defer func() { + if retErr != nil { + retErr = fmt.Errorf("%v: %w", e.valueFlagName, retErr) + } + }() + + imageRef, err := e.fetchRefParser.GetImageRef(ctx, value) + if err != nil { + return nil, err + } + return e.getImage( + ctx, + container, + externalFilePaths, + externalFilePathsAllowNotExist, + excludeSourceCodeInfo, + imageRef, + ) +} + func (e *envReader) ListFiles( ctx context.Context, container app.EnvStdinContainer, @@ -194,7 +226,14 @@ func (e *envReader) ListFiles( switch t := ref.(type) { case buffetch.ImageRef: // if we have an image, list the files in the image - image, err := e.getImage(ctx, container, t) + image, err := e.getImage( + ctx, + container, + nil, + false, + true, + t, + ) if err != nil { return nil, err } @@ -250,9 +289,17 @@ func (e *envReader) getEnvFromImage( configOverride string, externalFilePaths []string, externalFilePathsAllowNotExist bool, + excludeSourceCodeInfo bool, imageRef buffetch.ImageRef, ) (_ Env, retErr error) { - image, err := e.getImage(ctx, container, imageRef) + image, err := e.getImage( + ctx, + container, + externalFilePaths, + externalFilePathsAllowNotExist, + excludeSourceCodeInfo, + imageRef, + ) if err != nil { return nil, err } @@ -260,26 +307,6 @@ func (e *envReader) getEnvFromImage( if err != nil { return nil, err } - if len(externalFilePaths) > 0 { - // this is usually a full rel file path, but because this is a read image, - // the root is always ".", so this is OK, although awkward - rootRelFilePaths := make([]string, len(externalFilePaths)) - for i, externalFilePath := range externalFilePaths { - rootRelFilePath, err := imageRef.PathResolver().ExternalPathToRelPath(externalFilePath) - if err != nil { - return nil, err - } - rootRelFilePaths[i] = rootRelFilePath - } - if externalFilePathsAllowNotExist { - image, err = bufimage.ImageWithOnlyRootRelFilePathsAllowNotExist(image, rootRelFilePaths) - } else { - image, err = bufimage.ImageWithOnlyRootRelFilePaths(image, rootRelFilePaths) - } - if err != nil { - return nil, err - } - } return newEnv(image, config), nil } @@ -353,6 +380,9 @@ func (e *envReader) getEnvFromSource( func (e *envReader) getImage( ctx context.Context, container app.EnvStdinContainer, + externalFilePaths []string, + externalFilePathsAllowNotExist bool, + excludeSourceCodeInfo bool, imageRef buffetch.ImageRef, ) (_ bufimage.Image, retErr error) { readCloser, err := e.fetchReader.GetImageFile(ctx, container, imageRef) @@ -366,21 +396,84 @@ func (e *envReader) getImage( if err != nil { return nil, err } - // we cannot determine fileDescriptorProtos ahead of time so we cannot handle extensions - // TODO: we do not happen to need them for our use case with linting, but we need to dicuss this protoImage := &imagev1.Image{} switch imageEncoding := imageRef.ImageEncoding(); imageEncoding { + // we have to double parse due to custom options + // See https://github.com/golang/protobuf/issues/1123 + // TODO: revisit case buffetch.ImageEncodingBin: - err = protoencoding.NewWireUnmarshaler(nil).Unmarshal(data, protoImage) + firstProtoImage := &imagev1.Image{} + timer := instrument.Start(e.logger, "first_wire_unmarshal") + if err := protoencoding.NewWireUnmarshaler(nil).Unmarshal(data, firstProtoImage); err != nil { + return nil, fmt.Errorf("could not unmarshal Image: %v", err) + } + timer.End() + timer = instrument.Start(e.logger, "new_resolver") + // TODO right now, NewResolver sets AllowUnresolvable to true all the time + // we want to make this into a check, and we verify if we need this for the individual command + resolver, err := protoencoding.NewResolver( + firstProtoImage.File..., + ) + if err != nil { + return nil, err + } + timer.End() + timer = instrument.Start(e.logger, "second_wire_unmarshal") + if err := protoencoding.NewWireUnmarshaler(resolver).Unmarshal(data, protoImage); err != nil { + return nil, fmt.Errorf("could not unmarshal Image: %v", err) + } + timer.End() case buffetch.ImageEncodingJSON: - err = protoencoding.NewJSONUnmarshaler(nil).Unmarshal(data, protoImage) + firstProtoImage := &imagev1.Image{} + timer := instrument.Start(e.logger, "first_json_unmarshal") + if err := protoencoding.NewJSONUnmarshaler(nil).Unmarshal(data, firstProtoImage); err != nil { + return nil, fmt.Errorf("could not unmarshal Image: %v", err) + } + // TODO right now, NewResolver sets AllowUnresolvable to true all the time + // we want to make this into a check, and we verify if we need this for the individual command + timer.End() + timer = instrument.Start(e.logger, "new_resolver") + resolver, err := protoencoding.NewResolver( + firstProtoImage.File..., + ) + if err != nil { + return nil, err + } + timer.End() + timer = instrument.Start(e.logger, "second_json_unmarshal") + if err := protoencoding.NewJSONUnmarshaler(resolver).Unmarshal(data, protoImage); err != nil { + return nil, fmt.Errorf("could not unmarshal Image: %v", err) + } + timer.End() default: return nil, fmt.Errorf("unknown image encoding: %v", imageEncoding) } + if excludeSourceCodeInfo { + for _, fileDescriptorProto := range protoImage.File { + fileDescriptorProto.SourceCodeInfo = nil + } + } + image, err := bufimage.NewImageForProto(protoImage) if err != nil { - return nil, fmt.Errorf("could not unmarshal Image: %v", err) + return nil, err + } + if len(externalFilePaths) == 0 { + return image, nil + } + // this is usually a full rel file path, but because this is a read image, + // the root is always ".", so this is OK, although awkward + rootRelFilePaths := make([]string, len(externalFilePaths)) + for i, externalFilePath := range externalFilePaths { + rootRelFilePath, err := imageRef.PathResolver().ExternalPathToRelPath(externalFilePath) + if err != nil { + return nil, err + } + rootRelFilePaths[i] = rootRelFilePath + } + if externalFilePathsAllowNotExist { + return bufimage.ImageWithOnlyRootRelFilePathsAllowNotExist(image, rootRelFilePaths) } - return bufimage.NewImageForProto(protoImage) + return bufimage.ImageWithOnlyRootRelFilePaths(image, rootRelFilePaths) } func (e *envReader) getSourceBucketAndConfig( diff --git a/internal/buf/cmd/buf/buf.go b/internal/buf/cmd/buf/buf.go index fba16437df..f4e92edc31 100644 --- a/internal/buf/cmd/buf/buf.go +++ b/internal/buf/cmd/buf/buf.go @@ -21,7 +21,7 @@ import ( "github.com/bufbuild/buf/internal/pkg/app/appflag" ) -const version = "0.17.0" +const version = "0.18.0-dev" // Main is the main. func Main(use string, options ...RootCommandOption) { diff --git a/internal/buf/cmd/buf/buf_test.go b/internal/buf/cmd/buf/buf_test.go index 83ea624e41..e47e8b73a3 100644 --- a/internal/buf/cmd/buf/buf_test.go +++ b/internal/buf/cmd/buf/buf_test.go @@ -18,6 +18,7 @@ import ( "bytes" "context" "fmt" + "io" "io/ioutil" "os" "path/filepath" @@ -31,35 +32,43 @@ import ( ) func TestSuccess1(t *testing.T) { - testRun(t, 0, ``, "image", "build", "-o", app.DevNullFilePath, "--source", filepath.Join("testdata", "success")) + t.Parallel() + testRunStdout(t, 0, ``, "image", "build", "-o", app.DevNullFilePath, "--source", filepath.Join("testdata", "success")) } func TestSuccess2(t *testing.T) { - testRun(t, 0, ``, "image", "build", "-o", app.DevNullFilePath, "--exclude-imports", "--source", filepath.Join("testdata", "success")) + t.Parallel() + testRunStdout(t, 0, ``, "image", "build", "-o", app.DevNullFilePath, "--exclude-imports", "--source", filepath.Join("testdata", "success")) } func TestSuccess3(t *testing.T) { - testRun(t, 0, ``, "image", "build", "-o", app.DevNullFilePath, "--exclude-source-info", "--source", filepath.Join("testdata", "success")) + t.Parallel() + testRunStdout(t, 0, ``, "image", "build", "-o", app.DevNullFilePath, "--exclude-source-info", "--source", filepath.Join("testdata", "success")) } func TestSuccess4(t *testing.T) { - testRun(t, 0, ``, "image", "build", "-o", app.DevNullFilePath, "--exclude-imports", "--exclude-source-info", "--source", filepath.Join("testdata", "success")) + t.Parallel() + testRunStdout(t, 0, ``, "image", "build", "-o", app.DevNullFilePath, "--exclude-imports", "--exclude-source-info", "--source", filepath.Join("testdata", "success")) } func TestSuccess5(t *testing.T) { - testRun(t, 0, ``, "image", "build", "-o", app.DevNullFilePath, "--exclude-imports", "--exclude-source-info", "-o", app.DevNullFilePath, "--source", filepath.Join("testdata", "success")) + t.Parallel() + testRunStdout(t, 0, ``, "image", "build", "-o", app.DevNullFilePath, "--exclude-imports", "--exclude-source-info", "-o", app.DevNullFilePath, "--source", filepath.Join("testdata", "success")) } func TestSuccess6(t *testing.T) { - testRun(t, 0, ``, "check", "lint", "--input", filepath.Join("testdata", "success")) + t.Parallel() + testRunStdout(t, 0, ``, "check", "lint", "--input", filepath.Join("testdata", "success")) } func TestSuccessProfile1(t *testing.T) { - testRunProfile(t, 0, ``, "image", "build", "-o", app.DevNullFilePath, "--source", filepath.Join("testdata", "success")) + t.Parallel() + testRunStdoutProfile(t, 0, ``, "image", "build", "-o", app.DevNullFilePath, "--source", filepath.Join("testdata", "success")) } func TestFail1(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 0, ``, @@ -70,7 +79,8 @@ func TestFail1(t *testing.T) { } func TestFail2(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 0, ``, @@ -82,7 +92,8 @@ func TestFail2(t *testing.T) { } func TestFail3(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 0, ``, @@ -94,7 +105,8 @@ func TestFail3(t *testing.T) { } func TestFail4(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 0, ``, @@ -107,7 +119,8 @@ func TestFail4(t *testing.T) { } func TestFail5(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 1, `testdata/fail/buf/buf.proto:3:1:Files with package "other" must be within a directory "other" relative to root but were in directory "buf". @@ -120,7 +133,8 @@ func TestFail5(t *testing.T) { } func TestFail6(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 1, `testdata/fail/buf/buf.proto:3:1:Files with package "other" must be within a directory "other" relative to root but were in directory "buf". @@ -135,7 +149,8 @@ func TestFail6(t *testing.T) { } func TestFail7(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 1, `testdata/fail/buf/buf.proto:3:1:Files with package "other" must be within a directory "other" relative to root but were in directory "fail/buf". @@ -152,7 +167,8 @@ func TestFail7(t *testing.T) { } func TestFail8(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 1, `testdata/fail2/buf/buf.proto:6:9:Field name "oneTwo" should be lower_snake_case, such as "one_two". @@ -165,7 +181,8 @@ func TestFail8(t *testing.T) { } func TestFail9(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 1, `testdata/fail2/buf/buf.proto:6:9:Field name "oneTwo" should be lower_snake_case, such as "one_two".`, @@ -179,7 +196,8 @@ func TestFail9(t *testing.T) { } func TestFail10(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 0, ``, @@ -193,7 +211,8 @@ func TestFail10(t *testing.T) { } func TestFail11(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 1, `testdata/fail2/buf/buf2.proto:5:8:buf/buf.proto: does not exist`, @@ -207,7 +226,8 @@ func TestFail11(t *testing.T) { } func TestFail12(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 1, `lint: @@ -226,7 +246,8 @@ func TestFail12(t *testing.T) { } func TestFailCheckBreaking1(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 1, ` @@ -247,7 +268,8 @@ func TestFailCheckBreaking1(t *testing.T) { } func TestCheckLsLintCheckers1(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 0, ` @@ -264,7 +286,8 @@ func TestCheckLsLintCheckers1(t *testing.T) { } func TestCheckLsLintCheckers2(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 0, ` @@ -280,7 +303,8 @@ func TestCheckLsLintCheckers2(t *testing.T) { } func TestCheckLsBreakingCheckers1(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 0, ` @@ -313,7 +337,8 @@ func TestCheckLsBreakingCheckers1(t *testing.T) { } func TestCheckLsBreakingCheckers2(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 0, ` @@ -329,7 +354,8 @@ func TestCheckLsBreakingCheckers2(t *testing.T) { } func TestLsFiles(t *testing.T) { - testRun( + t.Parallel() + testRunStdout( t, 0, ` @@ -341,23 +367,131 @@ func TestLsFiles(t *testing.T) { ) } -func testRun(t *testing.T, expectedExitCode int, expectedStdout string, args ...string) { - testRunCmd( +func TestImageConvertRoundtripBinaryJSONBinary(t *testing.T) { + t.Parallel() + + stdout := bytes.NewBuffer(nil) + testRun( + t, + 0, + nil, + stdout, + "image", + "build", + "-o", + "-", + "--source", + filepath.Join("testdata", "customoptions1"), + ) + + binary1 := stdout.Bytes() + require.NotEmpty(t, binary1) + + stdin := stdout + stdout = bytes.NewBuffer(nil) + testRun( + t, + 0, + stdin, + stdout, + "experimental", + "image", + "convert", + "-i", + "-", + "-o", + "-#format=json", + ) + + stdin = stdout + stdout = bytes.NewBuffer(nil) + testRun( + t, + 0, + stdin, + stdout, + "experimental", + "image", + "convert", + "-i", + "-#format=json", + "-o", + "-", + ) + + require.Equal(t, binary1, stdout.Bytes()) +} + +func TestImageConvertRoundtripJSONBinaryJSON(t *testing.T) { + t.Parallel() + + stdout := bytes.NewBuffer(nil) + testRun( + t, + 0, + nil, + stdout, + "image", + "build", + "-o", + "-#format=json", + "--source", + filepath.Join("testdata", "customoptions1"), + ) + + json1 := stdout.Bytes() + require.NotEmpty(t, json1) + + stdin := stdout + stdout = bytes.NewBuffer(nil) + testRun( + t, + 0, + stdin, + stdout, + "experimental", + "image", + "convert", + "-i", + "-#format=json", + "-o", + "-", + ) + + stdin = stdout + stdout = bytes.NewBuffer(nil) + testRun( + t, + 0, + stdin, + stdout, + "experimental", + "image", + "convert", + "-i", + "-", + "-o", + "-#format=json", + ) + + require.Equal(t, json1, stdout.Bytes()) +} + +func testRunStdout(t *testing.T, expectedExitCode int, expectedStdout string, args ...string) { + testRunStdoutInternal( t, - newRootCommand("test"), expectedExitCode, expectedStdout, args..., ) } -func testRunProfile(t *testing.T, expectedExitCode int, expectedStdout string, args ...string) { +func testRunStdoutProfile(t *testing.T, expectedExitCode int, expectedStdout string, args ...string) { profileDirPath, err := ioutil.TempDir("", "") require.NoError(t, err) defer func() { assert.NoError(t, os.RemoveAll(profileDirPath)) }() - testRunCmd( + testRunStdoutInternal( t, - newRootCommand("test"), 0, ``, append( @@ -370,26 +504,33 @@ func testRunProfile(t *testing.T, expectedExitCode int, expectedStdout string, a ) } -func testRunCmd(t *testing.T, cmd *appcmd.Command, expectedExitCode int, expectedStdout string, args ...string) { - t.Parallel() +func testRunStdoutInternal(t *testing.T, expectedExitCode int, expectedStdout string, args ...string) { stdout := bytes.NewBuffer(nil) + testRun(t, expectedExitCode, nil, stdout, args...) + require.Equal(t, stringutil.TrimLines(expectedStdout), stringutil.TrimLines(stdout.String())) +} + +func testRun( + t *testing.T, + expectedExitCode int, + stdin io.Reader, + stdout io.Writer, + args ...string, +) { stderr := bytes.NewBuffer(nil) exitCode := app.GetExitCode( appcmd.Run( context.Background(), app.NewContainer( nil, - nil, + stdin, stdout, stderr, append([]string{"test"}, args...)..., ), - cmd, + newRootCommand("test"), "test", ), ) - assert.Equal(t, expectedExitCode, exitCode, stringutil.TrimLines(stderr.String())) - if exitCode == expectedExitCode { - assert.Equal(t, stringutil.TrimLines(expectedStdout), stringutil.TrimLines(stdout.String()), stringutil.TrimLines(stderr.String())) - } + require.Equal(t, expectedExitCode, exitCode, stringutil.TrimLines(stderr.String())) } diff --git a/internal/buf/cmd/buf/cmds.go b/internal/buf/cmd/buf/cmds.go index c7edef4b16..7cbe75cc6c 100644 --- a/internal/buf/cmd/buf/cmds.go +++ b/internal/buf/cmd/buf/cmds.go @@ -27,6 +27,7 @@ func newRootCommand(use string, options ...RootCommandOption) *appcmd.Command { newImageCmd(builder), newCheckCmd(builder), newLsFilesCmd(builder), + newExperimentalCmd(builder), }, BindFlags: builder.BindRoot, } @@ -36,6 +37,16 @@ func newRootCommand(use string, options ...RootCommandOption) *appcmd.Command { return rootCommand } +func newExperimentalCmd(builder *builder) *appcmd.Command { + return &appcmd.Command{ + Use: "experimental", + Short: "Experimental commands. Unstable and will likely change.", + SubCommands: []*appcmd.Command{ + newExperimentalImageCmd(builder), + }, + } +} + func newImageCmd(builder *builder) *appcmd.Command { return &appcmd.Command{ Use: "image", @@ -46,10 +57,20 @@ func newImageCmd(builder *builder) *appcmd.Command { } } +func newExperimentalImageCmd(builder *builder) *appcmd.Command { + return &appcmd.Command{ + Use: "image", + Short: "Work with Images and FileDescriptorSets.", + SubCommands: []*appcmd.Command{ + newImageConvertCmd(builder), + }, + } +} + func newImageBuildCmd(builder *builder) *appcmd.Command { return &appcmd.Command{ Use: "build", - Short: "Build all files from the input location and output an Image or FileDescriptorSet.", + Short: "Build all files from the input location and output an Image or FileDescriptorSet.", Args: cobra.NoArgs, Run: builder.newRunFunc(imageBuild), BindFlags: appcmd.BindMultiple( @@ -66,6 +87,23 @@ func newImageBuildCmd(builder *builder) *appcmd.Command { } } +func newImageConvertCmd(builder *builder) *appcmd.Command { + return &appcmd.Command{ + Use: "convert", + Short: "Convert the input Image to an output Image with the specified format and filters.", + Args: cobra.NoArgs, + Run: builder.newRunFunc(imageConvert), + BindFlags: appcmd.BindMultiple( + builder.bindImageConvertInput, + builder.bindImageConvertFiles, + builder.bindImageConvertOutput, + builder.bindImageConvertAsFileDescriptorSet, + builder.bindImageConvertExcludeImports, + builder.bindImageConvertExcludeSourceInfo, + ), + } +} + func newCheckCmd(builder *builder) *appcmd.Command { return &appcmd.Command{ Use: "check", diff --git a/internal/buf/cmd/buf/flags.go b/internal/buf/cmd/buf/flags.go index f2e60df09c..68a843d402 100644 --- a/internal/buf/cmd/buf/flags.go +++ b/internal/buf/cmd/buf/flags.go @@ -30,6 +30,8 @@ const ( imageBuildInputFlagName = "source" imageBuildConfigFlagName = "source-config" imageBuildOutputFlagName = "output" + imageConvertInputFlagName = "image" + imageConvertOutputFlagName = "output" checkLintInputFlagName = "input" checkLintConfigFlagName = "input-config" checkBreakingInputFlagName = "input" @@ -50,6 +52,7 @@ type flags struct { AgainstConfig string Input string AgainstInput string + ConvertInput string Output string AsFileDescriptorSet bool ExcludeImports bool @@ -142,6 +145,35 @@ func (b *builder) bindImageBuildErrorFormat(flagSet *pflag.FlagSet) { flagSet.StringVar(&b.ErrorFormat, errorFormatFlagName, "text", "The format for build errors, printed to stderr. Must be one of [text,json].") } +func (b *builder) bindImageConvertInput(flagSet *pflag.FlagSet) { + // TODO: cobra cannot have the same variable with different inputs, we need + // to refactor the variables to have different binds per function + flagSet.StringVarP(&b.ConvertInput, imageConvertInputFlagName, "i", "", fmt.Sprintf(`The image to convert. Must be one of format %s.`, buffetch.ImageFormatsString)) +} + +func (b *builder) bindImageConvertFiles(flagSet *pflag.FlagSet) { + flagSet.StringSliceVar(&b.Files, "file", nil, `Limit to specific files. This is an advanced feature and is not recommended.`) +} + +func (b *builder) bindImageConvertOutput(flagSet *pflag.FlagSet) { + flagSet.StringVarP(&b.Output, imageConvertOutputFlagName, "o", "", fmt.Sprintf(`Required. The location to write the image to. Must be one of format %s.`, buffetch.ImageFormatsString)) +} + +func (b *builder) bindImageConvertAsFileDescriptorSet(flagSet *pflag.FlagSet) { + flagSet.BoolVar(&b.AsFileDescriptorSet, "as-file-descriptor-set", false, `Output as a google.protobuf.FileDescriptorSet instead of an image. + +Note that images are wire-compatible with FileDescriptorSets, however this flag will strip +the additional metadata added for Buf usage.`) +} + +func (b *builder) bindImageConvertExcludeImports(flagSet *pflag.FlagSet) { + flagSet.BoolVar(&b.ExcludeImports, "exclude-imports", false, "Exclude imports.") +} + +func (b *builder) bindImageConvertExcludeSourceInfo(flagSet *pflag.FlagSet) { + flagSet.BoolVar(&b.ExcludeSourceInfo, "exclude-source-info", false, "Exclude source info.") +} + func (b *builder) bindCheckLintInput(flagSet *pflag.FlagSet) { flagSet.StringVar(&b.Input, checkLintInputFlagName, ".", fmt.Sprintf(`The source or image to lint. Must be one of format %s.`, buffetch.AllFormatsString)) } diff --git a/internal/buf/cmd/buf/run.go b/internal/buf/cmd/buf/run.go index 3e74ecd00c..da24ecc7d7 100644 --- a/internal/buf/cmd/buf/run.go +++ b/internal/buf/cmd/buf/run.go @@ -71,6 +71,37 @@ func imageBuild(ctx context.Context, container *container) (retErr error) { ) } +func imageConvert(ctx context.Context, container *container) (retErr error) { + if container.Output == "" { + return fmt.Errorf("--%s is required", imageBuildOutputFlagName) + } + image, err := internal.NewBufcliEnvReader( + container.Logger(), + imageConvertInputFlagName, + "", + ).GetImage( + ctx, + container, + container.ConvertInput, + container.Files, + false, + container.ExcludeSourceInfo, + ) + if err != nil { + return err + } + return internal.NewBufcliImageWriter( + container.Logger(), + ).PutImage( + ctx, + container, + container.Output, + image, + container.AsFileDescriptorSet, + container.ExcludeImports, + ) +} + func checkLint(ctx context.Context, container *container) (retErr error) { asJSON, err := internal.IsLintFormatJSON(errorFormatFlagName, container.ErrorFormat) if err != nil { diff --git a/internal/buf/cmd/buf/testdata/customoptions1/a.proto b/internal/buf/cmd/buf/testdata/customoptions1/a.proto new file mode 100644 index 0000000000..541c86c568 --- /dev/null +++ b/internal/buf/cmd/buf/testdata/customoptions1/a.proto @@ -0,0 +1,11 @@ +syntax = "proto3"; + +import "google/protobuf/descriptor.proto"; + +extend google.protobuf.FieldOptions { + int32 baz = 50007; +} + +message Foo { + string bar = 1 [(baz) = 42]; +} diff --git a/internal/buf/cmd/protoc-gen-buf-check-breaking/breaking.go b/internal/buf/cmd/protoc-gen-buf-check-breaking/breaking.go index bbcaa72d06..819b73f679 100644 --- a/internal/buf/cmd/protoc-gen-buf-check-breaking/breaking.go +++ b/internal/buf/cmd/protoc-gen-buf-check-breaking/breaking.go @@ -80,6 +80,7 @@ func handle( encoding.GetJSONStringOrStringValue(externalConfig.AgainstInputConfig), files, // limit to the input files if specified true, // allow files in the against input to not exist + false, // keep for now ) if err != nil { responseWriter.WriteError(err.Error()) diff --git a/internal/pkg/proto/protoencoding/json_marshaler.go b/internal/pkg/proto/protoencoding/json_marshaler.go index b2f20db983..671ea2e2c0 100644 --- a/internal/pkg/proto/protoencoding/json_marshaler.go +++ b/internal/pkg/proto/protoencoding/json_marshaler.go @@ -20,6 +20,7 @@ import ( "google.golang.org/protobuf/encoding/protojson" "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/reflect/protoreflect" ) type jsonMarshaler struct { @@ -37,6 +38,9 @@ func newJSONMarshaler(resolver Resolver, indent string, useProtoNames bool) Mars } func (m *jsonMarshaler) Marshal(message proto.Message) ([]byte, error) { + if err := reparseUnrecognized(m.resolver, message.ProtoReflect()); err != nil { + return nil, err + } options := protojson.MarshalOptions{ Resolver: m.resolver, Indent: m.indent, @@ -60,3 +64,56 @@ func (m *jsonMarshaler) Marshal(message proto.Message) ([]byte, error) { } return buffer.Bytes(), nil } + +func reparseUnrecognized(resolver Resolver, reflectMessage protoreflect.Message) error { + if resolver == nil { + return nil + } + unknown := reflectMessage.GetUnknown() + if len(unknown) > 0 { + reflectMessage.SetUnknown(nil) + options := proto.UnmarshalOptions{ + Resolver: resolver, + Merge: true, + } + if err := options.Unmarshal(unknown, reflectMessage.Interface()); err != nil { + return err + } + } + var err error + reflectMessage.Range(func(fieldDescriptor protoreflect.FieldDescriptor, value protoreflect.Value) bool { + err = reparseUnrecognizedInField(resolver, fieldDescriptor, value) + return err == nil + }) + return err +} + +func reparseUnrecognizedInField(resolver Resolver, fieldDescriptor protoreflect.FieldDescriptor, value protoreflect.Value) error { + if fieldDescriptor.IsMap() { + valDesc := fieldDescriptor.MapValue() + if valDesc.Kind() != protoreflect.MessageKind && valDesc.Kind() != protoreflect.GroupKind { + // nothing to reparse + return nil + } + var err error + value.Map().Range(func(k protoreflect.MapKey, v protoreflect.Value) bool { + err = reparseUnrecognized(resolver, v.Message()) + return err == nil + }) + return err + } + if fieldDescriptor.Kind() != protoreflect.MessageKind && fieldDescriptor.Kind() != protoreflect.GroupKind { + // nothing to reparse + return nil + } + if fieldDescriptor.IsList() { + list := value.List() + for i := 0; i < list.Len(); i++ { + if err := reparseUnrecognized(resolver, list.Get(i).Message()); err != nil { + return err + } + } + return nil + } + return reparseUnrecognized(resolver, value.Message()) +} diff --git a/internal/pkg/proto/protoencoding/json_unmarshaler.go b/internal/pkg/proto/protoencoding/json_unmarshaler.go index 6d6158212e..783391e4f7 100644 --- a/internal/pkg/proto/protoencoding/json_unmarshaler.go +++ b/internal/pkg/proto/protoencoding/json_unmarshaler.go @@ -32,6 +32,8 @@ func newJSONUnmarshaler(resolver Resolver) Unmarshaler { func (m *jsonUnmarshaler) Unmarshal(data []byte, message proto.Message) error { options := protojson.UnmarshalOptions{ Resolver: m.resolver, + // TODO: make this an option + DiscardUnknown: true, } return options.Unmarshal(data, message) } diff --git a/internal/pkg/proto/protoencoding/resolver.go b/internal/pkg/proto/protoencoding/resolver.go index 3127fad40f..cd89bb00db 100644 --- a/internal/pkg/proto/protoencoding/resolver.go +++ b/internal/pkg/proto/protoencoding/resolver.go @@ -26,7 +26,10 @@ func newResolver(fileDescriptorProtos ...*descriptorpb.FileDescriptorProto) (Res if len(fileDescriptorProtos) == 0 { return nil, nil } - files, err := protodesc.NewFiles( + // TODO: handle if resolvable + files, err := protodesc.FileOptions{ + AllowUnresolvable: true, + }.NewFiles( &descriptorpb.FileDescriptorSet{ File: fileDescriptorProtos, },