From 2c6225ad828f917dc9ee23d3267ae75d04b7740b Mon Sep 17 00:00:00 2001 From: antoinetoussaint-byte <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 25 Oct 2026 18:43:39 -0400 Subject: [PATCH 1/4] fix(generate): client sources follow buf.yaml modules and excludes `codefly generate client --from //` walked the service's proto directory flat. On a v2 buf.yaml with several modules that named a vendored upstream file `vendor/codefly/base/v0/x.proto` instead of `codefly/base/v0/x.proto`, so vendorsUpstreamModules never matched it against the descriptor set and generation took the Sources path with imports that resolve to nothing; and it compiled a module's `excludes` beside the canonical tree, declaring every message twice. Both stopped the accounts service of module-saas-starter from generating any client. Each source is now named relative to the buf module that holds it, and excluded directories are skipped, so Sources are exactly the files and names buildDescriptorSet compiled. Co-Authored-By: Claude Opus 5.5 --- cmd/generate/client.go | 112 ++++++++++++++++++---- cmd/generate/client_proto_sources_test.go | 64 +++++++++++++ 2 files changed, 157 insertions(+), 19 deletions(-) create mode 100644 cmd/generate/client_proto_sources_test.go diff --git a/cmd/generate/client.go b/cmd/generate/client.go index b05b51d2..11ab02b8 100644 --- a/cmd/generate/client.go +++ b/cmd/generate/client.go @@ -5,6 +5,7 @@ import ( "crypto/sha256" "fmt" "os" + "path" "path/filepath" "strings" "time" @@ -26,6 +27,7 @@ import ( "google.golang.org/protobuf/types/descriptorpb" "github.com/spf13/cobra" + "gopkg.in/yaml.v3" ) var ( @@ -431,29 +433,55 @@ func buildLocalContractEndpoint(ctx context.Context, service *resources.Service, return nil, nil, nil, fmt.Errorf("endpoint %s reports neither a grpc nor a rest contract", endpoint.Name) } -// collectProtoSources reads every .proto file directly (and doubly) under -// protoDir into proto.GenerateClient's Sources form, path relative to -// protoDir — the same files buildDescriptorSet compiled, so generation from -// Sources produces the same code the persisted descriptor set describes. +// collectProtoSources reads the .proto files buf compiles from protoDir into +// proto.GenerateClient's Sources form — the same files buildDescriptorSet +// compiled, under the same names, so generation from Sources produces the code +// the persisted descriptor set describes. +// +// The names and the set both come from buf.yaml. A v2 buf.yaml may declare +// several modules: each file is named relative to the module that holds it (a +// file under a `vendor` module is `codefly/base/v0/x.proto`, as every import of +// it spells it and as the descriptor set names it), and a module's `excludes` +// are not compiled at all. Walking protoDir flat instead named a vendored +// upstream file `vendor/...`, so vendorsUpstreamModules never recognised it and +// generation took the Sources path with imports that resolved to nothing, and +// it compiled an excluded tree beside the canonical one, declaring every +// message twice. func collectProtoSources(protoDir string) ([]coreproto.Source, error) { + modules, err := bufModules(protoDir) + if err != nil { + return nil, err + } var sources []coreproto.Source - err := filepath.WalkDir(protoDir, func(p string, d os.DirEntry, err error) error { - if err != nil || d.IsDir() || !strings.HasSuffix(p, ".proto") { - return err - } - rel, err := filepath.Rel(protoDir, p) - if err != nil { - return err - } - content, err := os.ReadFile(p) //nolint:gosec // p is walked from a service's own committed proto directory + for _, module := range modules { + root := filepath.Join(protoDir, filepath.FromSlash(module.Path)) + err := filepath.WalkDir(root, func(p string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + if p != root && module.excludes(protoDir, p) { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(p, ".proto") { + return nil + } + rel, err := filepath.Rel(root, p) + if err != nil { + return err + } + content, err := os.ReadFile(p) //nolint:gosec // p is walked from a service's own committed proto directory + if err != nil { + return err + } + sources = append(sources, coreproto.Source{Path: filepath.ToSlash(rel), Content: content}) + return nil + }) if err != nil { - return err + return nil, err } - sources = append(sources, coreproto.Source{Path: filepath.ToSlash(rel), Content: content}) - return nil - }) - if err != nil { - return nil, err } if len(sources) == 0 { return nil, fmt.Errorf("no .proto files under %s", protoDir) @@ -461,6 +489,52 @@ func collectProtoSources(protoDir string) ([]coreproto.Source, error) { return sources, nil } +// bufModule is one module of a buf.yaml: its directory relative to the +// buf.yaml, and the directories (relative to the buf.yaml too) it excludes. +type bufModule struct { + Path string `yaml:"path"` + Excludes []string `yaml:"excludes"` +} + +func (m bufModule) excludes(protoDir, dir string) bool { + rel, err := filepath.Rel(protoDir, dir) + if err != nil { + return false + } + rel = filepath.ToSlash(rel) + for _, excluded := range m.Excludes { + if rel == path.Clean(excluded) { + return true + } + } + return false +} + +// bufModules is the modules protoDir/buf.yaml declares: a v2 file's `modules`, +// or the one module at its root when it declares none (v1, or v2 without the +// key). +func bufModules(protoDir string) ([]bufModule, error) { + raw, err := os.ReadFile(filepath.Join(protoDir, "buf.yaml")) //nolint:gosec // the service's own buf.yaml + if err != nil { + return nil, fmt.Errorf("cannot read %s: %w", filepath.Join(protoDir, "buf.yaml"), err) + } + var config struct { + Modules []bufModule `yaml:"modules"` + } + if err := yaml.Unmarshal(raw, &config); err != nil { + return nil, fmt.Errorf("cannot parse %s: %w", filepath.Join(protoDir, "buf.yaml"), err) + } + if len(config.Modules) == 0 { + return []bufModule{{Path: "."}}, nil + } + for i, module := range config.Modules { + if module.Path == "" { + config.Modules[i].Path = "." + } + } + return config.Modules, nil +} + // resolvePackageSource handles --from package:@: it finds the // composed module whose package manifest ID matches id, requires it to be // pinned at exactly version, and reads its committed API contract catalog. diff --git a/cmd/generate/client_proto_sources_test.go b/cmd/generate/client_proto_sources_test.go new file mode 100644 index 00000000..59f47877 --- /dev/null +++ b/cmd/generate/client_proto_sources_test.go @@ -0,0 +1,64 @@ +package generate + +import ( + "os" + "path/filepath" + "sort" + "testing" +) + +// A v2 buf.yaml with a vendor module and an excluded tree: every source is +// named relative to the module that holds it, as its importers spell it, and an +// excluded tree is not a source. +func TestCollectProtoSourcesFollowsBufModules(t *testing.T) { + dir := t.TempDir() + write := func(rel, content string) { + t.Helper() + p := filepath.Join(dir, filepath.FromSlash(rel)) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } + write("buf.yaml", "version: v2\nmodules:\n - path: .\n excludes:\n - facade\n - vendor\n - path: vendor\n") + write("acme/v1/api.proto", "syntax = \"proto3\";\n") + write("facade/api.proto", "syntax = \"proto3\";\n") + write("vendor/upstream/base/v0/base.proto", "syntax = \"proto3\";\n") + + sources, err := collectProtoSources(dir) + if err != nil { + t.Fatal(err) + } + var names []string + for _, source := range sources { + names = append(names, source.Path) + } + sort.Strings(names) + want := []string{"acme/v1/api.proto", "upstream/base/v0/base.proto"} + if len(names) != len(want) || names[0] != want[0] || names[1] != want[1] { + t.Fatalf("sources = %v, want %v", names, want) + } +} + +// A buf.yaml declaring no modules is one module at its root. +func TestCollectProtoSourcesSingleModule(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "buf.yaml"), []byte("version: v1\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(dir, "acme"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "acme", "api.proto"), []byte("syntax = \"proto3\";\n"), 0o600); err != nil { + t.Fatal(err) + } + sources, err := collectProtoSources(dir) + if err != nil { + t.Fatal(err) + } + if len(sources) != 1 || sources[0].Path != "acme/api.proto" { + t.Fatalf("sources = %+v", sources) + } +} From fb7bd32c76cbfd8fecfa90dce9f879532b38cbef Mon Sep 17 00:00:00 2001 From: antoinetoussaint-byte <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 25 Oct 2026 20:11:19 -0400 Subject: [PATCH 2/4] fix(generate): buf names the client's source set; the buf.yaml parser goes Review r52: collectProtoSources re-implemented buf's module and exclude rules by parsing buf.yaml, while buf already runs in the proto companion, and it had drifted from buf (v1 build.excludes and v2 includes ignored; a symlinked module folder contributed nothing). The descriptor set and the source set now come from one companion run: `buf build` as before, then `buf ls-files --format json`, each listed file read from the path buf found it at and named by its import path. The hand parser is deleted. copyTree copies a symlinked directory as its target, since the target is not mounted into the companion. Tests: the listing is read by import path and refuses a path outside the tree (unit); through the pinned companion, v2 modules with excludes, v1 build.excludes, v2 includes and a symlinked module folder each yield exactly buf's source set (the symlink case fails without the copyTree change). Re-verified byte-identical output: module-saas-starter's libraries/source-read-sdk at 8aa26f46 and module-annotations' clients differ only in the generated-by stamp. Co-Authored-By: Claude Opus 5.5 --- cmd/generate/client.go | 114 +----------------- .../client_proto_sources_companion_test.go | 71 +++++++++++ cmd/generate/client_proto_sources_test.go | 70 ++++------- cmd/generate/contracts.go | 109 +++++++++++++++-- 4 files changed, 198 insertions(+), 166 deletions(-) create mode 100644 cmd/generate/client_proto_sources_companion_test.go diff --git a/cmd/generate/client.go b/cmd/generate/client.go index 11ab02b8..a4fa1fb7 100644 --- a/cmd/generate/client.go +++ b/cmd/generate/client.go @@ -4,8 +4,6 @@ import ( "context" "crypto/sha256" "fmt" - "os" - "path" "path/filepath" "strings" "time" @@ -27,7 +25,6 @@ import ( "google.golang.org/protobuf/types/descriptorpb" "github.com/spf13/cobra" - "gopkg.in/yaml.v3" ) var ( @@ -378,7 +375,10 @@ func buildLocalContractEndpoint(ctx context.Context, service *resources.Service, if ok, _ := shared.FileExists(ctx, filepath.Join(protoDir, "buf.yaml")); !ok { return nil, nil, nil, fmt.Errorf("service %s has no proto/buf.yaml; cannot build a descriptor set for endpoint %s", service.Name, endpoint.Name) } - descriptorSet, err := buildDescriptorSet(ctx, protoDir) + // buf compiles the descriptor set and names its own source set in the + // same companion run, so Sources are exactly the files and import names + // the descriptor set was built from. + descriptorSet, protoSources, err := compileProtoDir(ctx, protoDir, true) if err != nil { return nil, nil, nil, fmt.Errorf("cannot build descriptor set: %w", err) } @@ -386,10 +386,6 @@ func buildLocalContractEndpoint(ctx context.Context, service *resources.Service, if err = googleproto.Unmarshal(descriptorSet, &set); err != nil { return nil, nil, nil, fmt.Errorf("cannot parse generated descriptor set: %w", err) } - protoSources, err := collectProtoSources(protoDir) - if err != nil { - return nil, nil, nil, fmt.Errorf("cannot read proto sources: %w", err) - } // The contract's package is the one the service's own protos declare // their services in, as `generate contracts` settles it; an endpoint // declaring none in service.codefly.yaml is the common case. @@ -433,108 +429,6 @@ func buildLocalContractEndpoint(ctx context.Context, service *resources.Service, return nil, nil, nil, fmt.Errorf("endpoint %s reports neither a grpc nor a rest contract", endpoint.Name) } -// collectProtoSources reads the .proto files buf compiles from protoDir into -// proto.GenerateClient's Sources form — the same files buildDescriptorSet -// compiled, under the same names, so generation from Sources produces the code -// the persisted descriptor set describes. -// -// The names and the set both come from buf.yaml. A v2 buf.yaml may declare -// several modules: each file is named relative to the module that holds it (a -// file under a `vendor` module is `codefly/base/v0/x.proto`, as every import of -// it spells it and as the descriptor set names it), and a module's `excludes` -// are not compiled at all. Walking protoDir flat instead named a vendored -// upstream file `vendor/...`, so vendorsUpstreamModules never recognised it and -// generation took the Sources path with imports that resolved to nothing, and -// it compiled an excluded tree beside the canonical one, declaring every -// message twice. -func collectProtoSources(protoDir string) ([]coreproto.Source, error) { - modules, err := bufModules(protoDir) - if err != nil { - return nil, err - } - var sources []coreproto.Source - for _, module := range modules { - root := filepath.Join(protoDir, filepath.FromSlash(module.Path)) - err := filepath.WalkDir(root, func(p string, d os.DirEntry, err error) error { - if err != nil { - return err - } - if d.IsDir() { - if p != root && module.excludes(protoDir, p) { - return filepath.SkipDir - } - return nil - } - if !strings.HasSuffix(p, ".proto") { - return nil - } - rel, err := filepath.Rel(root, p) - if err != nil { - return err - } - content, err := os.ReadFile(p) //nolint:gosec // p is walked from a service's own committed proto directory - if err != nil { - return err - } - sources = append(sources, coreproto.Source{Path: filepath.ToSlash(rel), Content: content}) - return nil - }) - if err != nil { - return nil, err - } - } - if len(sources) == 0 { - return nil, fmt.Errorf("no .proto files under %s", protoDir) - } - return sources, nil -} - -// bufModule is one module of a buf.yaml: its directory relative to the -// buf.yaml, and the directories (relative to the buf.yaml too) it excludes. -type bufModule struct { - Path string `yaml:"path"` - Excludes []string `yaml:"excludes"` -} - -func (m bufModule) excludes(protoDir, dir string) bool { - rel, err := filepath.Rel(protoDir, dir) - if err != nil { - return false - } - rel = filepath.ToSlash(rel) - for _, excluded := range m.Excludes { - if rel == path.Clean(excluded) { - return true - } - } - return false -} - -// bufModules is the modules protoDir/buf.yaml declares: a v2 file's `modules`, -// or the one module at its root when it declares none (v1, or v2 without the -// key). -func bufModules(protoDir string) ([]bufModule, error) { - raw, err := os.ReadFile(filepath.Join(protoDir, "buf.yaml")) //nolint:gosec // the service's own buf.yaml - if err != nil { - return nil, fmt.Errorf("cannot read %s: %w", filepath.Join(protoDir, "buf.yaml"), err) - } - var config struct { - Modules []bufModule `yaml:"modules"` - } - if err := yaml.Unmarshal(raw, &config); err != nil { - return nil, fmt.Errorf("cannot parse %s: %w", filepath.Join(protoDir, "buf.yaml"), err) - } - if len(config.Modules) == 0 { - return []bufModule{{Path: "."}}, nil - } - for i, module := range config.Modules { - if module.Path == "" { - config.Modules[i].Path = "." - } - } - return config.Modules, nil -} - // resolvePackageSource handles --from package:@: it finds the // composed module whose package manifest ID matches id, requires it to be // pinned at exactly version, and reads its committed API contract catalog. diff --git a/cmd/generate/client_proto_sources_companion_test.go b/cmd/generate/client_proto_sources_companion_test.go new file mode 100644 index 00000000..b79a7b21 --- /dev/null +++ b/cmd/generate/client_proto_sources_companion_test.go @@ -0,0 +1,71 @@ +//go:build proto_companion_required + +package generate + +import ( + "os" + "path/filepath" + "sort" + "testing" + + "github.com/stretchr/testify/require" +) + +// sourceNames is the source set buf names for a proto tree, through the +// pinned companion. +func sourceNames(t *testing.T, dir string) []string { + t.Helper() + _, sources, err := compileProtoDir(t.Context(), dir, true) + require.NoError(t, err) + var names []string + for _, source := range sources { + names = append(names, source.Path) + } + sort.Strings(names) + return names +} + +func writeProtos(t *testing.T, root string, rels ...string) { + t.Helper() + for _, rel := range rels { + writeTestFile(t, filepath.Join(root, rel), "syntax = \"proto3\";\n") + } +} + +// A v2 buf.yaml with a vendor module and an excluded tree: every source is +// named relative to the module that holds it, and an excluded tree is not a +// source. +func TestBufNamesV2ModulesAndExcludesThroughPinnedCompanion(t *testing.T) { + dir := t.TempDir() + writeTestFile(t, filepath.Join(dir, "buf.yaml"), "version: v2\nmodules:\n - path: .\n excludes: [facade, vendor]\n - path: vendor\n") + writeProtos(t, dir, "acme/v1/api.proto", "facade/api.proto", "vendor/upstream/base/v0/base.proto") + require.Equal(t, []string{"acme/v1/api.proto", "upstream/base/v0/base.proto"}, sourceNames(t, dir)) +} + +// A v1 buf.yaml's build.excludes keep a tree out of the source set. +func TestBufNamesV1BuildExcludesThroughPinnedCompanion(t *testing.T) { + dir := t.TempDir() + writeTestFile(t, filepath.Join(dir, "buf.yaml"), "version: v1\nbuild:\n excludes:\n - facade\n") + writeProtos(t, dir, "acme/v1/api.proto", "facade/api.proto") + require.Equal(t, []string{"acme/v1/api.proto"}, sourceNames(t, dir)) +} + +// A v2 module's includes narrow it to the trees named. +func TestBufNamesV2IncludesThroughPinnedCompanion(t *testing.T) { + dir := t.TempDir() + writeTestFile(t, filepath.Join(dir, "buf.yaml"), "version: v2\nmodules:\n - path: .\n includes: [acme]\n") + writeProtos(t, dir, "acme/v1/api.proto", "other/v1/other.proto") + require.Equal(t, []string{"acme/v1/api.proto"}, sourceNames(t, dir)) +} + +// A module folder that is a symlink contributes its files under their import +// names: the link's target is copied in, since it is not mounted. +func TestBufNamesASymlinkedModuleThroughPinnedCompanion(t *testing.T) { + root := t.TempDir() + dir, shared := filepath.Join(root, "proto"), filepath.Join(root, "shared") + writeProtos(t, shared, "upstream/base/v0/base.proto") + writeTestFile(t, filepath.Join(dir, "buf.yaml"), "version: v2\nmodules:\n - path: .\n excludes: [vendor]\n - path: vendor\n") + writeProtos(t, dir, "acme/v1/api.proto") + require.NoError(t, os.Symlink(shared, filepath.Join(dir, "vendor"))) + require.Equal(t, []string{"acme/v1/api.proto", "upstream/base/v0/base.proto"}, sourceNames(t, dir)) +} diff --git a/cmd/generate/client_proto_sources_test.go b/cmd/generate/client_proto_sources_test.go index 59f47877..2018cc8e 100644 --- a/cmd/generate/client_proto_sources_test.go +++ b/cmd/generate/client_proto_sources_test.go @@ -3,62 +3,44 @@ package generate import ( "os" "path/filepath" - "sort" "testing" ) -// A v2 buf.yaml with a vendor module and an excluded tree: every source is -// named relative to the module that holds it, as its importers spell it, and an -// excluded tree is not a source. -func TestCollectProtoSourcesFollowsBufModules(t *testing.T) { - dir := t.TempDir() - write := func(rel, content string) { - t.Helper() - p := filepath.Join(dir, filepath.FromSlash(rel)) - if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(p, []byte(content), 0o600); err != nil { - t.Fatal(err) - } - } - write("buf.yaml", "version: v2\nmodules:\n - path: .\n excludes:\n - facade\n - vendor\n - path: vendor\n") - write("acme/v1/api.proto", "syntax = \"proto3\";\n") - write("facade/api.proto", "syntax = \"proto3\";\n") - write("vendor/upstream/base/v0/base.proto", "syntax = \"proto3\";\n") - - sources, err := collectProtoSources(dir) +// buf names the source set: each file is read from the path buf found it at +// and named by its import path, and a file buf lists only as an import is not +// a source. +func TestProtoSourcesFromListingNamesByImportPath(t *testing.T) { + root := t.TempDir() + for _, rel := range []string{"acme/v1/api.proto", "vendor/upstream/base/v0/base.proto"} { + writeTestFile(t, filepath.Join(root, rel), "syntax = \"proto3\";\n// "+rel+"\n") + } + listing := []byte(`{"path":"acme/v1/api.proto","import_path":"acme/v1/api.proto","is_import":false} +{"path":"vendor/upstream/base/v0/base.proto","import_path":"upstream/base/v0/base.proto","is_import":false} +{"path":"buf/validate/validate.proto","import_path":"buf/validate/validate.proto","is_import":true} +`) + sources, err := protoSourcesFromListing(root, listing) if err != nil { t.Fatal(err) } - var names []string - for _, source := range sources { - names = append(names, source.Path) + if len(sources) != 2 || sources[0].Path != "acme/v1/api.proto" || sources[1].Path != "upstream/base/v0/base.proto" { + t.Fatalf("sources = %+v", sources) } - sort.Strings(names) - want := []string{"acme/v1/api.proto", "upstream/base/v0/base.proto"} - if len(names) != len(want) || names[0] != want[0] || names[1] != want[1] { - t.Fatalf("sources = %v, want %v", names, want) + if string(sources[1].Content) != "syntax = \"proto3\";\n// vendor/upstream/base/v0/base.proto\n" { + t.Fatalf("content read from the wrong file: %q", sources[1].Content) } } -// A buf.yaml declaring no modules is one module at its root. -func TestCollectProtoSourcesSingleModule(t *testing.T) { - dir := t.TempDir() - if err := os.WriteFile(filepath.Join(dir, "buf.yaml"), []byte("version: v1\n"), 0o600); err != nil { - t.Fatal(err) - } - if err := os.MkdirAll(filepath.Join(dir, "acme"), 0o755); err != nil { - t.Fatal(err) +// A listed path that leaves the copied tree, or a listing with nothing to +// compile, is refused rather than read. +func TestProtoSourcesFromListingRefusesWhatItCannotRead(t *testing.T) { + root := t.TempDir() + if _, err := protoSourcesFromListing(root, []byte(`{"path":"../escape.proto","import_path":"escape.proto"}`)); err == nil { + t.Fatal("a path outside the tree was read") } - if err := os.WriteFile(filepath.Join(dir, "acme", "api.proto"), []byte("syntax = \"proto3\";\n"), 0o600); err != nil { - t.Fatal(err) + if _, err := protoSourcesFromListing(root, nil); err == nil { + t.Fatal("an empty listing was accepted") } - sources, err := collectProtoSources(dir) - if err != nil { + if _, err := os.Stat(filepath.Join(root, "escape.proto")); !os.IsNotExist(err) { t.Fatal(err) } - if len(sources) != 1 || sources[0].Path != "acme/api.proto" { - t.Fatalf("sources = %+v", sources) - } } diff --git a/cmd/generate/contracts.go b/cmd/generate/contracts.go index 5eb8c5cc..911f4669 100644 --- a/cmd/generate/contracts.go +++ b/cmd/generate/contracts.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "os" "path/filepath" "sort" @@ -656,22 +657,32 @@ func printContractLine(endpoint *composition.APIContractEndpoint) { // included. It works from a temporary copy of protoDir so buf's dependency // resolution never mutates the service's committed buf.lock. func buildDescriptorSet(ctx context.Context, protoDir string) ([]byte, error) { + descriptor, _, err := compileProtoDir(ctx, protoDir, false) + return descriptor, err +} + +// compileProtoDir has buf, in the pinned proto companion, build protoDir's +// descriptor set and, when listSources is set, name the source set it built +// it from (`buf ls-files --format json`): every file buf compiles, read from +// where buf found it and named by its import path. buf.yaml's modules, +// includes, excludes and symlinks are buf's to interpret, never this CLI's. +func compileProtoDir(ctx context.Context, protoDir string, listSources bool) ([]byte, []proto.Source, error) { if !runners.DockerEngineRunning(ctx) { - return nil, fmt.Errorf("docker is not running; please start Docker first") + return nil, nil, fmt.Errorf("docker is not running; please start Docker first") } image, err := proto.CompanionImage(ctx) if err != nil { - return nil, fmt.Errorf("cannot get companion image: %w", err) + return nil, nil, fmt.Errorf("cannot get companion image: %w", err) } tmpDir, err := os.MkdirTemp("", "codefly-contract-proto") if err != nil { - return nil, err + return nil, nil, err } defer func() { _ = os.RemoveAll(tmpDir) }() if err = copyTree(protoDir, tmpDir); err != nil { - return nil, fmt.Errorf("cannot copy proto sources: %w", err) + return nil, nil, fmt.Errorf("cannot copy proto sources: %w", err) } projectContainerRecovery(ctx) @@ -679,7 +690,7 @@ func buildDescriptorSet(ctx context.Context, protoDir string) ([]byte, error) { name := fmt.Sprintf("contracts-%d", time.Now().UnixMilli()) runner, err := runners.NewDockerEnvironment(ctx, image, tmpDir, name) if err != nil { - return nil, fmt.Errorf("cannot create docker runner: %w", err) + return nil, nil, fmt.Errorf("cannot create docker runner: %w", err) } // A descriptor-set container holds no state worth preserving across runs: it // is created, driven once and shut down in the defer below. Marking it @@ -705,31 +716,92 @@ func buildDescriptorSet(ctx context.Context, protoDir string) ([]byte, error) { }() if err = runner.Init(ctx); err != nil { - return nil, fmt.Errorf("cannot init runner: %w", err) + return nil, nil, fmt.Errorf("cannot init runner: %w", err) } depProc, err := runner.NewProcess("buf", "dep", "update") if err != nil { - return nil, fmt.Errorf("cannot create process: %w", err) + return nil, nil, fmt.Errorf("cannot create process: %w", err) } if err = depProc.Run(ctx); err != nil { - return nil, fmt.Errorf("cannot update buf dependencies: %w", err) + return nil, nil, fmt.Errorf("cannot update buf dependencies: %w", err) } const descriptorFile = ".codefly-contract.binpb" buildProc, err := runner.NewProcess("buf", "build", "--as-file-descriptor-set", "-o", "/workspace/"+descriptorFile) if err != nil { - return nil, fmt.Errorf("cannot create process: %w", err) + return nil, nil, fmt.Errorf("cannot create process: %w", err) } if err = buildProc.Run(ctx); err != nil { - return nil, fmt.Errorf("cannot build descriptor set: %w", err) + return nil, nil, fmt.Errorf("cannot build descriptor set: %w", err) + } + + descriptor, err := os.ReadFile(filepath.Join(tmpDir, descriptorFile)) + if err != nil || !listSources { + return descriptor, nil, err + } + lsProc, err := runner.NewProcess("buf", "ls-files", "--format", "json") + if err != nil { + return nil, nil, fmt.Errorf("cannot create process: %w", err) + } + stdout, err := lsProc.StdoutPipe() + if err != nil { + return nil, nil, fmt.Errorf("cannot read buf ls-files: %w", err) + } + if err = lsProc.Start(ctx); err != nil { + return nil, nil, fmt.Errorf("cannot list proto sources: %w", err) + } + listing, readErr := io.ReadAll(stdout) + if err = lsProc.Wait(ctx); err != nil { + return nil, nil, fmt.Errorf("cannot list proto sources: %w", err) + } + if readErr != nil { + return nil, nil, fmt.Errorf("cannot read buf ls-files: %w", readErr) } + sources, err := protoSourcesFromListing(tmpDir, listing) + return descriptor, sources, err +} + +// bufListedFile is one line of `buf ls-files --format json`. +type bufListedFile struct { + Path string `json:"path"` + ImportPath string `json:"import_path"` + IsImport bool `json:"is_import"` +} - return os.ReadFile(filepath.Join(tmpDir, descriptorFile)) +// protoSourcesFromListing reads every file buf listed, from its path under +// root, as a Source named by its import path — the name every importer spells +// and the descriptor set records. Files buf lists only as imports are not +// sources. +func protoSourcesFromListing(root string, listing []byte) ([]proto.Source, error) { + var sources []proto.Source + decoder := json.NewDecoder(bytes.NewReader(listing)) + for decoder.More() { + var file bufListedFile + if err := decoder.Decode(&file); err != nil { + return nil, fmt.Errorf("cannot parse buf ls-files: %w", err) + } + if file.IsImport { + continue + } + if file.Path == "" || file.ImportPath == "" || !filepath.IsLocal(filepath.FromSlash(file.Path)) { + return nil, fmt.Errorf("buf ls-files listed an unusable file %q (%q)", file.Path, file.ImportPath) + } + content, err := os.ReadFile(filepath.Join(root, filepath.FromSlash(file.Path))) //nolint:gosec // a local path buf listed under root + if err != nil { + return nil, err + } + sources = append(sources, proto.Source{Path: file.ImportPath, Content: content}) + } + if len(sources) == 0 { + return nil, fmt.Errorf("buf lists no .proto files to compile") + } + return sources, nil } // copyTree copies the file tree rooted at src into dst, creating dst as -// needed. +// needed. A symlink is copied as what it points at, so a symlinked module +// folder reaches buf in the container, where the link's target is not mounted. func copyTree(src, dst string) error { return filepath.WalkDir(src, func(p string, entry os.DirEntry, walkErr error) error { if walkErr != nil { @@ -740,6 +812,19 @@ func copyTree(src, dst string) error { return err } target := filepath.Join(dst, rel) + if entry.Type()&os.ModeSymlink != 0 { + info, statErr := os.Stat(p) + if statErr != nil { + return statErr + } + if info.IsDir() { + resolved, linkErr := filepath.EvalSymlinks(p) + if linkErr != nil { + return linkErr + } + return copyTree(resolved, target) + } + } if entry.IsDir() { return os.MkdirAll(target, 0o755) } From 5dba2e5fb5245ccbd30a619bda1f7898014b4f15 Mon Sep 17 00:00:00 2001 From: antoinetoussaint-byte <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 25 Oct 2026 20:11:19 -0400 Subject: [PATCH 3/4] fix(generate proto): a caller's --path selects --proto as the one input Since #1128 preserved a template's declared inputs, a caller --path was added beside them. buf applies --path to every input, so a template with two inputs (module-document-store: its own protos and ../proto-deps) failed with "no .proto files were targeted" for the input that does not hold the path, and the store's runnables agent fell back to v0.1.212. A caller's --path names files of --proto, so it now selects --proto as the positional input, which replaces the template's inputs as buf defines; with no --path the template's inputs are used as declared. Tests: the invocation for a two-input template with a caller path (unit), and the store's layout through the pinned companion (fails on main with the store's error, passes here). Reproduced and fixed on module-document-store authority/behind-the-hop: tools/generate-service-proto.sh fails on main (5536d73) and leaves the committed output byte-identical with this build. Co-Authored-By: Claude Opus 5.5 --- cmd/generate/proto.go | 12 +++++--- cmd/generate/proto_companion_test.go | 41 ++++++++++++++++++++++++++++ cmd/generate/proto_test.go | 5 +++- 3 files changed, 53 insertions(+), 5 deletions(-) diff --git a/cmd/generate/proto.go b/cmd/generate/proto.go index 1703c139..21390f10 100644 --- a/cmd/generate/proto.go +++ b/cmd/generate/proto.go @@ -945,9 +945,13 @@ func protoGenerationPathArgs(protoDir string, absolute bool) []string { return args } -// A positional Buf input replaces the template's inputs, including their path -// filters. Supply the default only when the template leaves input selection to -// the caller. Relative template inputs resolve in the template's working dir. +// The template's inputs are used as declared unless the caller selects files. +// A caller's --path names files of --proto, so it selects --proto as the one +// input: buf applies a --path flag to every input, and an input that does not +// hold the path targets no file and fails the whole run ("no .proto files were +// targeted"), so the template's inputs cannot be kept beside it. A positional +// input replaces them, as buf itself defines. Relative template inputs resolve +// in the template's working dir. func protoGenerationArgs(protoDir, templatePath string, template []byte) ([]string, error) { var selection struct { Inputs []yaml.Node `yaml:"inputs"` @@ -956,7 +960,7 @@ func protoGenerationArgs(protoDir, templatePath string, template []byte) ([]stri return nil, fmt.Errorf("cannot read generation template inputs: %w", err) } args := []string{"generate"} - if len(selection.Inputs) == 0 { + if len(selection.Inputs) == 0 || len(protoPaths) > 0 { args = append(args, protoDir) } args = append(args, "--template", templatePath) diff --git a/cmd/generate/proto_companion_test.go b/cmd/generate/proto_companion_test.go index fc7520a1..231b90a1 100644 --- a/cmd/generate/proto_companion_test.go +++ b/cmd/generate/proto_companion_test.go @@ -135,3 +135,44 @@ plugins: require.ErrorIs(t, err, os.ErrNotExist) } } + +// The document store's layout: a template declaring two inputs (its own protos +// with a path selection, and a sibling tree of client contracts), generated +// with a caller --path. buf applies --path to every input, so keeping both +// inputs failed with "no .proto files were targeted"; the caller's selection +// makes --proto the one input. +func TestProtoTwoInputTemplateWithCallerPathThroughPinnedCompanion(t *testing.T) { + root := t.TempDir() + writeTestFile(t, filepath.Join(root, "workspace.codefly.yaml"), "name: example\n") + input := filepath.Join(root, "proto") + for _, dir := range []string{"proto", "deps"} { + writeTestFile(t, filepath.Join(root, dir, "buf.yaml"), "version: v2\nmodules:\n - path: .\n") + } + for _, name := range []string{"proto/api/v1/api", "proto/api/v1/other", "deps/client/v1/client"} { + writeTestFile(t, filepath.Join(root, name+".proto"), `syntax = "proto3"; +package example.v1; +option go_package = "example.com/generated;example"; +message `+filepath.Base(name)+` { string value = 1; } +`) + } + writeTestFile(t, filepath.Join(input, "buf.gen.yaml"), `version: v2 +inputs: + - directory: . + paths: [api/v1/api.proto] + - directory: ../deps +plugins: + - local: protoc-gen-go + out: ../code/gen + opt: paths=source_relative +`) + oldTemplate, oldLocal, oldPaths := protoTemplate, protoLocal, protoPaths + t.Cleanup(func() { protoTemplate, protoLocal, protoPaths = oldTemplate, oldLocal, oldPaths }) + protoTemplate, protoLocal, protoPaths = "", false, []string{"api/v1/api.proto"} + require.NoError(t, generateProtoCode(t.Context(), input, input)) + _, err := os.Stat(filepath.Join(root, "code/gen/api/v1/api.pb.go")) + require.NoError(t, err, "the caller's file is generated") + for _, name := range []string{"api/v1/other.pb.go", "client/v1/client.pb.go"} { + _, err := os.Stat(filepath.Join(root, "code/gen", name)) + require.ErrorIs(t, err, os.ErrNotExist, "only the caller's selection is generated: %s", name) + } +} diff --git a/cmd/generate/proto_test.go b/cmd/generate/proto_test.go index 7e20d3c3..ffaaba42 100644 --- a/cmd/generate/proto_test.go +++ b/cmd/generate/proto_test.go @@ -44,7 +44,10 @@ func TestProtoGenerationArgsPreserveBufInputSelection(t *testing.T) { {"v2 default", "version: v2\n", nil, []string{"generate", "/workspace/proto", "--template", "/staging/template.yaml"}}, {"empty inputs", "version: v2\ninputs: []\n", nil, []string{"generate", "/workspace/proto", "--template", "/staging/template.yaml"}}, {"declared selection", "version: v2\ninputs:\n - directory: .\n paths: [api.proto]\n - directory: ../other\n", nil, []string{"generate", "--template", "/staging/template.yaml"}}, - {"caller override", "version: v2\ninputs:\n - directory: .\n paths: [api.proto]\n", []string{"chosen.proto"}, []string{"generate", "--template", "/staging/template.yaml", "--path", "/workspace/proto/chosen.proto"}}, + {"caller override", "version: v2\ninputs:\n - directory: .\n paths: [api.proto]\n", []string{"chosen.proto"}, []string{"generate", "/workspace/proto", "--template", "/staging/template.yaml", "--path", "/workspace/proto/chosen.proto"}}, + // buf applies --path to every input: a second input that does not hold + // the caller's file would target nothing and fail the run. + {"caller selection over two inputs", "version: v2\ninputs:\n - directory: .\n paths: [api.proto]\n - directory: ../deps\n", []string{"api.proto"}, []string{"generate", "/workspace/proto", "--template", "/staging/template.yaml", "--path", "/workspace/proto/api.proto"}}, } { t.Run(tc.name, func(t *testing.T) { protoPaths = tc.paths From a055f9f1f9b3ceb8b2b6ffa6d6f865d62674975a Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 25 Oct 2026 20:50:27 -0400 Subject: [PATCH 4/4] fix(publish clients): buf names the published contract's sources; copyTree refuses looping links `publish clients` listed a published contract's proto sources with a flat directory walk over the tree `generate contracts` copies verbatim, buf.yaml included. On a tree with a vendor module and excluded folders it named the vendored files vendor/... (so no upstream module matched and the generator took the Sources path with imports that resolve to nothing) and compiled the excluded facade: the defect this branch fixed for `generate client`. BuildModuleClientEntry now takes the source lister, and `publish clients` passes generate.ListProtoSources: the same buf build + `buf ls-files --format json` companion run `generate client` uses. The flat walk is deleted, so one mechanism names a proto tree's sources. Sources are sorted by import path, as the deleted walk sorted them. copyTree follows a symlinked folder (one outside the tree is copied in, which keeps a shared module folder working) but refuses, by name, a link to a folder that holds it or to one already being copied, instead of recursing until the path is too long. `generate proto --path` replaces the template's inputs with --proto only when the template declares more than one input; a single input keeps its input-level options (paths, types, exclude_paths) beside the caller's --path, as before. Review r70: F1 (major), F2, F3. Co-Authored-By: Claude Opus 5.5 --- cmd/generate/client_proto_sources_test.go | 44 +++++++++++++++++ cmd/generate/contracts.go | 44 +++++++++++++++-- cmd/generate/proto.go | 15 +++--- cmd/generate/proto_test.go | 5 +- cmd/publish/clients.go | 3 +- .../clients_proto_sources_companion_test.go | 49 +++++++++++++++++++ pkg/generators/module_clients.go | 47 ++++-------------- pkg/generators/module_clients_test.go | 17 +++++-- 8 files changed, 170 insertions(+), 54 deletions(-) create mode 100644 cmd/publish/clients_proto_sources_companion_test.go diff --git a/cmd/generate/client_proto_sources_test.go b/cmd/generate/client_proto_sources_test.go index 2018cc8e..e7dbf70d 100644 --- a/cmd/generate/client_proto_sources_test.go +++ b/cmd/generate/client_proto_sources_test.go @@ -3,6 +3,7 @@ package generate import ( "os" "path/filepath" + "strings" "testing" ) @@ -44,3 +45,46 @@ func TestProtoSourcesFromListingRefusesWhatItCannotRead(t *testing.T) { t.Fatal(err) } } + +// A linked folder outside the tree is copied as what it holds; a link to a +// folder that holds it, or to one already being copied, is refused by name +// rather than recursed into. +func TestCopyTreeFollowsLinkedFoldersAndRefusesLoops(t *testing.T) { + root := t.TempDir() + src, shared := filepath.Join(root, "proto"), filepath.Join(root, "shared") + writeTestFile(t, filepath.Join(src, "acme/v1/api.proto"), "a") + writeTestFile(t, filepath.Join(shared, "upstream/base.proto"), "b") + if err := os.Symlink(shared, filepath.Join(src, "vendor")); err != nil { + t.Fatal(err) + } + dst := filepath.Join(t.TempDir(), "copy") + if err := copyTree(src, dst); err != nil { + t.Fatal(err) + } + if data, err := os.ReadFile(filepath.Join(dst, "vendor/upstream/base.proto")); err != nil || string(data) != "b" { + t.Fatalf("linked folder not copied: %q, %v", data, err) + } + + for name, link := range map[string]struct{ at, to string }{ + "to its own folder": {"acme/loop", "acme"}, + "to the copied root": {"acme/v1/up", "."}, + "back through a link": {"../shared/upstream/back", "."}, + "to a folder above root": {"acme/out", ".."}, + } { + t.Run(name, func(t *testing.T) { + at := filepath.Join(src, filepath.FromSlash(link.at)) + if err := os.Symlink(filepath.Join(src, link.to), at); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Remove(at) }) + err := copyTree(src, filepath.Join(t.TempDir(), "copy")) + if err == nil || !strings.Contains(err.Error(), "looping link") { + t.Fatalf("copyTree = %v, want a looping link refused", err) + } + resolved, _ := filepath.EvalSymlinks(at) + if !strings.Contains(err.Error(), resolved) { + t.Fatalf("refusal %q does not name the link's target %s", err, resolved) + } + }) + } +} diff --git a/cmd/generate/contracts.go b/cmd/generate/contracts.go index 911f4669..66924da7 100644 --- a/cmd/generate/contracts.go +++ b/cmd/generate/contracts.go @@ -661,6 +661,14 @@ func buildDescriptorSet(ctx context.Context, protoDir string) ([]byte, error) { return descriptor, err } +// ListProtoSources names protoDir's source set as buf compiles it: the one +// source-set mechanism, shared by `generate client` and `publish clients` +// (over a published contract's proto tree, which carries its buf.yaml). +func ListProtoSources(ctx context.Context, protoDir string) ([]proto.Source, error) { + _, sources, err := compileProtoDir(ctx, protoDir, true) + return sources, err +} + // compileProtoDir has buf, in the pinned proto companion, build protoDir's // descriptor set and, when listSources is set, name the source set it built // it from (`buf ls-files --format json`): every file buf compiles, read from @@ -796,18 +804,34 @@ func protoSourcesFromListing(root string, listing []byte) ([]proto.Source, error if len(sources) == 0 { return nil, fmt.Errorf("buf lists no .proto files to compile") } + sort.Slice(sources, func(i, j int) bool { return sources[i].Path < sources[j].Path }) return sources, nil } // copyTree copies the file tree rooted at src into dst, creating dst as // needed. A symlink is copied as what it points at, so a symlinked module -// folder reaches buf in the container, where the link's target is not mounted. +// folder reaches buf in the container, where the link's target is not mounted, +// and the published contract's proto tree is self-contained. A linked folder +// may live outside src (a module shared between services is the reason to link +// one), but a link to a folder that holds it, or to one already being copied, +// is a loop and is refused by name. func copyTree(src, dst string) error { - return filepath.WalkDir(src, func(p string, entry os.DirEntry, walkErr error) error { + root, err := filepath.EvalSymlinks(src) + if err != nil { + return err + } + return copyResolvedTree(root, dst, []string{root}) +} + +// copyResolvedTree copies root, a directory with no symlink in its path, into +// dst. copying holds every resolved folder whose copy is in progress, outermost +// first and root last. +func copyResolvedTree(root, dst string, copying []string) error { + return filepath.WalkDir(root, func(p string, entry os.DirEntry, walkErr error) error { if walkErr != nil { return walkErr } - rel, err := filepath.Rel(src, p) + rel, err := filepath.Rel(root, p) if err != nil { return err } @@ -822,7 +846,13 @@ func copyTree(src, dst string) error { if linkErr != nil { return linkErr } - return copyTree(resolved, target) + // WalkDir follows no link, so p's folder is already resolved. + for _, held := range append([]string{filepath.Dir(p)}, copying...) { + if folderHolds(resolved, held) { + return fmt.Errorf("symlink %s points at %s, which holds it: a looping link cannot be copied", p, resolved) + } + } + return copyResolvedTree(resolved, target, append(copying[:len(copying):len(copying)], resolved)) } } if entry.IsDir() { @@ -836,6 +866,12 @@ func copyTree(src, dst string) error { }) } +// folderHolds reports whether dir is folder or lies beneath it. +func folderHolds(folder, dir string) bool { + rel, err := filepath.Rel(folder, dir) + return err == nil && filepath.IsLocal(rel) +} + // runContractsCheck regenerates contracts into a throwaway directory and // diffs them against output, without writing anything real. func runContractsCheck(ctx context.Context, workspace *resources.Workspace, module *resources.Module, output string) error { diff --git a/cmd/generate/proto.go b/cmd/generate/proto.go index 21390f10..313e3d3a 100644 --- a/cmd/generate/proto.go +++ b/cmd/generate/proto.go @@ -945,13 +945,14 @@ func protoGenerationPathArgs(protoDir string, absolute bool) []string { return args } -// The template's inputs are used as declared unless the caller selects files. -// A caller's --path names files of --proto, so it selects --proto as the one -// input: buf applies a --path flag to every input, and an input that does not +// The template's inputs are used as declared, with their input-level options +// (paths, types, exclude_paths), unless a caller's --path meets more than one +// of them. buf applies a --path flag to every input, and an input that does not // hold the path targets no file and fails the whole run ("no .proto files were -// targeted"), so the template's inputs cannot be kept beside it. A positional -// input replaces them, as buf itself defines. Relative template inputs resolve -// in the template's working dir. +// targeted"), so a caller's --path over several inputs selects --proto, whose +// files it names, as the one input. A positional input replaces the template's, +// as buf itself defines. Relative template inputs resolve in the template's +// working dir. func protoGenerationArgs(protoDir, templatePath string, template []byte) ([]string, error) { var selection struct { Inputs []yaml.Node `yaml:"inputs"` @@ -960,7 +961,7 @@ func protoGenerationArgs(protoDir, templatePath string, template []byte) ([]stri return nil, fmt.Errorf("cannot read generation template inputs: %w", err) } args := []string{"generate"} - if len(selection.Inputs) == 0 || len(protoPaths) > 0 { + if len(selection.Inputs) == 0 || (len(protoPaths) > 0 && len(selection.Inputs) > 1) { args = append(args, protoDir) } args = append(args, "--template", templatePath) diff --git a/cmd/generate/proto_test.go b/cmd/generate/proto_test.go index ffaaba42..83d4a0b7 100644 --- a/cmd/generate/proto_test.go +++ b/cmd/generate/proto_test.go @@ -44,7 +44,10 @@ func TestProtoGenerationArgsPreserveBufInputSelection(t *testing.T) { {"v2 default", "version: v2\n", nil, []string{"generate", "/workspace/proto", "--template", "/staging/template.yaml"}}, {"empty inputs", "version: v2\ninputs: []\n", nil, []string{"generate", "/workspace/proto", "--template", "/staging/template.yaml"}}, {"declared selection", "version: v2\ninputs:\n - directory: .\n paths: [api.proto]\n - directory: ../other\n", nil, []string{"generate", "--template", "/staging/template.yaml"}}, - {"caller override", "version: v2\ninputs:\n - directory: .\n paths: [api.proto]\n", []string{"chosen.proto"}, []string{"generate", "/workspace/proto", "--template", "/staging/template.yaml", "--path", "/workspace/proto/chosen.proto"}}, + // One declared input keeps its input-level options beside a caller's + // --path: nothing else can be targeted by it. + {"caller override", "version: v2\ninputs:\n - directory: .\n paths: [api.proto]\n", []string{"chosen.proto"}, []string{"generate", "--template", "/staging/template.yaml", "--path", "/workspace/proto/chosen.proto"}}, + {"caller selection keeps a single input's options", "version: v2\ninputs:\n - directory: .\n types: [acme.v1.Api]\n exclude_paths: [internal]\n", []string{"api.proto"}, []string{"generate", "--template", "/staging/template.yaml", "--path", "/workspace/proto/api.proto"}}, // buf applies --path to every input: a second input that does not hold // the caller's file would target nothing and fail the run. {"caller selection over two inputs", "version: v2\ninputs:\n - directory: .\n paths: [api.proto]\n - directory: ../deps\n", []string{"api.proto"}, []string{"generate", "/workspace/proto", "--template", "/staging/template.yaml", "--path", "/workspace/proto/api.proto"}}, diff --git a/cmd/publish/clients.go b/cmd/publish/clients.go index 2dcb7b95..f1e7e61a 100644 --- a/cmd/publish/clients.go +++ b/cmd/publish/clients.go @@ -17,6 +17,7 @@ import ( "github.com/Masterminds/semver" "github.com/codefly-dev/cli/cmd/common" + "github.com/codefly-dev/cli/cmd/generate" "github.com/codefly-dev/cli/pkg/cli" clicomposition "github.com/codefly-dev/cli/pkg/composition" "github.com/codefly-dev/cli/pkg/generators" @@ -465,7 +466,7 @@ func resolveClientsOutputRoot(flag string) (root string, cleanup func(), err err // Whatever shipped is immutable, so a partial failure is recorded before it // is returned and a re-run publishes only the languages still missing. func publishClientWork(ctx context.Context, run *clientsRun, item *clientWork, cfg librarystore.StoreConfig, policy librarystore.RepositoryPolicy, outputRoot string, w io.Writer) error { - entry, err := generators.BuildModuleClientEntry(ctx, run.module.Dir(), run.module.Name, run.catalog, item.plan) + entry, err := generators.BuildModuleClientEntry(ctx, run.module.Dir(), run.module.Name, run.catalog, item.plan, generate.ListProtoSources) if err != nil { return err } diff --git a/cmd/publish/clients_proto_sources_companion_test.go b/cmd/publish/clients_proto_sources_companion_test.go new file mode 100644 index 00000000..7c728fb9 --- /dev/null +++ b/cmd/publish/clients_proto_sources_companion_test.go @@ -0,0 +1,49 @@ +//go:build proto_companion_required + +package publish + +import ( + "os" + "path/filepath" + "testing" + + "github.com/codefly-dev/cli/cmd/generate" + "github.com/codefly-dev/cli/pkg/generators" + "github.com/codefly-dev/core/composition" + "github.com/stretchr/testify/require" +) + +// `generate contracts` publishes a service's proto tree verbatim beside its +// contract, buf.yaml included. Over a tree with a vendor module and excluded +// folders (module "." excludes codefly and vendor; vendor is its own module), +// publish clients names the sources as buf does: the vendored file by its +// import path, never vendor/..., and the excluded facade not at all. +func TestPublishedContractSourcesFollowBufModulesThroughPinnedCompanion(t *testing.T) { + moduleDir := t.TempDir() + contractDir := filepath.Join(moduleDir, "contracts", "api", "accounts", "connect") + contract := []byte("descriptor bytes") + for rel, content := range map[string]string{ + "contract.binpb": string(contract), + "proto/buf.yaml": "version: v2\nmodules:\n - path: .\n excludes: [codefly, vendor]\n - path: vendor\n", + "proto/accounts/v1/accounts.proto": "syntax = \"proto3\";\npackage accounts.v1;\nimport \"codefly/base/v0/base.proto\";\nmessage Account { codefly.base.v0.Ref ref = 1; }\n", + "proto/codefly/api.proto": "syntax = \"proto3\";\npackage codefly.facade;\nimport \"accounts/v1/accounts.proto\";\nmessage Facade { accounts.v1.Account account = 1; }\n", + "proto/vendor/codefly/base/v0/base.proto": "syntax = \"proto3\";\npackage codefly.base.v0;\nmessage Ref { string id = 1; }\n", + } { + path := filepath.Join(contractDir, filepath.FromSlash(rel)) + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) + require.NoError(t, os.WriteFile(path, []byte(content), 0o600)) + } + plan := &generators.ModuleClientPlan{Endpoint: composition.APIContractEndpoint{ + Service: "accounts", Endpoint: "connect", Kind: composition.APIContractKindProtobuf, + Path: "contracts/api/accounts/connect/contract.binpb", Digest: composition.APIContractDigest(contract), + }} + catalog := &composition.APIContractCatalog{Package: "codefly/saas-starter", Version: "0.1.0"} + + entry, err := generators.BuildModuleClientEntry(t.Context(), moduleDir, "saas-starter", catalog, plan, generate.ListProtoSources) + require.NoError(t, err) + var names []string + for _, source := range entry.ProtoSources { + names = append(names, source.Path) + } + require.Equal(t, []string{"accounts/v1/accounts.proto", "codefly/base/v0/base.proto"}, names) +} diff --git a/pkg/generators/module_clients.go b/pkg/generators/module_clients.go index a17369b5..034b6cb9 100644 --- a/pkg/generators/module_clients.go +++ b/pkg/generators/module_clients.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "io" - "io/fs" "os" "path/filepath" "sort" @@ -172,11 +171,18 @@ func PlanModuleClients(moduleName string, catalog *composition.APIContractCatalo return plans, nil } +// ProtoSourceLister names a proto tree's source set as buf compiles it. +// `codefly generate` owns the one implementation (generate.ListProtoSources), +// which runs buf in the pinned proto companion. +type ProtoSourceLister func(ctx context.Context, protoDir string) ([]coreproto.Source, error) + // BuildModuleClientEntry reads exactly the committed package contract that is // being published. Callers validate the package manifest, catalog and artifact // digests before invoking it, so generated clients cannot diverge from the -// module package under the same version. -func BuildModuleClientEntry(ctx context.Context, moduleDir, moduleName string, catalog *composition.APIContractCatalog, plan *ModuleClientPlan) (*ContractEntry, error) { +// module package under the same version. A protobuf contract's sources are +// what listSources names over the proto tree published beside it, so buf.yaml +// modules and excludes decide them exactly as they did the contract. +func BuildModuleClientEntry(ctx context.Context, moduleDir, moduleName string, catalog *composition.APIContractCatalog, plan *ModuleClientPlan, listSources ProtoSourceLister) (*ContractEntry, error) { contractBytes, err := ReadCatalogContractBytes(moduleDir, &plan.Endpoint) if err != nil { return nil, fmt.Errorf("read contract for %s/%s: %w", plan.Endpoint.Service, plan.Endpoint.Endpoint, err) @@ -184,7 +190,7 @@ func BuildModuleClientEntry(ctx context.Context, moduleDir, moduleName string, c var protoSources []coreproto.Source if plan.Endpoint.Kind == composition.APIContractKindProtobuf { protoDir := filepath.Join(filepath.Dir(filepath.Join(moduleDir, filepath.FromSlash(plan.Endpoint.Path))), "proto") - protoSources, err = collectPublishedProtoSources(ctx, protoDir) + protoSources, err = listSources(ctx, protoDir) if err != nil { return nil, fmt.Errorf("read proto sources for %s/%s: %w", plan.Endpoint.Service, plan.Endpoint.Endpoint, err) } @@ -201,39 +207,6 @@ func BuildModuleClientEntry(ctx context.Context, moduleDir, moduleName string, c }, nil } -func collectPublishedProtoSources(ctx context.Context, protoDir string) ([]coreproto.Source, error) { - var sources []coreproto.Source - err := filepath.WalkDir(protoDir, func(path string, entry fs.DirEntry, walkErr error) error { - if walkErr != nil { - return walkErr - } - if err := ctx.Err(); err != nil { - return err - } - if entry.IsDir() || filepath.Ext(path) != ".proto" { - return nil - } - rel, err := filepath.Rel(protoDir, path) - if err != nil { - return err - } - content, err := os.ReadFile(path) //nolint:gosec // walked beneath the validated contract directory. - if err != nil { - return err - } - sources = append(sources, coreproto.Source{Path: filepath.ToSlash(rel), Content: content}) - return nil - }) - if err != nil { - return nil, err - } - if len(sources) == 0 { - return nil, fmt.Errorf("no .proto files under %s", protoDir) - } - sort.Slice(sources, func(i, j int) bool { return sources[i].Path < sources[j].Path }) - return sources, nil -} - func containsLanguage(list []languages.Language, lang languages.Language) bool { for _, candidate := range list { if candidate == lang { diff --git a/pkg/generators/module_clients_test.go b/pkg/generators/module_clients_test.go index 4ca3ef91..5545e40a 100644 --- a/pkg/generators/module_clients_test.go +++ b/pkg/generators/module_clients_test.go @@ -1,10 +1,12 @@ package generators import ( + "context" "os" "path/filepath" "testing" + coreproto "github.com/codefly-dev/core/companions/proto" "github.com/codefly-dev/core/composition" "github.com/codefly-dev/core/languages" "github.com/stretchr/testify/require" @@ -147,9 +149,8 @@ func TestBuildModuleClientEntryUsesCommittedPackageArtifacts(t *testing.T) { dir := t.TempDir() contract := []byte("descriptor bytes") contractPath := filepath.Join(dir, "contracts", "api", "accounts", "connect", "contract.binpb") - require.NoError(t, os.MkdirAll(filepath.Join(filepath.Dir(contractPath), "proto", "accounts", "v1"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Dir(contractPath), 0o755)) require.NoError(t, os.WriteFile(contractPath, contract, 0o644)) - require.NoError(t, os.WriteFile(filepath.Join(filepath.Dir(contractPath), "proto", "accounts", "v1", "accounts.proto"), []byte("syntax = \"proto3\";"), 0o644)) catalog := &composition.APIContractCatalog{Package: "codefly/saas-starter", Version: "0.1.0"} plan := &ModuleClientPlan{ Endpoint: composition.APIContractEndpoint{ @@ -158,9 +159,17 @@ func TestBuildModuleClientEntryUsesCommittedPackageArtifacts(t *testing.T) { }, Services: []string{"AccountsService"}, } - entry, err := BuildModuleClientEntry(t.Context(), dir, "saas-starter", catalog, plan) + // The sources are what the lister names over the proto tree published + // beside the contract; nothing here walks that tree itself. + listed := []coreproto.Source{{Path: "accounts/v1/accounts.proto", Content: []byte("syntax = \"proto3\";")}} + var listedDir string + entry, err := BuildModuleClientEntry(t.Context(), dir, "saas-starter", catalog, plan, func(_ context.Context, protoDir string) ([]coreproto.Source, error) { + listedDir = protoDir + return listed, nil + }) require.NoError(t, err) + require.Equal(t, filepath.Join(filepath.Dir(contractPath), "proto"), listedDir) require.Equal(t, contract, entry.ContractBytes) - require.Equal(t, "accounts/v1/accounts.proto", entry.ProtoSources[0].Path) + require.Equal(t, listed, entry.ProtoSources) require.Equal(t, []string{"AccountsService"}, entry.Services) }