Skip to content

Commit 23472ca

Browse files
doriabledoriable
authored
Allow workspaces adding new module(s) with no module-specifc breaking configurations to run buf breaking (#3864)
1 parent fae9f4e commit 23472ca

File tree

15 files changed

+231
-19
lines changed

15 files changed

+231
-19
lines changed

‎CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
- Promote `buf beta stats` to `buf stats`.
66
- Update built-in Well-Known Types to Protobuf v31.1.
77
- Add `buf registry sdk info` command.
8+
- Allow workspaces that are adding new module(s) with no module-specific breaking configurations
9+
to run `buf breaking`, ignoring new module(s).
810

911
## [v1.54.0] - 2025-05-12
1012

‎private/buf/cmd/buf/command/breaking/breaking.go

Lines changed: 111 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ import (
1818
"context"
1919
"errors"
2020
"fmt"
21+
"maps"
22+
"slices"
2123

2224
"buf.build/go/app/appcmd"
2325
"buf.build/go/app/appext"
@@ -262,17 +264,28 @@ func run(
262264
}
263265
}
264266
if len(imageWithConfigs) != len(againstImages) {
265-
// If workspaces are being used as input, the number
266-
// of images MUST match. Otherwise the results will
267-
// be meaningless and yield false positives.
267+
// In the case where the input and against workspaces do not contain the same number of
268+
// images, this could happen if the input contains new module(s). However, we require
269+
// the number of images to match because of module-specific [bufconfig.BreakingConfig].
270+
// This can result in a less satisfying UX when adding modules to a workspace.
268271
//
269-
// And similar to the note above, if the roots change,
270-
// we're torched.
271-
return fmt.Errorf(
272-
"input contained %d images, whereas against contained %d images",
273-
len(imageWithConfigs),
274-
len(againstImages),
275-
)
272+
// To mitigate this for users adding new modules to their workspace, for the case where
273+
// len(imageWithConfigs) > len(againstImages), if all modules in [imageWithConfigs] have
274+
// the same [bufconfig.BreakingConfig] (so no unique, module-specific [bufconfig.BreakingConfig]),
275+
// we query the [againstImages] for the matching modules and ignore any modules from
276+
// [imageWithConfigs] that are not found in [againstImages].
277+
//
278+
// In the case where len(imageWithConfigs) < len(againstImages) or there are module-specific
279+
// [bufconfig.BreakingConfig], we still return an error. Also, if the roots change, we're
280+
// torched. (Issue #3641)
281+
if len(imageWithConfigs) > len(againstImages) && hasNoUniqueBreakingConfig(imageWithConfigs) {
282+
imageWithConfigs, err = filterImageWithConfigsNotInAgainstImages(imageWithConfigs, againstImages)
283+
if err != nil {
284+
return err
285+
}
286+
} else {
287+
return newInputAgainstImageCountError(len(imageWithConfigs), len(againstImages))
288+
}
276289
}
277290
// We add all check configs (both lint and breaking) as related configs to check if plugins
278291
// have rules configured.
@@ -340,3 +353,91 @@ func validateFlags(flags *flags) error {
340353
}
341354
return nil
342355
}
356+
357+
// hasNoUniqueBreakingConfig iterates through imageWithConfigs and checks to see if there
358+
// are any unique [bufconfig.BreakingConfig]. It returns true if all [bufconfig.BreakingConfig]
359+
// are the same across all the images.
360+
func hasNoUniqueBreakingConfig(imageWithConfigs []bufctl.ImageWithConfig) bool {
361+
var base bufconfig.BreakingConfig
362+
for _, imageWithConfig := range imageWithConfigs {
363+
if base == nil {
364+
base = imageWithConfig.BreakingConfig()
365+
continue
366+
}
367+
if !equalBreakingConfig(base, imageWithConfig.BreakingConfig()) {
368+
return false
369+
}
370+
base = imageWithConfig.BreakingConfig()
371+
}
372+
return true
373+
}
374+
375+
// Checks if the specified [bufconfig.BreakingConfig]s are equal. Returns true if both
376+
// [bufconfig.BreakingConfig] have the same configuration parameters.
377+
func equalBreakingConfig(breakingConfig1, breakingConfig2 bufconfig.BreakingConfig) bool {
378+
if breakingConfig1.Disabled() == breakingConfig2.Disabled() &&
379+
breakingConfig1.FileVersion() == breakingConfig2.FileVersion() &&
380+
slices.Equal(breakingConfig1.UseIDsAndCategories(), breakingConfig2.UseIDsAndCategories()) &&
381+
slices.Equal(breakingConfig1.ExceptIDsAndCategories(), breakingConfig2.ExceptIDsAndCategories()) &&
382+
slices.Equal(breakingConfig1.IgnorePaths(), breakingConfig2.IgnorePaths()) &&
383+
maps.EqualFunc(
384+
breakingConfig1.IgnoreIDOrCategoryToPaths(),
385+
breakingConfig2.IgnoreIDOrCategoryToPaths(),
386+
slices.Equal[[]string],
387+
) &&
388+
breakingConfig1.DisableBuiltin() == breakingConfig2.DisableBuiltin() &&
389+
breakingConfig1.IgnoreUnstablePackages() == breakingConfig2.IgnoreUnstablePackages() {
390+
return true
391+
}
392+
return false
393+
}
394+
395+
// A helper function for filtering out [bufctl.ImageWithConfig]s from [imagesWithConfig]
396+
// if there is no corresponding image in [againstImages]. We determine this based on image
397+
// file path.
398+
//
399+
// This assumes that len(imageWithConfigs) > len(againstImages).
400+
// We also expect that each image in [againstImages] is mapped only once to a single
401+
// imageWithConfig in [imagesWithConfig]. If an againstImage is found, then we don't check
402+
// it again. We also validate that each image in [againstImages] is mapped to an imageWithConfig
403+
// from [imageWithConfigs].
404+
func filterImageWithConfigsNotInAgainstImages(
405+
imageWithConfigs []bufctl.ImageWithConfig,
406+
againstImages []bufimage.Image,
407+
) ([]bufctl.ImageWithConfig, error) {
408+
foundAgainstImageIndices := make(map[int]struct{})
409+
var filteredImageWithConfigs []bufctl.ImageWithConfig
410+
for _, imageWithConfig := range imageWithConfigs {
411+
for _, imageFile := range imageWithConfig.Files() {
412+
var foundImage bufimage.Image
413+
for i, againstImage := range againstImages {
414+
if _, ok := foundAgainstImageIndices[i]; ok {
415+
continue
416+
}
417+
if againstImage.GetFile(imageFile.Path()) != nil {
418+
foundAgainstImageIndices[i] = struct{}{}
419+
foundImage = againstImage
420+
break
421+
}
422+
}
423+
if foundImage != nil {
424+
filteredImageWithConfigs = append(filteredImageWithConfigs, imageWithConfig)
425+
break
426+
}
427+
}
428+
}
429+
// If we are unsuccessful in mapping all againstImages to a unique imageWithConfig, then
430+
// we return the same error message.
431+
if len(foundAgainstImageIndices) != len(againstImages) || len(againstImages) != len(filteredImageWithConfigs) {
432+
return nil, newInputAgainstImageCountError(len(imageWithConfigs), len(againstImages))
433+
}
434+
return filteredImageWithConfigs, nil
435+
}
436+
437+
func newInputAgainstImageCountError(lenImageWithConfigs, lenAgainstImages int) error {
438+
return fmt.Errorf(
439+
"input contained %d images, whereas against contained %d images",
440+
lenImageWithConfigs,
441+
lenAgainstImages,
442+
)
443+
}

