fix: remove Output.Path internally

Having both Output.Path and Output.Paths was a source of errors waiting to
happen if the caller didn't call ResolvePaths first. This patch resolves the
paths when deserialising so it reduces the mental load overall.

Author: jgresty

config/api.go

@@ -1,6 +1,7 @@
 package config
 
 import (
+	"encoding/json"
 	"fmt"
 
 	"github.com/bmatcuk/doublestar/v4"
@@ -69,23 +70,36 @@ type Overlay struct {
 // Output defines where the aggregate versioned OpenAPI specs should be created
 // during compilation.
 type Output struct {
+	Paths []string
+}
+
+// outputJSON exists for historical purposes, we allowed both Path and Paths to
+// be specified in the api spec config. This makes handling internally more
+// complex as we have to deal with both, instead we can deal with it at
+// deserialisation time.
+type outputJSON struct {
 	Path  string   `json:"path,omitempty"`
 	Paths []string `json:"paths,omitempty"`
 }
 
-// EffectivePaths returns a slice of effective configured output paths, whether
-// a single or multiple output paths have been configured.
-func (o *Output) ResolvePaths() []string {
-	if o == nil {
-		return []string{}
+func (o *Output) UnmarshalJSON(data []byte) error {
+	oj := outputJSON{}
+	err := json.Unmarshal(data, &oj)
+	if err != nil {
+		return err
 	}
-	if o.Path != "" {
-		return []string{o.Path}
+	if oj.Path != "" {
+		if len(oj.Paths) > 0 {
+			return fmt.Errorf("output should specify one of 'path' or 'paths', not both")
+		}
+		o.Paths = []string{oj.Path}
+		return nil
 	}
-	return o.Paths
+	o.Paths = oj.Paths
+	return nil
 }
 
-func (a APIs) init(p *Project) error {
+func (a APIs) init() error {
 	if len(a) == 0 {
 		return fmt.Errorf("no apis defined")
 	}
@@ -100,12 +114,6 @@ func (a APIs) init(p *Project) error {
 				return fmt.Errorf("%w (apis.%s.resources[%d])", err, api.Name, rcIndex)
 			}
 		}
-		if api.Output != nil {
-			if len(api.Output.Paths) > 0 && api.Output.Path != "" {
-				return fmt.Errorf("output should specify one of 'path' or 'paths', not both (apis.%s.output)",
-					api.Name)
-			}
-		}
 	}
 	return nil
 }

config/api_test.go

@@ -1,44 +1,56 @@
 package config_test
 
 import (
-	"reflect"
+	"encoding/json"
 	"testing"
 
+	qt "github.com/frankban/quicktest"
+
 	"github.com/snyk/vervet/v8/config"
 )
 
-func TestOutput_ResolvePaths(t *testing.T) {
+func TestOutput_deserialise(t *testing.T) {
 	tests := []struct {
-		name    string
-		subject *config.Output
-		want    []string
+		name        string
+		subject     string
+		wantPaths   []string
+		expectedErr string
 	}{
 		{
-			name:    "nil",
-			subject: nil,
-			want:    []string{},
+			name:        "nil",
+			subject:     "{}",
+			wantPaths:   []string(nil),
+			expectedErr: "",
+		},
+		{
+			name:        "returns path if exists",
+			subject:     `{"path": "path1"}`,
+			wantPaths:   []string{"path1"},
+			expectedErr: "",
 		},
 		{
-			name: "returns path if exists",
-			subject: &config.Output{
-				Path:  "path",
-				Paths: []string{"path1", "path2"},
-			},
-			want: []string{"path"},
+			name:        "return paths if exists",
+			subject:     `{"paths": ["path1", "path2"]}`,
+			wantPaths:   []string{"path1", "path2"},
+			expectedErr: "",
 		},
 		{
-			name: "return paths if path is empty",
-			subject: &config.Output{
-				Path:  "",
-				Paths: []string{"path1", "path2"},
-			},
-			want: []string{"path1", "path2"},
+			name:        "errors if both path and paths exist",
+			subject:     `{"path": "path1", "paths": ["path1", "path2"]}`,
+			wantPaths:   []string{},
+			expectedErr: "output should specify one of 'path' or 'paths', not both",
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := tt.subject.ResolvePaths(); !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("ResolvePaths() = %v, want %v", got, tt.want)
+			c := qt.New(t)
+			out := config.Output{}
+			err := json.Unmarshal([]byte(tt.subject), &out)
+			if tt.expectedErr != "" {
+				c.Assert(err.Error(), qt.Equals, tt.expectedErr)
+			} else {
+				c.Assert(err, qt.IsNil)
+				c.Assert(out.Paths, qt.DeepEquals, tt.wantPaths)
 			}
 		})
 	}

config/config_test.go

@@ -52,7 +52,7 @@ servers:
     description: Test API`[1:],
 				}},
 				Output: &config.Output{
-					Path: "testdata/output",
+					Paths: []string{"testdata/output"},
 				},
 			},
 		},
@@ -90,7 +90,9 @@ apis:
         - /another/place
         - /and/another
 `[1:],
-		err: `output should specify one of 'path' or 'paths', not both \(apis\.testapi\.output\)`,
+		err: "failed to unmarshal project configuration: " +
+			"error unmarshaling JSON: " +
+			"output should specify one of 'path' or 'paths', not both",
 	}, {
 		err: `no apis defined`,
 	}}

config/project.go

@@ -45,7 +45,7 @@ func (p *Project) validate() error {
 	if err != nil {
 		return err
 	}
-	err = p.APIs.init(p)
+	err = p.APIs.init()
 	if err != nil {
 		return err
 	}

internal/cmd/backstage.go

@@ -190,7 +190,7 @@ func processCatalog(ctx *cli.Context, w io.Writer) error {
 	sort.Strings(apiNames)
 	for _, apiName := range apiNames {
 		apiConf := proj.APIs[apiName]
-		outputPaths := apiConf.Output.ResolvePaths()
+		outputPaths := apiConf.Output.Paths
 		if len(outputPaths) == 0 {
 			log.Printf("API %q has no output paths, this command will have no effect", apiName)
 			continue

internal/cmd/compiler.go

@@ -151,7 +151,7 @@ func projectFromContext(ctx *cli.Context) (*config.Project, error) {
 			Path: ctx.Args().Get(0),
 		}},
 		Output: &config.Output{
-			Path: ctx.Args().Get(1),
+			Paths: []string{ctx.Args().Get(1)},
 		},
 	}
 	if includePath := ctx.String("include"); includePath != "" {

internal/compiler/compiler.go

@@ -102,9 +102,6 @@ func New(ctx context.Context, proj *config.Project, options ...CompilerOption) (
 		// Build output
 		if apiConfig.Output != nil {
 			paths := apiConfig.Output.Paths
-			if len(paths) == 0 && apiConfig.Output.Path != "" {
-				paths = []string{apiConfig.Output.Path}
-			}
 			if len(paths) > 0 {
 				a.output = &output{
 					paths: paths,

internal/simplebuild/build_test.go

@@ -857,7 +857,7 @@ func TestBuildSkipsVersionCheckWhenFetchFails(t *testing.T) {
 	c.Assert(err, qt.IsNil)
 
 	dummyOutput := &config.Output{
-		Path: tempDir,
+		Paths: []string{tempDir},
 	}
 	dummyAPI := &config.API{
 		Name:      "dummy-api",

internal/simplebuild/output.go

@@ -27,7 +27,7 @@ type DocWriter struct {
 // NewWriter initialises any output paths, removing existing files and
 // directories if they are present.
 func NewWriter(cfg config.Output, appendOutputFiles bool) (*DocWriter, error) {
-	paths := getOutputPaths(cfg)
+	paths := cfg.Paths
 	toClear := paths
 	if appendOutputFiles {
 		// We treat the first path as the source of truth and copy the whole
@@ -128,22 +128,6 @@ func (out *DocWriter) Finalize() error {
 	return nil
 }
 
-// Some services have a need to write specs to multiple destinations. This
-// tends to happen in Typescript services in which we want to write specs to
-// two places:
-//   - src/** for committing into git and ingesting into Backstage
-//   - dist/** for runtime module access to compiled specs.
-//
-// To maintain backwards compatibility we still allow a single path in the
-// config file then normalise that here to an array.
-func getOutputPaths(cfg config.Output) []string {
-	paths := cfg.Paths
-	if len(paths) == 0 && cfg.Path != "" {
-		paths = []string{cfg.Path}
-	}
-	return paths
-}
-
 func getExisingSpecFiles(dir string) ([]string, error) {
 	var outputFiles []string
 	err := filepath.WalkDir(dir, func(filePath string, d fs.DirEntry, err error) error {

internal/simplebuild/output_test.go

@@ -36,7 +36,7 @@ func TestDocSet_WriteOutputs(t *testing.T) {
 			name: "write the doc sets to outputs",
 			args: args{
 				cfg: config.Output{
-					Path: t.TempDir(),
+					Paths: []string{t.TempDir()},
 				},
 			},
 			docs: DocSet{
@@ -47,10 +47,10 @@ func TestDocSet_WriteOutputs(t *testing.T) {
 			},
 			assert: func(t *testing.T, args args) {
 				t.Helper()
-				files, err := filepath.Glob(filepath.Join(args.cfg.Path, "*"))
+				files, err := filepath.Glob(filepath.Join(args.cfg.Paths[0], "*"))
 				c.Assert(err, qt.IsNil)
 				c.Assert(files, qt.HasLen, 2)
-				goEmbedContents, err := os.ReadFile(path.Join(args.cfg.Path, "embed.go"))
+				goEmbedContents, err := os.ReadFile(path.Join(args.cfg.Paths[0], "embed.go"))
 				c.Assert(err, qt.IsNil)
 				c.Assert(string(goEmbedContents), qt.Contains, "2024-01-01")
 			},
@@ -59,7 +59,7 @@ func TestDocSet_WriteOutputs(t *testing.T) {
 			name: "clears dir if appendOutputFiles is false",
 			args: args{
 				cfg: config.Output{
-					Path: t.TempDir(),
+					Paths: []string{t.TempDir()},
 				},
 				appendOutputFiles: false,
 			},
@@ -71,15 +71,15 @@ func TestDocSet_WriteOutputs(t *testing.T) {
 			},
 			setup: func(t *testing.T, args args) {
 				t.Helper()
-				err = os.WriteFile(path.Join(args.cfg.Path, "existing-file"), []byte("existing"), 0644)
+				err = os.WriteFile(path.Join(args.cfg.Paths[0], "existing-file"), []byte("existing"), 0644)
 				c.Assert(err, qt.IsNil)
 			},
 			assert: func(t *testing.T, args args) {
 				t.Helper()
-				files, err := filepath.Glob(filepath.Join(args.cfg.Path, "*"))
+				files, err := filepath.Glob(filepath.Join(args.cfg.Paths[0], "*"))
 				c.Assert(err, qt.IsNil)
 				c.Assert(files, qt.HasLen, 2)
-				goEmbedContents, err := os.ReadFile(path.Join(args.cfg.Path, "embed.go"))
+				goEmbedContents, err := os.ReadFile(path.Join(args.cfg.Paths[0], "embed.go"))
 				c.Assert(err, qt.IsNil)
 				c.Assert(string(goEmbedContents), qt.Contains, "2024-01-01")
 			},
@@ -89,7 +89,7 @@ func TestDocSet_WriteOutputs(t *testing.T) {
 			name: "merges files if appendOutputFiles is true, embeds existing files",
 			args: args{
 				cfg: config.Output{
-					Path: t.TempDir(),
+					Paths: []string{t.TempDir()},
 				},
 				appendOutputFiles: true,
 			},
@@ -101,19 +101,19 @@ func TestDocSet_WriteOutputs(t *testing.T) {
 			},
 			setup: func(t *testing.T, args args) {
 				t.Helper()
-				err = os.WriteFile(path.Join(args.cfg.Path, "2024-02-01"), []byte("existing"), 0644)
+				err = os.WriteFile(path.Join(args.cfg.Paths[0], "2024-02-01"), []byte("existing"), 0644)
 				c.Assert(err, qt.IsNil)
-				err = os.WriteFile(path.Join(args.cfg.Path, "2024-02-02"), []byte("existing"), 0644)
+				err = os.WriteFile(path.Join(args.cfg.Paths[0], "2024-02-02"), []byte("existing"), 0644)
 				c.Assert(err, qt.IsNil)
-				err = os.WriteFile(path.Join(args.cfg.Path, "2024-02-03"), []byte("existing"), 0644)
+				err = os.WriteFile(path.Join(args.cfg.Paths[0], "2024-02-03"), []byte("existing"), 0644)
 				c.Assert(err, qt.IsNil)
 			},
 			assert: func(t *testing.T, args args) {
 				t.Helper()
-				files, err := filepath.Glob(filepath.Join(args.cfg.Path, "*"))
+				files, err := filepath.Glob(filepath.Join(args.cfg.Paths[0], "*"))
 				c.Assert(err, qt.IsNil)
 				c.Assert(files, qt.HasLen, 2+3)
-				goEmbedContents, err := os.ReadFile(path.Join(args.cfg.Path, "embed.go"))
+				goEmbedContents, err := os.ReadFile(path.Join(args.cfg.Paths[0], "embed.go"))
 				c.Assert(err, qt.IsNil)
 				c.Assert(string(goEmbedContents), qt.Contains, "2024-01-01")
 				c.Assert(string(goEmbedContents), qt.Contains, "2024-02-01")

specs/load.go

@@ -56,14 +56,14 @@ func GetOutputSpecs(ctx context.Context, api *config.API) ([]*vervet.Document, e
 // lazy operations.
 func GetOutputSpecsItr(ctx context.Context, api *config.API) iter.Seq2[*vervet.Document, error] {
 	return func(yield func(*vervet.Document, error) bool) {
-		path := api.Output.ResolvePaths()
-		if len(path) == 0 {
+		outPaths := api.Output.Paths
+		if len(outPaths) == 0 {
 			// No output defined for this api
 			return
 		}
 		resource := &config.ResourceSet{
 			// All output paths should have the same contents
-			Path: path[0],
+			Path: outPaths[0],
 		}
 		paths, err := files.LocalFSSource{}.Match(resource)
 		if err != nil {