diff --git a/cookbooks.go b/cookbooks.go index b5c34c2..b5d4672 100644 --- a/cookbooks.go +++ b/cookbooks.go @@ -101,20 +101,35 @@ type Cookbook struct { } // AllFiles flattens the flat all_files manifest and all nine per-segment slices -// into a single slice. A server response populates one shape or the other, so -// the merge yields the cookbook's files in either case. +// into a single slice, deduplicated by path. +// +// Most servers populate one shape or the other, but nothing guarantees it, and +// a file listed in both must still yield one entry: callers turn each entry +// into work (Download turns it into a file to fetch and write), so a duplicate +// means two writers for one path. The all_files entry wins, and within the +// per-segment slices the first occurrence does. func (cb *Cookbook) AllFiles() []CookbookFileRef { - all := make([]CookbookFileRef, 0, - len(cb.AllFilesManifest)+ - len(cb.Files)+len(cb.Definitions)+len(cb.Libraries)+ - len(cb.Attributes)+len(cb.Recipes)+len(cb.Providers)+ - len(cb.Resources)+len(cb.RootFiles)+len(cb.Templates)) - all = append(all, cb.AllFilesManifest...) + total := len(cb.AllFilesManifest) + + len(cb.Files) + len(cb.Definitions) + len(cb.Libraries) + + len(cb.Attributes) + len(cb.Recipes) + len(cb.Providers) + + len(cb.Resources) + len(cb.RootFiles) + len(cb.Templates) + all := make([]CookbookFileRef, 0, total) + seen := make(map[string]bool, total) + add := func(refs []CookbookFileRef) { + for _, ref := range refs { + if seen[ref.Path] { + continue + } + seen[ref.Path] = true + all = append(all, ref) + } + } + add(cb.AllFilesManifest) for _, seg := range [][]CookbookFileRef{ cb.Files, cb.Definitions, cb.Libraries, cb.Attributes, cb.Recipes, cb.Providers, cb.Resources, cb.RootFiles, cb.Templates, } { - all = append(all, seg...) + add(seg) } return all } diff --git a/cookbooks_test.go b/cookbooks_test.go index 30b3aa5..2d2e120 100644 --- a/cookbooks_test.go +++ b/cookbooks_test.go @@ -7,11 +7,13 @@ import ( "fmt" "io" "net/http" + "net/http/httptest" "os" "path/filepath" "slices" "strings" "sync" + "sync/atomic" "testing" "github.com/cinc-project/cinc-api/internal/cinctest" @@ -691,3 +693,55 @@ func TestLocalCookbookFromDir_SkipsDanglingSymlink(t *testing.T) { t.Errorf("packed %+v, want just metadata.rb", cb.files) } } + +// A server may populate both the flat all_files manifest and the per-segment +// slices. Merging them must not yield the same file twice: Download turns each +// entry into a job, and two jobs for one path race to write the same file. +func TestAllFiles_DedupesByPath(t *testing.T) { + cb := &Cookbook{ + AllFilesManifest: []CookbookFileRef{ + {Path: "recipes/default.rb", Checksum: "aa"}, + {Path: "metadata.rb", Checksum: "bb"}, + }, + Recipes: []CookbookFileRef{{Path: "recipes/default.rb", Checksum: "aa"}}, + RootFiles: []CookbookFileRef{{Path: "metadata.rb", Checksum: "bb"}}, + } + all := cb.AllFiles() + if len(all) != 2 { + t.Fatalf("AllFiles() returned %d refs, want 2: %+v", len(all), all) + } + seen := map[string]bool{} + for _, ref := range all { + if seen[ref.Path] { + t.Errorf("duplicate path %q", ref.Path) + } + seen[ref.Path] = true + } +} + +// Download must fetch and write each cookbook file exactly once, even when the +// manifest lists it in both shapes. +func TestDownload_FetchesEachFileOnce(t *testing.T) { + var fetches int64 + files := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt64(&fetches, 1) + w.Write([]byte("# default recipe\n")) + })) + defer files.Close() + + srv := cinctest.New(t) + srv.Handle("GET /organizations/o/cookbooks/nginx/1.0.0", cinctest.Route{ + Body: fmt.Sprintf(`{"cookbook_name":"nginx","version":"1.0.0", + "all_files":[{"path":"recipes/default.rb","url":%q}], + "recipes":[{"path":"recipes/default.rb","url":%q}]}`, + files.URL+"/f", files.URL+"/f"), + }) + c := newTestClient(t, srv.Server) + dest := t.TempDir() + if err := c.Cookbooks.Download(context.Background(), "nginx", "1.0.0", dest); err != nil { + t.Fatalf("Download: %v", err) + } + if got := atomic.LoadInt64(&fetches); got != 1 { + t.Errorf("bookshelf fetches = %d, want 1", got) + } +}