‎private/buf/cmd/buf/testdata/workspace/fail/breaking/other/proto/request.proto

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,6 @@ syntax = "proto3";
22

33
package request;
44

5-
message Request {}
5+
message Request {
6+
int64 name = 1;
7+
}

‎private/buf/cmd/buf/testdata/workspace/fail/modulebreakingconfig/a/proto/a/v1/a.proto

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
syntax = "proto3";
2+
3+
package a.v1;
4+
5+
message A {}

‎private/buf/cmd/buf/testdata/workspace/fail/modulebreakingconfig/a/proto/buf.yaml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
version: v1

‎private/buf/cmd/buf/testdata/workspace/fail/modulebreakingconfig/buf.work.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
version: v1
2+
directories:
3+
- a/proto
4+
- other/proto
5+
- proto

‎private/buf/cmd/buf/testdata/workspace/fail/modulebreakingconfig/other/proto/buf.yaml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
version: v1
2+
name: bufbuild.test/workspace/request

‎private/buf/cmd/buf/testdata/workspace/fail/modulebreakingconfig/other/proto/request.proto

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
syntax = "proto3";
2+
3+
package request;
4+
5+
message Request {
6+
string name = 1;
7+
}

‎private/buf/cmd/buf/testdata/workspace/fail/modulebreakingconfig/proto/buf.yaml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
version: v1
2+
name: bufbuild.test/workspace/rpc
3+
deps:
4+
- bufbuild.test/workspace/request
5+
breaking:
6+
use:
7+
- PACKAGE

‎private/buf/cmd/buf/testdata/workspace/fail/modulebreakingconfig/proto/rpc.proto

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
syntax = "proto3";
2+
3+
package example;
4+
5+
import "request.proto";
6+
7+
message RPC {
8+
request.Request req = 1;
9+
}

‎private/buf/cmd/buf/testdata/workspace/fail/v2/modulebreakingconfig/a/proto/a/v1/a.proto

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
syntax = "proto3";
2+
3+
package a.v1;
4+
5+
message A {}

‎private/buf/cmd/buf/testdata/workspace/fail/v2/modulebreakingconfig/buf.yaml

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
version: v2
2+
modules:
3+
- path: a/proto
4+
- path: other/proto
5+
name: bufbuild.test/workspace/request
6+
- path: proto
7+
name: bufbuild.test/workspace/rpc
8+
breaking:
9+
use:
10+
- PACKAGE

‎private/buf/cmd/buf/testdata/workspace/fail/v2/modulebreakingconfig/other/proto/request.proto

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
syntax = "proto3";
2+
3+
package request;
4+
5+
message Request {
6+
string name = 1;
7+
}

‎private/buf/cmd/buf/testdata/workspace/fail/v2/modulebreakingconfig/proto/rpc.proto

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
syntax = "proto3";
2+
3+
package example;
4+
5+
import "request.proto";
6+
7+
message RPC {
8+
request.Request req = 1;
9+
}

‎private/buf/cmd/buf/workspace_test.go

Lines changed: 48 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -982,8 +982,6 @@ func TestWorkspaceProtoFile(t *testing.T) {
982982

983983
func TestWorkspaceBreakingFail(t *testing.T) {
984984
t.Parallel()
985-
// The two workspaces define a different number of
986-
// images, so it's impossible to verify compatibility.
987985
testRunStdout(
988986
t,
989987
nil,
@@ -992,6 +990,7 @@ func TestWorkspaceBreakingFail(t *testing.T) {
992990
"build",
993991
filepath.Join("testdata", "workspace", "fail", "breaking"),
994992
)
993+
// If the against contained more images than the input, we return a mismatch.
995994
testRunStdoutStderrNoWarn(
996995
t,
997996
nil,
@@ -1014,6 +1013,53 @@ func TestWorkspaceBreakingFail(t *testing.T) {
10141013
"--against",
10151014
filepath.Join("testdata", "workspace", "success", "v2", "breaking"),
10161015
)
1016+
// If the input contains more images than the against, we attempt to map against images
1017+
// if there are no unique module-specific breaking configs for the input.
1018+
testRunStdout(
1019+
t,
1020+
nil,
1021+
bufctl.ExitCodeFileAnnotation,
1022+
filepath.FromSlash(
1023+
`testdata/workspace/success/dir/other/proto/request.proto:6:3:Field "1" with name "name" on message "Request" changed type from "int64" to "string".`,
1024+
),
1025+
"breaking",
1026+
filepath.Join("testdata", "workspace", "success", "dir"),
1027+
"--against",
1028+
filepath.Join("testdata", "workspace", "fail", "breaking"),
1029+
)
1030+
testRunStdout(
1031+
t,
1032+
nil,
1033+
bufctl.ExitCodeFileAnnotation,
1034+
filepath.FromSlash(
1035+
`testdata/workspace/success/v2/dir/other/proto/request.proto:6:3:Field "1" with name "name" on message "Request" changed type from "int64" to "string".`,
1036+
),
1037+
"breaking",
1038+
filepath.Join("testdata", "workspace", "success", "v2", "dir"),
1039+
"--against",
1040+
filepath.Join("testdata", "workspace", "fail", "breaking"),
1041+
)
1042+
// If we find unique module-specific breaking changes, then we still fail with a mismatch error.
1043+
testRunStdout(
1044+
t,
1045+
nil,
1046+
1,
1047+
``,
1048+
"breaking",
1049+
filepath.Join("testdata", "workspace", "fail", "modulebreakingconfig"),
1050+
"--against",
1051+
filepath.Join("testdata", "workspace", "fail", "breaking"),
1052+
)
1053+
testRunStdout(
1054+
t,
1055+
nil,
1056+
1,
1057+
``,
1058+
"breaking",
1059+
filepath.Join("testdata", "workspace", "fail", "v2", "modulebreakingconfig"),
1060+
"--against",
1061+
filepath.Join("testdata", "workspace", "fail", "breaking"),
1062+
)
10171063
}
10181064

10191065
func TestWorkspaceDuplicateDirPathSuccess(t *testing.T) {
@@ -1269,7 +1315,6 @@ func TestWorkspaceJumpContextFail(t *testing.T) {
12691315
nil,
12701316
1,
12711317
``,
1272-
// TODO FUTURE: figure out why even on windows, the cleaned, unnormalised path is "/"-separated from decode error
12731318
`Failure: decode testdata/workspace/fail/jumpcontext/buf.work.yaml: directory "../breaking/other/proto" is invalid: ../breaking/other/proto: is outside the context directory`,
12741319
"build",
12751320
filepath.Join("testdata", "workspace", "fail", "jumpcontext"),
@@ -1279,7 +1324,6 @@ func TestWorkspaceJumpContextFail(t *testing.T) {
12791324
nil,
12801325
1,
12811326
``,
1282-
// TODO FUTURE: figure out why even on windows, the cleaned, unnormalised path is "/"-separated from decode error
12831327
`Failure: decode testdata/workspace/fail/v2/jumpcontext/buf.yaml: invalid module path: ../breaking/other/proto: is outside the context directory`,
12841328
"build",
12851329
filepath.Join("testdata", "workspace", "fail", "v2", "jumpcontext"),
@@ -1294,7 +1338,6 @@ func TestWorkspaceDirOverlapFail(t *testing.T) {
12941338
nil,
12951339
1,
12961340
``,
1297-
// TODO FUTURE: figure out why even on windows, the cleaned, unnormalised path is "/"-separated from decode error
12981341
`Failure: decode testdata/workspace/fail/diroverlap/buf.work.yaml: directory "foo" contains directory "foo/bar"`,
12991342
"build",
13001343
filepath.Join("testdata", "workspace", "fail", "diroverlap"),
@@ -1350,7 +1393,6 @@ func TestWorkspaceNoVersionFail(t *testing.T) {
13501393
nil,
13511394
1,
13521395
``,
1353-
// TODO FUTURE: figure out why even on windows, the cleaned, unnormalised path is "/"-separated from decode error
13541396
`Failure: decode testdata/workspace/fail/noversion/buf.work.yaml: "version" is not set. Please add "version: v1"`,
13551397
"build",
13561398
filepath.Join("testdata", "workspace", "fail", "noversion"),
@@ -1365,7 +1407,6 @@ func TestWorkspaceInvalidVersionFail(t *testing.T) {
13651407
nil,
13661408
1,
13671409
``,
1368-
// TODO FUTURE: figure out why even on windows, the cleaned, unnormalised path is "/"-separated from decode error
13691410
`Failure: decode testdata/workspace/fail/invalidversion/buf.work.yaml: unknown file version: "v9"`,
13701411
"build",
13711412
filepath.Join("testdata", "workspace", "fail", "invalidversion"),
@@ -1380,7 +1421,6 @@ func TestWorkspaceNoDirectoriesFail(t *testing.T) {
13801421
nil,
13811422
1,
13821423
``,
1383-
// TODO FUTURE: figure out why even on windows, the cleaned, unnormalised path is "/"-separated from decode error
13841424
`Failure: decode testdata/workspace/fail/nodirectories/buf.work.yaml: directories is empty`,
13851425
"build",
13861426
filepath.Join("testdata", "workspace", "fail", "nodirectories"),

0 commit comments

Comments
 (0)