From 40f12412d8f7159ddf4fef3efda1f8eb82de94b3 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 04:00:54 +0300 Subject: [PATCH 01/11] feat(engine): let compilers own format detection and options --- README.md | 22 ++- cmd/morphic/command_test.go | 5 +- cmd/morphic/compile.go | 15 +- cmd/morphic/compile_test.go | 91 +++++++++- cmd/morphic/edgecases_test.go | 6 + cmd/morphic/options.go | 51 ++++++ cmd/morphic/options_test.go | 64 +++++++ cmd/morphic/testdata/compile-help.txt | 6 + compilers/compilers.go | 65 +++++++ compilers/compilers_test.go | 108 +++++++++++- compilers/openapi/detect.go | 155 +++++++++++++++++ compilers/openapi/detect_test.go | 181 ++++++++++++++++++++ compilers/openapi/doc.go | 6 +- compilers/openapi/options.go | 128 +++++++++++++- compilers/openapi/options_test.go | 123 ++++++++++++++ docs/architecture.md | 11 +- engine/doc.go | 8 +- engine/engine.go | 90 +++++++--- engine/engine_test.go | 233 ++++++++++++++++++++++---- engine/sniff.go | 54 ------ engine/sniff_test.go | 45 ----- internal/archtest/arch_test.go | 6 +- 22 files changed, 1301 insertions(+), 172 deletions(-) create mode 100644 cmd/morphic/options.go create mode 100644 cmd/morphic/options_test.go create mode 100644 compilers/openapi/detect.go create mode 100644 compilers/openapi/detect_test.go delete mode 100644 engine/sniff.go delete mode 100644 engine/sniff_test.go diff --git a/README.md b/README.md index ccb1d64c..3d498570 100644 --- a/README.md +++ b/README.md @@ -118,15 +118,35 @@ The flags below are `compile`'s: | `--fail-on error\|warning` | Exit non-zero when a diagnostic at or above this severity is emitted (default `error`). | | `--skip-validate` | Skip the referential-integrity `validate` pass. | | `--explain ` | Report what compiling produced at this source coordinate instead of writing the document. | +| `--opt =` | Set one option on the compiler the spec selects. Repeatable; a repeated key is refused. | Diagnostics print one per line as ` #: `. Exit codes: `0` clean (and for any help request), `1` a diagnostic reached the `--fail-on` threshold (or the spec could not be lowered), `2` a usage or I/O error. +#### Compiler options + +`--opt` names an option in the vocabulary of whichever compiler recognizes the spec — morphic +itself knows none of them, and an unknown name is refused by the compiler rather than ignored. +The OpenAPI compiler accepts: + +| Option | Values | Meaning | +|---|---|---| +| `grouping` | `tags` (default), `path-prefix` | How operations are grouped into operation groups. | +| `allow-external-refs` | `true`, `false` (default) | Let `$ref` resolution leave the source document, reading files and fetching URLs. | +| `overlay` | a file path | Apply an [OpenAPI Overlay](https://spec.openapis.org/overlay/latest.html) document to the source before lowering. | +| `overlay-lax` | `true`, `false` (default) | Do not refuse when an overlay action's selector matches nothing. | + +```bash +morphic compile openapi.yaml --opt grouping=path-prefix --opt overlay=patch.yaml +``` + ### Library The same pipeline is available as a package. `engine.New` builds the default registry (OpenAPI -compiler + `validate` pass); `Run` sniffs the format, compiles, and runs passes. +compiler + `validate` pass); `Run` asks the registered compilers which of them recognizes the +source, compiles, and runs passes. A Go caller can set compiler options as a typed value through +`RunOptions.FormatOptions` instead of as text through `RunOptions.CompilerOptions`. ```go eng, err := engine.New() diff --git a/cmd/morphic/command_test.go b/cmd/morphic/command_test.go index a94b45b4..509da896 100644 --- a/cmd/morphic/command_test.go +++ b/cmd/morphic/command_test.go @@ -61,11 +61,14 @@ func TestNewCompileFlags_DefinesEveryFlag(t *testing.T) { assert.Empty(t, opts.outPath) assert.False(t, opts.skipValidate) assert.Empty(t, opts.explain) + assert.NotNil(t, opts.settings, + "the settings map must exist before Parse writes into it") + assert.Empty(t, opts.settings) } // compileFlagNames is every flag compile accepts, asserted from both the // constructor and the command-table entry so the two cannot drift. -var compileFlagNames = []string{"o", "fail-on", "skip-validate", "explain"} +var compileFlagNames = []string{"o", "fail-on", "skip-validate", "explain", "opt"} func TestCommand_PrintFlagsDocumentsTheCommandsOwnFlags(t *testing.T) { t.Parallel() diff --git a/cmd/morphic/compile.go b/cmd/morphic/compile.go index bad5c41f..391b25f5 100644 --- a/cmd/morphic/compile.go +++ b/cmd/morphic/compile.go @@ -73,7 +73,10 @@ func newCompileCommand() command { "diagnostics to stderr.\n\n" + "--explain reports what compiling produced at one source coordinate — the\n" + "type node interned there, the coordinates interned beneath it, and the\n" + - "diagnostics stamped at it — instead of writing the document.", + "diagnostics stamped at it — instead of writing the document.\n\n" + + "--opt passes a setting to the compiler the spec selects, which names and\n" + + "validates its own options; morphic itself knows none of them. The OpenAPI\n" + + "compiler's are listed in the README.", printFlags: func(w io.Writer) { fs, _ := newCompileFlags() fs.SetOutput(w) @@ -89,6 +92,7 @@ type compileOptions struct { failOn string skipValidate bool explain string + settings settingFlag } // newCompileFlags returns compile's FlagSet and the options its flags write @@ -100,7 +104,7 @@ func newCompileFlags() (*flag.FlagSet, *compileOptions) { fs := flag.NewFlagSet("compile", flag.ContinueOnError) fs.SetOutput(io.Discard) - var opts compileOptions + opts := compileOptions{settings: settingFlag{}} fs.StringVar(&opts.outPath, "o", "", "write IR JSON to this file instead of stdout") fs.StringVar(&opts.failOn, "fail-on", "error", "fail (exit 1) on diagnostics at or above this severity: error|warning") @@ -108,6 +112,8 @@ func newCompileFlags() (*flag.FlagSet, *compileOptions) { "skip the referential-integrity validate pass") fs.StringVar(&opts.explain, "explain", "", "report what compiling produced at this source pointer instead of writing IR JSON") + fs.Var(opts.settings, "opt", + "set one `key=value` option on the compiler the spec selects (repeatable)") return fs, &opts } @@ -157,7 +163,10 @@ func compileSpec(specPath string, opts compileOptions, stdout, stderr io.Writer) return 2 } - res, err := eng.Run(context.Background(), specPath, engine.RunOptions{SkipValidate: opts.skipValidate}) + res, err := eng.Run(context.Background(), specPath, engine.RunOptions{ + CompilerOptions: opts.settings, + SkipValidate: opts.skipValidate, + }) if err != nil { emitf(stderr, "morphic: %v\n", err) return 2 diff --git a/cmd/morphic/compile_test.go b/cmd/morphic/compile_test.go index 0ec71571..e9f3f67e 100644 --- a/cmd/morphic/compile_test.go +++ b/cmd/morphic/compile_test.go @@ -45,7 +45,7 @@ func TestRun_ParseUnknownSpecFails(t *testing.T) { var stdout, stderr bytes.Buffer code := run([]string{"compile", spec}, &stdout, &stderr) assert.Equal(t, 2, code) - assert.Contains(t, stderr.String(), "unrecognized spec format") + assert.Contains(t, stderr.String(), "no compiler recognizes") } func TestRun_DiagnosticsGateExitCode(t *testing.T) { @@ -108,3 +108,92 @@ func TestRun_UsageErrors(t *testing.T) { }) } } + +// groupingSpec tags its one operation "zoo" while its path starts with "a", so +// the two grouping strategies name the group differently and the assertion below +// cannot pass by accident. +const groupingSpec = `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: + /a/b: + get: + operationId: ab + tags: [zoo] + responses: {"200": {description: ok}} +` + +// TestRun_CompilerOptionReachesTheCompiler asserts from the CLI what the Go API +// could already do: a compiler option the user set changes the document. The +// control run pins that the difference is the flag and not the spec. +func TestRun_CompilerOptionReachesTheCompiler(t *testing.T) { + t.Parallel() + spec := writeFile(t, "spec.yaml", groupingSpec) + + assert.Equal(t, "zoo", groupNameOf(t, spec), "the default groups by tag") + assert.Equal(t, "a", groupNameOf(t, spec, "-opt", "grouping=path-prefix"), + "the compiler option must reach the compiler") +} + +// groupNameOf compiles spec with args and returns its first operation group's +// source name. +func groupNameOf(t *testing.T, spec string, args ...string) string { + t.Helper() + var stdout, stderr bytes.Buffer + code := run(append([]string{"compile", spec}, args...), &stdout, &stderr) + require.Equal(t, 0, code, "stderr: %s", stderr.String()) + + var doc ir.Document + require.NoError(t, json.Unmarshal(stdout.Bytes(), &doc)) + require.Len(t, doc.Services, 1) + require.NotEmpty(t, doc.Services[0].Groups) + return doc.Services[0].Groups[0].Name.Source +} + +// TestRun_OverlayOptionReachesTheCompiler covers the option whose value is a +// file. A compiler reads no files of its own, so the whole chain has to work for +// this to compile anything: the CLI collects the path, the engine loads it, and +// the compiler decodes the bytes into its own overlay type. +func TestRun_OverlayOptionReachesTheCompiler(t *testing.T) { + t.Parallel() + dir := t.TempDir() + spec := filepath.Join(dir, "spec.yaml") + require.NoError(t, os.WriteFile(spec, []byte(testspec.Tiny), 0o644)) + overlay := filepath.Join(dir, "patch.yaml") + require.NoError(t, os.WriteFile(overlay, []byte(`overlay: 1.0.0 +info: {title: Patch, version: "1"} +actions: + - target: $.info + update: {title: Patched} +`), 0o644)) + + var stdout, stderr bytes.Buffer + code := run([]string{"compile", spec, "--opt", "overlay=" + overlay}, &stdout, &stderr) + require.Equal(t, 0, code, "stderr: %s", stderr.String()) + + var doc ir.Document + require.NoError(t, json.Unmarshal(stdout.Bytes(), &doc)) + assert.Equal(t, "Patched", doc.Name, "the overlay must have been applied") + assert.Len(t, doc.Sources, 2, "an applied overlay is a second source") +} + +func TestRun_BadCompilerOptionIsRefused(t *testing.T) { + t.Parallel() + spec := writeFile(t, "spec.yaml", testspec.Tiny) + tests := []struct { + name, arg, wantErr string + }{ + {"malformed pair", "grouping", "want key=value"}, + {"unknown name", "gruoping=tags", `unknown option "gruoping"`}, + {"unusable value", "grouping=alphabetical", `want "tags" or "path-prefix"`}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var stdout, stderr bytes.Buffer + code := run([]string{"compile", spec, "--opt", tc.arg}, &stdout, &stderr) + assert.Equal(t, 2, code) + assert.Contains(t, stderr.String(), tc.wantErr) + assert.Empty(t, stdout.String(), "a refused option must write no document") + }) + } +} diff --git a/cmd/morphic/edgecases_test.go b/cmd/morphic/edgecases_test.go index 3e3c36d7..33354754 100644 --- a/cmd/morphic/edgecases_test.go +++ b/cmd/morphic/edgecases_test.go @@ -89,6 +89,12 @@ func (nilDocCompiler) Formats() []compilers.SourceFormat { return []compilers.SourceFormat{{Name: "openapi", Version: "3.1"}} } +func (nilDocCompiler) Detect(compilers.Source) (compilers.SourceFormat, bool) { + return compilers.SourceFormat{Name: "openapi", Version: "3.1"}, true +} + +func (nilDocCompiler) DecodeOptions(compilers.OptionSet) (any, error) { return nil, nil } + func (nilDocCompiler) Compile(context.Context, []compilers.Source, compilers.Options) (*ir.Document, []ir.Diagnostic, error) { return nil, []ir.Diagnostic{{ Severity: ir.SeverityError, diff --git a/cmd/morphic/options.go b/cmd/morphic/options.go new file mode 100644 index 00000000..e00c2c0b --- /dev/null +++ b/cmd/morphic/options.go @@ -0,0 +1,51 @@ +package main + +import ( + "fmt" + "maps" + "slices" + "strings" +) + +// settingFlag collects the repeated `-opt key=value` settings that configure +// whichever compiler the spec turns out to need. +// +// It carries the pairs verbatim rather than parsing them, because the CLI does +// not know which compiler will read them: the names, the accepted values and +// what counts as a file path are the compiler's own, and a CLI that validated +// any of them would be holding a second copy of a vocabulary it cannot see +// change. What is checked here is the shape every compiler's settings share — +// that a pair has both halves, and that no key was given twice. +type settingFlag map[string]string + +// String renders the settings as they were typed, sorted so the rendering does +// not depend on map order. The flag package calls it on a zero value to decide +// whether to print a default, so a nil map must render as the empty string. +func (s settingFlag) String() string { + pairs := make([]string, 0, len(s)) + for _, key := range slices.Sorted(maps.Keys(s)) { + pairs = append(pairs, key+"="+s[key]) + } + return strings.Join(pairs, " ") +} + +// Set records one key=value pair. A repeated key is refused rather than +// overwritten: one of the two values would take effect with nothing to say +// which, and a user who typed both meant something by each. +func (s settingFlag) Set(raw string) error { + if s == nil { + return fmt.Errorf("no option set to write %q into", raw) + } + key, value, ok := strings.Cut(raw, "=") + if !ok { + return fmt.Errorf("want key=value, got %q", raw) + } + if key == "" { + return fmt.Errorf("empty option name in %q", raw) + } + if _, repeated := s[key]; repeated { + return fmt.Errorf("option %q set more than once", key) + } + s[key] = value + return nil +} diff --git a/cmd/morphic/options_test.go b/cmd/morphic/options_test.go new file mode 100644 index 00000000..0dc2b3a4 --- /dev/null +++ b/cmd/morphic/options_test.go @@ -0,0 +1,64 @@ +package main + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSettingFlag_SetCollectsPairs(t *testing.T) { + t.Parallel() + got := settingFlag{} + require.NoError(t, got.Set("grouping=path-prefix")) + require.NoError(t, got.Set("overlay=patch.yaml")) + // A value may carry the separator; only the first one splits the pair. + require.NoError(t, got.Set("note=a=b")) + // An empty value is a value, not an absent setting: the compiler decides + // whether it can use one. + require.NoError(t, got.Set("quiet=")) + + assert.Equal(t, settingFlag{ + "grouping": "path-prefix", + "overlay": "patch.yaml", + "note": "a=b", + "quiet": "", + }, got) + assert.Equal(t, "grouping=path-prefix note=a=b overlay=patch.yaml quiet=", got.String()) +} + +func TestSettingFlag_StringOfNilRendersEmpty(t *testing.T) { + t.Parallel() + // The flag package renders a zero value to decide whether to print a + // default, so this is reached before any -opt is typed. + var unset settingFlag + assert.Empty(t, unset.String()) +} + +func TestSettingFlag_SetRefusals(t *testing.T) { + t.Parallel() + tests := []struct { + name, raw, wantErr string + }{ + {"no separator", "grouping", `want key=value, got "grouping"`}, + {"empty name", "=tags", `empty option name in "=tags"`}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + err := settingFlag{}.Set(tc.raw) + require.Error(t, err) + assert.EqualError(t, err, tc.wantErr) + }) + } + + repeated := settingFlag{} + require.NoError(t, repeated.Set("grouping=tags")) + err := repeated.Set("grouping=path-prefix") + require.Error(t, err, "one of the two values would win with nothing to say which") + assert.EqualError(t, err, `option "grouping" set more than once`) + assert.Equal(t, "tags", repeated["grouping"], "the refused value must not land") + + var unset settingFlag + assert.Error(t, unset.Set("grouping=tags"), "a nil set must refuse rather than panic") +} diff --git a/cmd/morphic/testdata/compile-help.txt b/cmd/morphic/testdata/compile-help.txt index 115d6b19..50b58dfe 100644 --- a/cmd/morphic/testdata/compile-help.txt +++ b/cmd/morphic/testdata/compile-help.txt @@ -8,6 +8,10 @@ diagnostics to stderr. type node interned there, the coordinates interned beneath it, and the diagnostics stamped at it — instead of writing the document. +--opt passes a setting to the compiler the spec selects, which names and +validates its own options; morphic itself knows none of them. The OpenAPI +compiler's are listed in the README. + flags: -explain string report what compiling produced at this source pointer instead of writing IR JSON @@ -15,5 +19,7 @@ flags: fail (exit 1) on diagnostics at or above this severity: error|warning (default "error") -o string write IR JSON to this file instead of stdout + -opt key=value + set one key=value option on the compiler the spec selects (repeatable) -skip-validate skip the referential-integrity validate pass diff --git a/compilers/compilers.go b/compilers/compilers.go index dc8401b2..6763146a 100644 --- a/compilers/compilers.go +++ b/compilers/compilers.go @@ -31,12 +31,50 @@ type Options struct { FormatOptions any } +// OptionSet is one compile's configuration as text: settings named in the +// compiler's own option vocabulary, which DecodeOptions turns into that +// compiler's FormatOptions value. +// +// It exists so a caller can configure a compiler it does not import. The CLI +// collects key=value pairs without knowing which compiler will read them, and +// the compiler names and validates every key, so no layer above holds a list of +// another format's options. +type OptionSet struct { + // Settings maps an option name to its textual value. A nil or empty map asks + // for defaults. + Settings map[string]string + // ReadFile loads a file a setting names, e.g. an overlay document. A + // compiler does no file I/O of its own — Source is the whole of its input, + // which is what keeps compilation pure and reentrant — so the caller supplies + // the reader and the read stays the caller's. A nil ReadFile means no setting + // may name a file. + ReadFile func(name string) ([]byte, error) +} + // Compiler lowers source documents into the IR. Implementations must be pure: // no package-level mutable state, no writes to stderr; spec problems are // returned as ir.Diagnostic values and the error return is reserved for // I/O-level and programmer errors. +// +// Detect and DecodeOptions are what keep the layers above format-agnostic: a +// compiler says what its own input looks like and what its own options are +// called, so registering one is the whole of adding a format. Both are required +// rather than optional interfaces on purpose — a compiler that answered neither +// would be registered and unreachable, which is a hole no caller can see. type Compiler interface { Formats() []SourceFormat + // Detect reports the format src declares, and whether this compiler + // recognizes it at all. Recognition is not support: a compiler may name a + // format it does not serve — a version it has yet to implement — so that the + // caller can say so rather than report the source as unrecognized. It must + // not report a parse failure as anything but "not mine"; the bytes of another + // format are ordinary input here, not an error. + Detect(src Source) (SourceFormat, bool) + // DecodeOptions turns textual settings into the value this compiler expects + // in Options.FormatOptions. An empty set yields defaults. An unknown key, an + // unusable value, or a file that cannot be read is an error — a setting that + // is silently ignored leaves the caller believing they configured something. + DecodeOptions(set OptionSet) (any, error) Compile(ctx context.Context, sources []Source, opts Options) (*ir.Document, []ir.Diagnostic, error) } @@ -45,6 +83,7 @@ type Compiler interface { // composes its registry explicitly. type Registry struct { byFormat map[SourceFormat]Compiler + ordered []Compiler } // NewRegistry returns an empty registry. @@ -67,9 +106,35 @@ func (r *Registry) Register(c Compiler) error { for _, format := range formats { r.byFormat[format] = c } + r.ordered = append(r.ordered, c) return nil } +// Detect asks each registered compiler in turn to recognize src, and returns +// the first that does: the compiler registered for the format it named, that +// format, and whether such a compiler exists. Nothing recognizing src is +// reported as the zero format, which is what tells "no compiler takes these +// bytes" from "this format is recognized but unsupported". +// +// The order is registration order, because it is the only order a caller +// controls — the variadic that composes the registry fixes it — and a map's +// would make two compilers that both claim a source resolve differently from +// run to run. +func (r *Registry) Detect(src Source) (Compiler, SourceFormat, bool) { + if len(src.Data) == 0 { + return nil, SourceFormat{}, false + } + for _, c := range r.ordered { + format, ok := c.Detect(src) + if !ok { + continue + } + owner, registered := r.byFormat[format] + return owner, format, registered + } + return nil, SourceFormat{}, false +} + // Lookup returns the compiler registered for format. func (r *Registry) Lookup(format SourceFormat) (Compiler, bool) { c, ok := r.byFormat[format] diff --git a/compilers/compilers_test.go b/compilers/compilers_test.go index aa2afb59..59ad7e29 100644 --- a/compilers/compilers_test.go +++ b/compilers/compilers_test.go @@ -1,6 +1,7 @@ package compilers_test import ( + "bytes" "context" "testing" @@ -11,11 +12,29 @@ import ( "github.com/dexpace/morphic/ir" ) -// stubCompiler registers under fixed formats and returns an empty document. -type stubCompiler struct{ formats []compilers.SourceFormat } +// stubCompiler registers under fixed formats and returns an empty document. It +// recognizes a source whose bytes contain its marker, so a registry holding two +// of them can be asked which one claims a given source. +type stubCompiler struct { + formats []compilers.SourceFormat + marker string + detects compilers.SourceFormat +} func (s *stubCompiler) Formats() []compilers.SourceFormat { return s.formats } +func (s *stubCompiler) Detect(src compilers.Source) (compilers.SourceFormat, bool) { + if s.marker == "" || !bytes.Contains(src.Data, []byte(s.marker)) { + return compilers.SourceFormat{}, false + } + if s.detects != (compilers.SourceFormat{}) { + return s.detects, true + } + return s.formats[0], true +} + +func (s *stubCompiler) DecodeOptions(compilers.OptionSet) (any, error) { return nil, nil } + func (s *stubCompiler) Compile(_ context.Context, _ []compilers.Source, _ compilers.Options) (*ir.Document, []ir.Diagnostic, error) { return &ir.Document{IRVersion: "0.1.0"}, nil, nil } @@ -26,7 +45,7 @@ func TestRegistry_RegisterAndLookup(t *testing.T) { oa := &stubCompiler{formats: []compilers.SourceFormat{ {Name: "openapi", Version: "3.0"}, {Name: "openapi", Version: "3.1"}, - }} + }, marker: "openapi"} require.NoError(t, reg.Register(oa)) got, ok := reg.Lookup(compilers.SourceFormat{Name: "openapi", Version: "3.1"}) @@ -60,3 +79,86 @@ func TestSourceFormat_String(t *testing.T) { t.Parallel() assert.Equal(t, "openapi@3.1", compilers.SourceFormat{Name: "openapi", Version: "3.1"}.String()) } + +// TestRegistry_DetectAsksEachCompilerInRegistrationOrder is the seam that keeps +// format knowledge out of the layers above: the registry asks, the compilers +// answer, and adding a format is a registration rather than an edit somewhere +// else. Registration order is the tie-break, so the same source resolves the +// same way on every run. +func TestRegistry_DetectAsksEachCompilerInRegistrationOrder(t *testing.T) { + t.Parallel() + first := &stubCompiler{ + formats: []compilers.SourceFormat{{Name: "alpha", Version: "1"}}, + marker: "shared", + } + second := &stubCompiler{ + formats: []compilers.SourceFormat{{Name: "beta", Version: "2"}}, + marker: "shared", + } + reg := compilers.NewRegistry() + require.NoError(t, reg.Register(first)) + require.NoError(t, reg.Register(second)) + + got, format, ok := reg.Detect(compilers.Source{Path: "s.txt", Data: []byte("shared bytes")}) + require.True(t, ok) + assert.Same(t, compilers.Compiler(first), got, "the earlier registration wins") + assert.Equal(t, compilers.SourceFormat{Name: "alpha", Version: "1"}, format) + + _, format, ok = reg.Detect(compilers.Source{Path: "s.txt", Data: []byte("nobody claims this")}) + assert.False(t, ok) + assert.Equal(t, compilers.SourceFormat{}, format, "the zero format means unrecognized") +} + +// TestRegistry_DetectReportsRecognizedButUnregistered separates the two ways a +// source can go uncompiled: nothing recognized it, or something did and named a +// format the registry does not carry. A caller that could not tell them apart +// would report a known spec dialect as an unreadable file. +func TestRegistry_DetectReportsRecognizedButUnregistered(t *testing.T) { + t.Parallel() + front := &stubCompiler{ + formats: []compilers.SourceFormat{{Name: "alpha", Version: "1"}}, + marker: "alpha", + detects: compilers.SourceFormat{Name: "alpha", Version: "9"}, + } + reg := compilers.NewRegistry() + require.NoError(t, reg.Register(front)) + + got, format, ok := reg.Detect(compilers.Source{Path: "s.txt", Data: []byte("alpha")}) + assert.False(t, ok, "no compiler is registered for alpha@9") + assert.Nil(t, got) + assert.Equal(t, compilers.SourceFormat{Name: "alpha", Version: "9"}, format) +} + +func TestRegistry_DetectEmptySource(t *testing.T) { + t.Parallel() + reg := compilers.NewRegistry() + require.NoError(t, reg.Register(&stubCompiler{ + formats: []compilers.SourceFormat{{Name: "alpha", Version: "1"}}, + marker: "alpha", + })) + + _, _, ok := reg.Detect(compilers.Source{Path: "empty.txt"}) + assert.False(t, ok, "no bytes declare no format") +} + +// TestRegistry_DetectSkipsCompilersThatDecline pins that a compiler declining a +// source does not end the search. +func TestRegistry_DetectSkipsCompilersThatDecline(t *testing.T) { + t.Parallel() + declines := &stubCompiler{ + formats: []compilers.SourceFormat{{Name: "alpha", Version: "1"}}, + marker: "alpha", + } + claims := &stubCompiler{ + formats: []compilers.SourceFormat{{Name: "beta", Version: "2"}}, + marker: "beta", + } + reg := compilers.NewRegistry() + require.NoError(t, reg.Register(declines)) + require.NoError(t, reg.Register(claims)) + + got, format, ok := reg.Detect(compilers.Source{Path: "s.txt", Data: []byte("beta")}) + require.True(t, ok) + assert.Same(t, compilers.Compiler(claims), got) + assert.Equal(t, compilers.SourceFormat{Name: "beta", Version: "2"}, format) +} diff --git a/compilers/openapi/detect.go b/compilers/openapi/detect.go new file mode 100644 index 00000000..25c07f96 --- /dev/null +++ b/compilers/openapi/detect.go @@ -0,0 +1,155 @@ +package openapi + +import ( + "bytes" + "encoding/json" + + yaml "gopkg.in/yaml.v3" + + "github.com/dexpace/morphic/compilers" +) + +// maxSniffBytes bounds the bytes Detect parses. Detection reads two top-level +// keys, and 64 KiB reaches them in any document a person wrote, so the cost of +// asking stays flat while spec size does not: a full parse of a 10 MB document +// costs hundreds of milliseconds before the compiler's own parse begins. +const maxSniffBytes = 64 << 10 + +// maxSniffEntries bounds the top-level entries read from a flow-style prefix. +// The keys being looked for are declared among a document's first few, and a +// prefix full of nothing else is not one this compiler will take. +const maxSniffEntries = 512 + +// sniffProbe holds the two discriminating top-level keys. Which one is present +// is the whole of the format question: an OpenAPI 3.x document declares +// `openapi`, a Swagger 2.0 document declares `swagger`. +type sniffProbe struct { + OpenAPI string `yaml:"openapi"` + Swagger string `yaml:"swagger"` +} + +// Detect implements compilers.Compiler. It reports the dialect src declares, +// keyed by the major.minor prefix of the version string. +// +// It names swagger@2.0 as well, which this compiler does not serve: a Swagger +// document is recognizably an API spec, and reporting it as one lets the caller +// say the format is unsupported rather than that the file is unreadable. The +// path is not consulted — an OpenAPI document is what it declares itself to be, +// under any extension. +func (*Compiler) Detect(src compilers.Source) (compilers.SourceFormat, bool) { + probe := sniff(src.Data) + switch { + case probe.OpenAPI != "": + return compilers.SourceFormat{Name: "openapi", Version: majorMinor(probe.OpenAPI)}, true + case probe.Swagger != "": + return compilers.SourceFormat{Name: "swagger", Version: majorMinor(probe.Swagger)}, true + default: + return compilers.SourceFormat{}, false + } +} + +// sniff reads the discriminating keys out of at most maxSniffBytes bytes, and +// reports the zero probe for anything it cannot read — bytes of another format +// are ordinary input here, and a parser's complaint about them answers no +// question a caller asked. +// +// A document within the cap is decoded whole and exactly. A larger one is +// decoded from a prefix, which cannot simply be cut: flow style — JSON is the +// common case — is one token stream with no line structure, so its entries are +// streamed instead, and block style is cut at its last complete line. +func sniff(data []byte) sniffProbe { + if len(data) <= maxSniffBytes { + return decodeYAML(data) + } + prefix := data[:maxSniffBytes] + if probe, ok := decodeFlowPrefix(prefix); ok { + return probe + } + return decodeYAML(wholeLines(prefix)) +} + +// decodeYAML reads the probe keys from a complete YAML (or JSON, its subset) +// document. +func decodeYAML(data []byte) sniffProbe { + var probe sniffProbe + if err := yaml.Unmarshal(data, &probe); err != nil { + return sniffProbe{} + } + return probe +} + +// decodeFlowPrefix reads the top-level entries of a prefix that opens a flow +// mapping, and reports whether it was one. The JSON decoder is used because it +// streams: a prefix cut mid-document still yields every entry it completed, +// where decoding those same bytes whole reports only that they end early. +func decodeFlowPrefix(prefix []byte) (sniffProbe, bool) { + dec := json.NewDecoder(bytes.NewReader(prefix)) + tok, err := dec.Token() + if err != nil || tok != json.Delim('{') { + return sniffProbe{}, false + } + + var probe sniffProbe + for range maxSniffEntries { + key, err := dec.Token() + if err != nil { + break + } + var value json.RawMessage + if err := dec.Decode(&value); err != nil { + break + } + recordEntry(&probe, key, value) + } + return probe, true +} + +// recordEntry stores value under probe's field for key. key is compared as read +// rather than asserted to a string: the closing delimiter of the mapping +// arrives here too, and it matches neither name. +func recordEntry(probe *sniffProbe, key json.Token, value json.RawMessage) { + switch key { + case "openapi": + probe.OpenAPI = jsonString(value) + case "swagger": + probe.Swagger = jsonString(value) + } +} + +// jsonString returns value as a string, or "" for any other shape. A version +// that is not a string does not declare a dialect. +func jsonString(value json.RawMessage) string { + var out string + if err := json.Unmarshal(value, &out); err != nil { + return "" + } + return out +} + +// wholeLines returns prefix up to and including its last newline, so a block +// document is cut between entries rather than inside one. A prefix with no +// newline in it is returned as it is; there is no better cut to make. +func wholeLines(prefix []byte) []byte { + if i := bytes.LastIndexByte(prefix, '\n'); i >= 0 { + return prefix[:i+1] + } + return prefix +} + +// majorMinor returns the "major.minor" prefix of a dotted version string, +// e.g. "3.1.0" → "3.1". Strings with fewer than two dots — a bare major +// version, or a version already in major.minor form — are returned unchanged. +func majorMinor(version string) string { + firstDot := -1 + for i := range len(version) { + if version[i] != '.' { + continue + } + if firstDot < 0 { + firstDot = i + continue + } + return version[:i] + } + return version +} diff --git a/compilers/openapi/detect_test.go b/compilers/openapi/detect_test.go new file mode 100644 index 00000000..0d7f9c1e --- /dev/null +++ b/compilers/openapi/detect_test.go @@ -0,0 +1,181 @@ +package openapi + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/compilers" +) + +func TestDetect_Formats(t *testing.T) { + t.Parallel() + cases := []struct { + name, path, src string + want compilers.SourceFormat + wantOK bool + }{ + {"openapi 3.1 yaml", "api.yaml", "openapi: 3.1.0\ninfo: {}\n", + compilers.SourceFormat{Name: "openapi", Version: "3.1"}, true}, + {"openapi 3.0 json", "api.json", `{"openapi": "3.0.3"}`, + compilers.SourceFormat{Name: "openapi", Version: "3.0"}, true}, + {"openapi 3.2 yaml", "api.yaml", "openapi: 3.2.0\ninfo: {}\n", + compilers.SourceFormat{Name: "openapi", Version: "3.2"}, true}, + // A version already in major.minor form (single dot) exercises + // majorMinor's unchanged-passthrough return. + {"openapi major.minor only", "api.yaml", "openapi: \"3.1\"\n", + compilers.SourceFormat{Name: "openapi", Version: "3.1"}, true}, + // A bare-major version (no dot) also reaches the passthrough return. + {"openapi bare major", "api.yaml", "openapi: \"4\"\n", + compilers.SourceFormat{Name: "openapi", Version: "4"}, true}, + // Recognized, and deliberately not a format Formats reports: naming it + // lets the caller say "unsupported" rather than "unreadable". + {"swagger", "api.yaml", "swagger: \"2.0\"\n", + compilers.SourceFormat{Name: "swagger", Version: "2.0"}, true}, + + // Everything below is another format's input, and none of it is an error + // here: three of the five planned compilers take bytes that are not YAML. + {"protobuf", "svc.proto", "syntax = \"proto3\";\nservice S { rpc Get (Q) returns (A); }\n", + compilers.SourceFormat{}, false}, + {"typespec", "main.tsp", "import \"@typespec/http\";\nmodel Pet { name: string; }\n", + compilers.SourceFormat{}, false}, + {"graphql", "s.graphql", "type Query {\n pet(id: ID!): Pet\n}\n", + compilers.SourceFormat{}, false}, + // Valid YAML that declares neither key: whether a source parses is not + // the question detection answers. + {"graphql that parses as yaml", "s.graphql", "type Query { a: String }\n", + compilers.SourceFormat{}, false}, + {"yaml that is no spec", "junk.yaml", "hello: world\n", + compilers.SourceFormat{}, false}, + {"unparseable yaml", "api.yaml", "openapi: [unterminated\n", + compilers.SourceFormat{}, false}, + {"empty", "empty.yaml", "", compilers.SourceFormat{}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, ok := New().Detect(compilers.Source{Path: tc.path, Data: []byte(tc.src)}) + assert.Equal(t, tc.wantOK, ok) + assert.Equal(t, tc.want, got) + }) + } +} + +// padTo returns src grown past the sniff cap by appending filler, so sniff takes +// its bounded-prefix path rather than decoding the source whole. +func padTo(src, filler string) string { + var b strings.Builder + b.WriteString(src) + for b.Len() <= maxSniffBytes { + b.WriteString(filler) + } + return b.String() +} + +// TestSniff_BeyondTheCap pins what the bound buys and what it costs. A document +// larger than the cap is read from its first maxSniffBytes in whichever style it +// is written, and a declaration past that point is not seen — detection stays +// flat in document size rather than paying a full parse to read two keys. +func TestSniff_BeyondTheCap(t *testing.T) { + t.Parallel() + const filler = "# a line of padding that says nothing about the format\n" + cases := []struct { + name, src string + want sniffProbe + }{ + {"block yaml declaring first", + padTo("openapi: 3.1.0\n", filler), sniffProbe{OpenAPI: "3.1.0"}}, + {"block yaml declaring past the cap", + padTo("", filler) + "openapi: 3.1.0\n", sniffProbe{}}, + {"flow json declaring first", + `{"openapi":"3.1.0","x":"` + strings.Repeat("p", maxSniffBytes) + `"}`, + sniffProbe{OpenAPI: "3.1.0"}}, + {"flow json declaring past the cap", + `{"x":"` + strings.Repeat("p", maxSniffBytes) + `","openapi":"3.1.0"}`, + sniffProbe{}}, + {"flow json swagger first", + `{"swagger":"2.0","x":"` + strings.Repeat("p", maxSniffBytes) + `"}`, + sniffProbe{Swagger: "2.0"}}, + // Neither YAML nor JSON, and larger than the cap: the prefix is parsed, + // fails, and the answer is silence rather than a parser's complaint. + {"protobuf past the cap", + padTo("syntax = \"proto3\";\n", "message M { string a = 1; }\n"), sniffProbe{}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + require.Greater(t, len(tc.src), maxSniffBytes, "the case must exceed the cap to test it") + assert.Equal(t, tc.want, sniff([]byte(tc.src))) + }) + } +} + +func TestDecodeFlowPrefix_ReadsWhatTheCutLeft(t *testing.T) { + t.Parallel() + cases := []struct { + name, prefix string + want sniffProbe + wantFlow bool + }{ + {"complete document", `{"openapi":"3.1.0","info":{"title":"T"}}`, + sniffProbe{OpenAPI: "3.1.0"}, true}, + {"cut inside a later value", `{"openapi":"3.1.0","info":{"title":"T`, + sniffProbe{OpenAPI: "3.1.0"}, true}, + {"cut inside a key", `{"openapi":"3.1.0","inf`, + sniffProbe{OpenAPI: "3.1.0"}, true}, + {"swagger", `{"swagger":"2.0","info":{}}`, sniffProbe{Swagger: "2.0"}, true}, + // A version that is not a string declares no dialect, and must not be + // read as one by accident. + {"non-string version", `{"openapi":3}`, sniffProbe{}, true}, + {"no flow mapping", "openapi: 3.1.0\n", sniffProbe{}, false}, + {"not even a token", "\x00", sniffProbe{}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, flow := decodeFlowPrefix([]byte(tc.prefix)) + assert.Equal(t, tc.wantFlow, flow) + assert.Equal(t, tc.want, got) + }) + } +} + +// TestDecodeFlowPrefix_StopsAtTheEntryCap proves the walk is bounded by its own +// count and not only by the byte cap: a declaration after maxSniffEntries other +// entries is not read. +func TestDecodeFlowPrefix_StopsAtTheEntryCap(t *testing.T) { + t.Parallel() + var b strings.Builder + b.WriteByte('{') + for i := range maxSniffEntries + 1 { + if i > 0 { + b.WriteByte(',') + } + b.WriteString(`"k`) + b.WriteString(strings.Repeat("x", 3)) + b.WriteString(string(rune('a' + i%26))) + b.WriteString(strings.Repeat("y", i%7)) + b.WriteString(`":0`) + } + b.WriteString(`,"openapi":"3.1.0"}`) + + got, flow := decodeFlowPrefix([]byte(b.String())) + require.True(t, flow) + assert.Equal(t, sniffProbe{}, got, "the entry past the cap is not read") +} + +func TestWholeLines(t *testing.T) { + t.Parallel() + assert.Equal(t, "a\nb\n", string(wholeLines([]byte("a\nb\nc")))) + assert.Equal(t, "nolines", string(wholeLines([]byte("nolines"))), + "a prefix with no newline has no better cut to make") +} + +func TestMajorMinor(t *testing.T) { + t.Parallel() + assert.Equal(t, "3.1", majorMinor("3.1.0")) + assert.Equal(t, "3.1", majorMinor("3.1")) + assert.Equal(t, "4", majorMinor("4")) +} diff --git a/compilers/openapi/doc.go b/compilers/openapi/doc.go index f3764aad..96933bd4 100644 --- a/compilers/openapi/doc.go +++ b/compilers/openapi/doc.go @@ -2,8 +2,10 @@ // implements compilers.Compiler. // // What is here is the compiler's public face and the assembly behind it: the -// Compiler, its Options, the document metadata, and the run that calls the -// lowerings in order and builds a Document out of what they return. +// Compiler, its Options and the vocabulary they answer to as text, the +// recognition of an OpenAPI document from its own bytes, the document metadata, +// and the run that calls the lowerings in order and builds a Document out of +// what they return. // // Parsing is delegated to github.com/speakeasy-api/openapi. Everything the // compiler itself decides — identity (pointer-derived IDs), hoisting, diff --git a/compilers/openapi/options.go b/compilers/openapi/options.go index 35c6b812..3ba06e62 100644 --- a/compilers/openapi/options.go +++ b/compilers/openapi/options.go @@ -1,6 +1,15 @@ package openapi -import "github.com/dexpace/morphic/compilers/openapi/internal/lowering" +import ( + "fmt" + "maps" + "slices" + "strconv" + "strings" + + "github.com/dexpace/morphic/compilers" + "github.com/dexpace/morphic/compilers/openapi/internal/lowering" +) // GroupingStrategy selects how operations are grouped into OperationGroups. It // is the injectable-policy seam (architecture principle 6): grouping is inferred @@ -57,10 +66,9 @@ type Options struct { // The document arrives as bytes, like the spec itself, because a compiler // performs no file I/O — reading it is the caller's job, which is what keeps // compilation pure and reentrant. A programmatic caller sets this through -// engine.RunOptions.FormatOptions, which the engine forwards verbatim; no -// morphic subcommand surfaces it yet, and the CLI exposes no other member of -// this type either, so adding a flag for one is a change about the CLI's option -// surface rather than about overlays. +// engine.RunOptions.FormatOptions, which the engine forwards verbatim; a caller +// who has only text names the file with the "overlay" setting and the reader in +// compilers.OptionSet loads it, so the read is still the caller's. // // An applied overlay becomes a second entry in Document.Sources, and every // position it introduced or rewrote names that entry as its Provenance.Source. @@ -89,3 +97,113 @@ func (o Options) withDefaults() Options { } return o } + +// The option names Options answers to as text. They are this compiler's own +// vocabulary: a caller writes them without knowing which compiler reads them, +// and nothing above this package holds the list. +const ( + optGrouping = "grouping" + optAllowExternalRefs = "allow-external-refs" + optOverlay = "overlay" + optOverlayLax = "overlay-lax" +) + +// optionNames lists the vocabulary, sorted, for the error that reports a name +// outside it. Deriving the list from one place is what keeps the error honest +// as the vocabulary grows. +func optionNames() []string { + names := []string{optGrouping, optAllowExternalRefs, optOverlay, optOverlayLax} + slices.Sort(names) + return names +} + +// DecodeOptions implements compilers.Compiler: it turns textual settings into +// an Options value. Settings are read in sorted order so that a set with two +// bad values always reports the same one. +func (*Compiler) DecodeOptions(set compilers.OptionSet) (any, error) { + var decode optionDecode + for _, key := range slices.Sorted(maps.Keys(set.Settings)) { + if err := decode.set(key, set.Settings[key]); err != nil { + return nil, err + } + } + opts, err := decode.finish(set.ReadFile) + if err != nil { + return nil, err + } + return opts, nil +} + +// optionDecode is an Options under construction. The overlay is described by two +// settings — the document and how strictly to apply it — so it is assembled once +// every setting has been read, and neither order of the two changes the result. +type optionDecode struct { + opts Options + overlayPath string + lax bool + laxSet bool +} + +// set reads one setting. +func (d *optionDecode) set(key, value string) error { + switch key { + case optGrouping: + return decodeGrouping(&d.opts.Grouping, value) + case optAllowExternalRefs: + return decodeBool(&d.opts.AllowExternalRefs, key, value) + case optOverlay: + d.overlayPath = value + return nil + case optOverlayLax: + d.laxSet = true + return decodeBool(&d.lax, key, value) + default: + return fmt.Errorf("openapi: unknown option %q (known: %s)", + key, strings.Join(optionNames(), ", ")) + } +} + +// finish assembles the decoded Options, loading the overlay document a setting +// named through the caller's reader. +func (d *optionDecode) finish(readFile func(name string) ([]byte, error)) (Options, error) { + if d.overlayPath == "" { + if d.laxSet { + return Options{}, fmt.Errorf("openapi: option %q applies only with %q", + optOverlayLax, optOverlay) + } + return d.opts, nil + } + if readFile == nil { + return Options{}, fmt.Errorf("openapi: option %q names a file and the caller supplied no reader", + optOverlay) + } + data, err := readFile(d.overlayPath) + if err != nil { + return Options{}, fmt.Errorf("openapi: option %q: %w", optOverlay, err) + } + d.opts.Overlay = &Overlay{Path: d.overlayPath, Data: data, Lax: d.lax} + return d.opts, nil +} + +// decodeGrouping reads a grouping strategy by its own name, which is the same +// string the JSON form uses. +func decodeGrouping(out *GroupingStrategy, value string) error { + switch strategy := GroupingStrategy(value); strategy { + case GroupByTags, GroupByPathPrefix: + *out = strategy + return nil + default: + return fmt.Errorf("openapi: option %q: want %q or %q, got %q", + optGrouping, GroupByTags, GroupByPathPrefix, value) + } +} + +// decodeBool reads a boolean setting, spelled as strconv.ParseBool accepts it. +func decodeBool(out *bool, key, value string) error { + parsed, err := strconv.ParseBool(value) + if err != nil { + return fmt.Errorf("openapi: option %q: want a boolean, got %q", key, value) + } + *out = parsed + return nil +} diff --git a/compilers/openapi/options_test.go b/compilers/openapi/options_test.go index 0895b078..0df2366f 100644 --- a/compilers/openapi/options_test.go +++ b/compilers/openapi/options_test.go @@ -2,6 +2,8 @@ package openapi import ( "context" + "errors" + "fmt" "testing" "github.com/stretchr/testify/assert" @@ -27,3 +29,124 @@ func TestParse_ExplicitOptions(t *testing.T) { require.NotNil(t, doc) assert.Equal(t, "a", doc.Services[0].Groups[0].Name.Source) } + +// TestDecodeOptions_TextReachesEveryField is the other half of #69's answer: a +// caller holding only text — the CLI, which imports no compiler — can set every +// option a Go caller can, including the overlay, whose value is a file the +// caller's own reader loads. +func TestDecodeOptions_TextReachesEveryField(t *testing.T) { + t.Parallel() + const overlayDoc = "overlay: 1.0.0\ninfo: {title: O, version: \"1\"}\nactions: []\n" + read := func(name string) ([]byte, error) { + if name != "patch.yaml" { + return nil, fmt.Errorf("no such file %q", name) + } + return []byte(overlayDoc), nil + } + + got, err := New().DecodeOptions(compilers.OptionSet{ + Settings: map[string]string{ + "grouping": "path-prefix", + "allow-external-refs": "true", + "overlay": "patch.yaml", + "overlay-lax": "true", + }, + ReadFile: read, + }) + require.NoError(t, err) + + opts, ok := got.(Options) + require.True(t, ok, "the decoded value must be this compiler's own options type") + assert.Equal(t, GroupByPathPrefix, opts.Grouping) + assert.True(t, opts.AllowExternalRefs) + require.NotNil(t, opts.Overlay) + assert.Equal(t, "patch.yaml", opts.Overlay.Path) + assert.Equal(t, overlayDoc, string(opts.Overlay.Data)) + assert.True(t, opts.Overlay.Lax) +} + +func TestDecodeOptions_EmptySetIsDefaults(t *testing.T) { + t.Parallel() + got, err := New().DecodeOptions(compilers.OptionSet{}) + require.NoError(t, err) + assert.Equal(t, Options{}, got) +} + +func TestDecodeOptions_Refusals(t *testing.T) { + t.Parallel() + readsNothing := func(string) ([]byte, error) { return nil, errors.New("no such file") } + tests := []struct { + name string + settings map[string]string + readFile func(string) ([]byte, error) + wantErr string + }{ + {"unknown name", map[string]string{"gruoping": "tags"}, nil, + `unknown option "gruoping" (known: allow-external-refs, grouping, overlay, overlay-lax)`}, + {"unknown grouping", map[string]string{"grouping": "alphabetical"}, nil, + `want "tags" or "path-prefix", got "alphabetical"`}, + {"non-boolean", map[string]string{"allow-external-refs": "yes please"}, nil, + `want a boolean, got "yes please"`}, + {"non-boolean laxness", map[string]string{"overlay": "p.yaml", "overlay-lax": "sort of"}, + readsNothing, `want a boolean, got "sort of"`}, + {"laxness with no overlay", map[string]string{"overlay-lax": "true"}, nil, + `"overlay-lax" applies only with "overlay"`}, + {"no reader for a file", map[string]string{"overlay": "p.yaml"}, nil, + "names a file and the caller supplied no reader"}, + {"unreadable file", map[string]string{"overlay": "p.yaml"}, readsNothing, + "no such file"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := New().DecodeOptions(compilers.OptionSet{ + Settings: tc.settings, + ReadFile: tc.readFile, + }) + require.Error(t, err, "a setting that cannot be honoured must not be ignored") + assert.Contains(t, err.Error(), tc.wantErr) + assert.Nil(t, got) + }) + } +} + +// TestDecodeOptions_OverlayLaxnessIsOrderIndependent pins that the two settings +// describing one overlay are assembled after both have been read: were they +// applied as they arrived, the laxness would depend on which name sorted first. +func TestDecodeOptions_OverlayLaxnessIsOrderIndependent(t *testing.T) { + t.Parallel() + read := func(string) ([]byte, error) { return []byte("overlay: 1.0.0\n"), nil } + + strict, err := New().DecodeOptions(compilers.OptionSet{ + Settings: map[string]string{"overlay": "p.yaml", "overlay-lax": "false"}, + ReadFile: read, + }) + require.NoError(t, err) + lax, err := New().DecodeOptions(compilers.OptionSet{ + Settings: map[string]string{"overlay-lax": "true", "overlay": "p.yaml"}, + ReadFile: read, + }) + require.NoError(t, err) + + assert.False(t, strict.(Options).Overlay.Lax) + assert.True(t, lax.(Options).Overlay.Lax) +} + +// TestDecodeOptions_FeedsCompile closes the loop the CLI walks: what +// DecodeOptions returns is what Compile accepts, so a setting is not merely +// parsed but read by the lowering. +func TestDecodeOptions_FeedsCompile(t *testing.T) { + t.Parallel() + spec := "openapi: 3.1.0\ninfo: {title: T, version: \"1\"}\npaths:\n /a/b:\n get: {operationId: ab, tags: [zoo], responses: {\"200\": {description: ok}}}\n" + formatOpts, err := New().DecodeOptions(compilers.OptionSet{ + Settings: map[string]string{"grouping": "path-prefix"}, + }) + require.NoError(t, err) + + doc, _, err := New().Compile(context.Background(), []compilers.Source{sourceOf(spec)}, + compilers.Options{FormatOptions: formatOpts}) + require.NoError(t, err) + require.NotNil(t, doc) + assert.Equal(t, "a", doc.Services[0].Groups[0].Name.Source, + "the decoded option must reach the lowering, not merely parse") +} diff --git a/docs/architecture.md b/docs/architecture.md index 2fe2ac37..03319666 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -97,9 +97,12 @@ framework that writes the type registry, derives a canonical name, or builds an Promoting something into the framework later is additive, while demoting it breaks every compiler, so borderline machinery starts outside and moves in on evidence from more than one format. -Compilers are registered in a registry keyed by detected format; the engine sniffs the source -format and dispatches. Milestone 1 ships the OpenAPI 3.x compiler only; the compiler registry, -provenance model, and IR are built for all eight from day one. +Compilers are registered in a registry keyed by the formats they report, and detection belongs to +them too: the registry asks each compiler in registration order whether it recognizes a source, and +the engine dispatches to the one that does. A compiler also decodes its own textual options, so +registering one is the whole of adding a format — no layer above names any of them. Milestone 1 +ships the OpenAPI 3.x compiler only; the compiler registry, provenance model, and IR are built for +all eight from day one. ### 2.2 IR passes (IR → IR) @@ -231,7 +234,7 @@ morphic/ │ ├── typespec/ smithy/ graphql/ asyncapi/ protobuf/ otp/ (future) ├── pass/ # Layer 1 — IR → IR passes (validate, dedup, filter, slice, overlay). ├── emitters/ # Layer 2 — emitter contract, plan layer, registry (future). -├── engine/ # Layer 3 — orchestration: sniff format, run compiler, passes, emitters. +├── engine/ # Layer 3 — orchestration: detect format, run compiler, passes, emitters. ├── internal/archtest/ # Layering, grammar, recursion and method-cap rules (tooling). ├── internal/harness/ # Bug-catching oracle sweep over a spec corpus (tooling). ├── internal/testspec/ # Spec fixtures shared by the tooling (tooling). diff --git a/engine/doc.go b/engine/doc.go index 835ea202..6215f09c 100644 --- a/engine/doc.go +++ b/engine/doc.go @@ -1,4 +1,6 @@ -// Package engine orchestrates the Morphic pipeline: it sniffs the source format, -// dispatches to the registered compiler, and runs IR passes. It is the only -// package that composes compilers and passes together. +// Package engine orchestrates the Morphic pipeline: it asks the registered +// compilers which of them recognizes the source, dispatches to that one, and +// runs IR passes. It is the only package that composes compilers and passes +// together, and it holds no knowledge of any source format — what a spec looks +// like and what its options are called are the compiler's to answer. package engine diff --git a/engine/engine.go b/engine/engine.go index 2885cf07..1c749b41 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -2,6 +2,7 @@ package engine import ( "context" + "errors" "fmt" "os" @@ -11,13 +12,26 @@ import ( "github.com/dexpace/morphic/pass" ) -// RunOptions configures a single pipeline run. FormatOptions is forwarded -// verbatim to the compiler as compilers.Options.FormatOptions. +// RunOptions configures a single pipeline run. +// +// Compiler options arrive by one of two channels. FormatOptions is the +// programmatic one: a value of the compiler's own options type, forwarded +// verbatim as compilers.Options.FormatOptions by a caller that imports the +// compiler. CompilerOptions is the textual one, for a caller — the CLI — that +// does not: the detected compiler decodes the settings into its own options +// type itself. type RunOptions struct { - FormatOptions any `json:"formatOptions,omitempty"` - SkipValidate bool `json:"skipValidate,omitempty"` + FormatOptions any `json:"formatOptions,omitempty"` + CompilerOptions map[string]string `json:"compilerOptions,omitempty"` + SkipValidate bool `json:"skipValidate,omitempty"` } +// errOptionChannels reports both option channels set at once. Which one wins +// would be a precedence rule no caller can see the effect of, and a run +// configured two ways is a mistake in the caller rather than a case to resolve. +var errOptionChannels = errors.New( + "engine: set FormatOptions or CompilerOptions, not both") + // Result is the outcome of a pipeline run. A nil Document alongside diagnostics // is a legal outcome (e.g. an unsupported spec version); the caller decides what // is fatal. @@ -27,8 +41,8 @@ type Result struct { Format compilers.SourceFormat `json:"format"` } -// Engine orchestrates the sniff → compiler → passes pipeline over a registry of -// compilers. +// Engine orchestrates the detect → compiler → passes pipeline over a registry +// of compilers. type Engine struct { registry *compilers.Registry } @@ -44,7 +58,7 @@ func New() (*Engine, error) { // register failure (a compiler reporting no formats, or two compilers claiming // the same format) surfaces as a Go error rather than a panic. Calling // NewWith with no compilers is legal: the resulting engine's Run always fails -// at the lookup step, which is the seam TestEngine_RunLookupMiss relies on to +// at detection, which is the seam TestEngine_RunNothingRegistered relies on to // reach that branch. func NewWith(fronts ...compilers.Compiler) (*Engine, error) { reg := compilers.NewRegistry() @@ -56,26 +70,28 @@ func NewWith(fronts ...compilers.Compiler) (*Engine, error) { return &Engine{registry: reg}, nil } -// Run executes the pipeline for the spec at specPath: read the file, sniff its -// format, dispatch to the matching compiler, and — unless disabled — append the -// validate pass's diagnostics. The Go error return is reserved for I/O and -// programmer errors; spec problems surface as diagnostics in the Result. +// Run executes the pipeline for the spec at specPath: read the file, ask the +// registered compilers which of them recognizes it, dispatch to that one, and — +// unless disabled — append the validate pass's diagnostics. The Go error return +// is reserved for I/O and programmer errors; spec problems surface as +// diagnostics in the Result. func (e *Engine) Run(ctx context.Context, specPath string, opts RunOptions) (*Result, error) { data, err := os.ReadFile(specPath) if err != nil { return nil, fmt.Errorf("engine: read spec %q: %w", specPath, err) } - format, err := Sniff(data) - if err != nil { - return nil, fmt.Errorf("engine: sniff %q: %w", specPath, err) - } - front, ok := e.registry.Lookup(format) + source := compilers.Source{Path: specPath, Data: data} + + front, format, ok := e.registry.Detect(source) if !ok { - return nil, fmt.Errorf("engine: no compiler registered for format %s", format) + return nil, unsupportedFormat(specPath, format) } - doc, diags, err := front.Compile(ctx, - []compilers.Source{{Path: specPath, Data: data}}, - compilers.Options{FormatOptions: opts.FormatOptions}) + formatOpts, err := formatOptions(front, opts) + if err != nil { + return nil, fmt.Errorf("engine: options for %q: %w", specPath, err) + } + doc, diags, err := front.Compile(ctx, []compilers.Source{source}, + compilers.Options{FormatOptions: formatOpts}) if err != nil { return nil, fmt.Errorf("engine: parse %q: %w", specPath, err) } @@ -88,3 +104,37 @@ func (e *Engine) Run(ctx context.Context, specPath string, opts RunOptions) (*Re } return &Result{Document: doc, Diagnostics: diags, Format: format}, nil } + +// unsupportedFormat reports a source no registered compiler will take. The two +// cases are worth telling apart: a zero format means nothing recognized the +// bytes at all, while a named one means a compiler read the source and named a +// format this build does not carry — a Swagger 2.0 document, say, which is a +// spec morphic understands the shape of and does not yet compile. +func unsupportedFormat(specPath string, format compilers.SourceFormat) error { + if format.Name == "" { + return fmt.Errorf("engine: no compiler recognizes %q as a supported spec format", specPath) + } + return fmt.Errorf("engine: no compiler registered for format %s (%q)", format, specPath) +} + +// formatOptions resolves the compiler options for one run, decoding the textual +// channel through the compiler that will read them. os.ReadFile is what makes a +// path-valued setting work: the engine already reads the spec, so loading a file +// a setting names keeps the I/O on this side of the contract and the compiler +// pure. +func formatOptions(front compilers.Compiler, opts RunOptions) (any, error) { + if len(opts.CompilerOptions) == 0 { + return opts.FormatOptions, nil + } + if opts.FormatOptions != nil { + return nil, errOptionChannels + } + decoded, err := front.DecodeOptions(compilers.OptionSet{ + Settings: opts.CompilerOptions, + ReadFile: os.ReadFile, + }) + if err != nil { + return nil, fmt.Errorf("decode: %w", err) + } + return decoded, nil +} diff --git a/engine/engine_test.go b/engine/engine_test.go index fd7fe6c5..ddcfc29d 100644 --- a/engine/engine_test.go +++ b/engine/engine_test.go @@ -2,8 +2,10 @@ package engine_test import ( "context" + "errors" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -17,11 +19,48 @@ import ( func writeSpec(t *testing.T, contents string) string { t.Helper() - path := filepath.Join(t.TempDir(), "spec.yaml") + return writeNamed(t, "spec.yaml", contents) +} + +func writeNamed(t *testing.T, name, contents string) string { + t.Helper() + path := filepath.Join(t.TempDir(), name) require.NoError(t, os.WriteFile(path, []byte(contents), 0o644)) return path } +// TestEngine_RunUnrecognizedFormat drives the input three of the five planned +// compilers take. None of it is YAML, and none of it is any concern of the YAML +// parser: what a user needs back is that no registered compiler claims these +// bytes, named in the vocabulary of spec formats. +// +// Running the parser first made the answer an accident of whether the bytes +// happened to parse — a one-line GraphQL document parses and a two-line one does +// not — and when they did not, the message quoted a decoder's complaint about an +// engine-internal type. +func TestEngine_RunUnrecognizedFormat(t *testing.T) { + t.Parallel() + cases := []struct{ name, file, src string }{ + {"protobuf", "svc.proto", "syntax = \"proto3\";\npackage demo;\nservice S { rpc Get (Q) returns (A); }\n"}, + {"typespec", "main.tsp", "import \"@typespec/http\";\nnamespace Demo;\nmodel Pet { name: string; }\n"}, + {"graphql", "schema.graphql", "type Query {\n pet(id: ID!): Pet\n}\n"}, + {"graphql one line", "one.graphql", "type Query { a: String }\n"}, + {"yaml that is no spec", "junk.yaml", "hello: world\n"}, + } + eng, err := engine.New() + require.NoError(t, err) + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + _, err := eng.Run(t.Context(), writeNamed(t, tc.file, tc.src), engine.RunOptions{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "no compiler recognizes") + assert.NotContains(t, err.Error(), "yaml:", "a parser's complaint is not an answer") + }) + } +} + func TestEngine_RunEndToEnd(t *testing.T) { t.Parallel() eng, err := engine.New() @@ -97,37 +136,64 @@ func TestEngine_RunMissingFile(t *testing.T) { require.Error(t, err) } -func TestEngine_RunSniffError(t *testing.T) { +// TestEngine_RunRecognizedButUnsupported pins the other half of the detection +// answer: a Swagger 2.0 document is a spec, and the OpenAPI compiler says so +// while serving no such format. Naming it beats reporting the file as +// unrecognized, which is what a user would be told if recognition and support +// were the same question. +func TestEngine_RunRecognizedButUnsupported(t *testing.T) { t.Parallel() eng, err := engine.New() require.NoError(t, err) - // Swagger 2.0 sniffs to a recognized-but-unsupported error, which Run wraps. - _, err = eng.Run(t.Context(), writeSpec(t, "swagger: \"2.0\"\n"), engine.RunOptions{}) + _, err = eng.Run(t.Context(), writeSpec(t, "swagger: \"2.0\"\ninfo: {}\n"), engine.RunOptions{}) require.Error(t, err) - assert.Contains(t, err.Error(), "engine: sniff") + assert.Contains(t, err.Error(), "no compiler registered for format swagger@2.0") } -func TestEngine_RunLookupMiss(t *testing.T) { +func TestEngine_RunNothingRegistered(t *testing.T) { t.Parallel() // NewWith() with zero compilers is load-bearing here: it is the only way to - // reach an engine with no compiler registered for the openapi 3.1 the spec - // sniffs to. Don't add a len(fronts) == 0 precondition to NewWith — doing so - // would make this branch unreachable. + // reach an engine that has nothing to ask about a source every registered + // compiler would otherwise claim. Don't add a len(fronts) == 0 precondition + // to NewWith — doing so would make this branch unreachable. eng, err := engine.NewWith() require.NoError(t, err) _, err = eng.Run(t.Context(), writeSpec(t, testspec.Tiny), engine.RunOptions{}) require.Error(t, err) - assert.Contains(t, err.Error(), "no compiler registered for format") + assert.Contains(t, err.Error(), "no compiler recognizes") } -// collidingCompiler claims a single fixed format. Two of them registered -// together make the second Register call fail, driving NewWith's error path. -type collidingCompiler struct{} +// TestEngine_RunEmptySource pins that an empty file is nobody's spec, without a +// compiler having to be asked about zero bytes. +func TestEngine_RunEmptySource(t *testing.T) { + t.Parallel() + eng, err := engine.New() + require.NoError(t, err) + _, err = eng.Run(t.Context(), writeSpec(t, ""), engine.RunOptions{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "no compiler recognizes") +} -func (collidingCompiler) Formats() []compilers.SourceFormat { +// stubFront answers the two contract questions every stub in this file answers +// the same way: it claims openapi 3.1, recognizes anything as openapi 3.1, and +// takes no options. Embedding it leaves each stub holding only the Compile +// behaviour it exists to model. +type stubFront struct{} + +func (stubFront) Formats() []compilers.SourceFormat { return []compilers.SourceFormat{{Name: "openapi", Version: "3.1"}} } +func (stubFront) Detect(compilers.Source) (compilers.SourceFormat, bool) { + return compilers.SourceFormat{Name: "openapi", Version: "3.1"}, true +} + +func (stubFront) DecodeOptions(compilers.OptionSet) (any, error) { return nil, nil } + +// collidingCompiler claims a single fixed format. Two of them registered +// together make the second Register call fail, driving NewWith's error path. +type collidingCompiler struct{ stubFront } + func (collidingCompiler) Compile(context.Context, []compilers.Source, compilers.Options) (*ir.Document, []ir.Diagnostic, error) { return nil, nil, nil } @@ -142,11 +208,7 @@ func TestNewWith_RegisterError(t *testing.T) { // errCompiler claims openapi 3.1 and always fails Compile, driving Run's // parse-error branch. -type errCompiler struct{} - -func (errCompiler) Formats() []compilers.SourceFormat { - return []compilers.SourceFormat{{Name: "openapi", Version: "3.1"}} -} +type errCompiler struct{ stubFront } func (errCompiler) Compile(context.Context, []compilers.Source, compilers.Options) (*ir.Document, []ir.Diagnostic, error) { return nil, nil, assert.AnError @@ -163,11 +225,7 @@ func TestEngine_RunParseError(t *testing.T) { // nilDocCompiler claims openapi 3.1 and returns a nil Document with no error — // a legal outcome that must skip the validate pass and surface a nil Document. -type nilDocCompiler struct{} - -func (nilDocCompiler) Formats() []compilers.SourceFormat { - return []compilers.SourceFormat{{Name: "openapi", Version: "3.1"}} -} +type nilDocCompiler struct{ stubFront } func (nilDocCompiler) Compile(context.Context, []compilers.Source, compilers.Options) (*ir.Document, []ir.Diagnostic, error) { return nil, []ir.Diagnostic{{Code: "x/none"}}, nil @@ -188,11 +246,7 @@ func TestEngine_RunNilDocument(t *testing.T) { // dangling type reference, so the validate pass — if it runs — reports // ir/dangling-type-ref. It claims the openapi 3.1 format so a tiny 3.1 spec // sniffs to it. -type danglingCompiler struct{} - -func (danglingCompiler) Formats() []compilers.SourceFormat { - return []compilers.SourceFormat{{Name: "openapi", Version: "3.1"}} -} +type danglingCompiler struct{ stubFront } func (danglingCompiler) Compile(_ context.Context, _ []compilers.Source, _ compilers.Options) (*ir.Document, []ir.Diagnostic, error) { doc := &ir.Document{ @@ -239,3 +293,124 @@ func TestEngine_ValidateRuns(t *testing.T) { assert.False(t, hasDiagCode(withoutPass.Diagnostics, "ir/dangling-type-ref"), "skipping validation suppresses the diagnostic") } + +// smithyCompiler is a compiler for a format the engine has never heard of. It +// recognizes its own input and names its own options, which is the whole of what +// registering a format takes. +type smithyCompiler struct{ options compilers.OptionSet } + +func (*smithyCompiler) Formats() []compilers.SourceFormat { + return []compilers.SourceFormat{{Name: "smithy", Version: "2.0"}} +} + +func (*smithyCompiler) Detect(src compilers.Source) (compilers.SourceFormat, bool) { + if !strings.HasPrefix(string(src.Data), "$version:") { + return compilers.SourceFormat{}, false + } + return compilers.SourceFormat{Name: "smithy", Version: "2.0"}, true +} + +func (s *smithyCompiler) DecodeOptions(set compilers.OptionSet) (any, error) { + if _, ok := set.Settings["boom"]; ok { + return nil, errors.New("boom is not an option") + } + s.options = set + return set.Settings["shape"], nil +} + +func (*smithyCompiler) Compile(_ context.Context, _ []compilers.Source, opts compilers.Options) (*ir.Document, []ir.Diagnostic, error) { + name, _ := opts.FormatOptions.(string) + return &ir.Document{Name: name, Types: ir.TypeRegistry{}}, nil, nil +} + +// TestEngine_RunNewFormatNeedsNoEngineEdit is the acceptance criterion for a +// compiler-owned seam: a format the engine names nowhere is reached by +// registering its compiler and nothing else. Detection, the option vocabulary +// and the version grammar all belong to the compiler, so this file — and every +// other file under engine/ — mentions neither smithy nor its options. +func TestEngine_RunNewFormatNeedsNoEngineEdit(t *testing.T) { + t.Parallel() + eng, err := engine.NewWith(&smithyCompiler{}) + require.NoError(t, err) + + res, err := eng.Run(t.Context(), writeNamed(t, "model.smithy", "$version: \"2\"\nnamespace demo\n"), + engine.RunOptions{CompilerOptions: map[string]string{"shape": "Widget"}}) + require.NoError(t, err) + require.NotNil(t, res.Document) + assert.Equal(t, compilers.SourceFormat{Name: "smithy", Version: "2.0"}, res.Format) + assert.Equal(t, "Widget", res.Document.Name, "the setting reached the compiler that decoded it") +} + +// TestEngine_RunCompilerOptionsAreLoadedByTheEngine pins the file half of the +// textual channel: a compiler does no file I/O, so a setting that names one is +// read through the reader the engine supplies. +func TestEngine_RunCompilerOptionsAreLoadedByTheEngine(t *testing.T) { + t.Parallel() + front := &smithyCompiler{} + eng, err := engine.NewWith(front) + require.NoError(t, err) + spec := writeNamed(t, "model.smithy", "$version: \"2\"\n") + + _, err = eng.Run(t.Context(), spec, engine.RunOptions{ + CompilerOptions: map[string]string{"shape": "Widget"}, + }) + require.NoError(t, err) + require.NotNil(t, front.options.ReadFile, "a compiler must be handed a reader it can use") + + got, err := front.options.ReadFile(spec) + require.NoError(t, err) + assert.Contains(t, string(got), "$version") +} + +func TestEngine_RunOptionRefusals(t *testing.T) { + t.Parallel() + tests := []struct { + name string + opts engine.RunOptions + wantErr string + }{ + {"both channels", engine.RunOptions{ + FormatOptions: "programmatic", + CompilerOptions: map[string]string{"shape": "Widget"}, + }, "set FormatOptions or CompilerOptions, not both"}, + {"undecodable setting", engine.RunOptions{ + CompilerOptions: map[string]string{"boom": "yes"}, + }, "boom is not an option"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + eng, err := engine.NewWith(&smithyCompiler{}) + require.NoError(t, err) + _, err = eng.Run(t.Context(), writeNamed(t, "model.smithy", "$version: \"2\"\n"), tc.opts) + require.Error(t, err) + assert.Contains(t, err.Error(), "engine: options for") + assert.Contains(t, err.Error(), tc.wantErr) + }) + } +} + +// TestEngine_RunCompilerOptionsReachTheOpenAPICompiler asserts through the real +// compiler what the stubs assert through the seam: the same document compiles +// differently under a setting no engine code understands. +func TestEngine_RunCompilerOptionsReachTheOpenAPICompiler(t *testing.T) { + t.Parallel() + eng, err := engine.New() + require.NoError(t, err) + spec := writeSpec(t, `openapi: 3.1.0 +info: {title: T, version: "1"} +paths: + /a/b: + get: {operationId: ab, tags: [zoo], responses: {"200": {description: ok}}} +`) + + byTag, err := eng.Run(t.Context(), spec, engine.RunOptions{}) + require.NoError(t, err) + byPath, err := eng.Run(t.Context(), spec, engine.RunOptions{ + CompilerOptions: map[string]string{"grouping": "path-prefix"}, + }) + require.NoError(t, err) + + assert.Equal(t, "zoo", byTag.Document.Services[0].Groups[0].Name.Source) + assert.Equal(t, "a", byPath.Document.Services[0].Groups[0].Name.Source) +} diff --git a/engine/sniff.go b/engine/sniff.go deleted file mode 100644 index c1f8edd7..00000000 --- a/engine/sniff.go +++ /dev/null @@ -1,54 +0,0 @@ -package engine - -import ( - "fmt" - - yaml "gopkg.in/yaml.v3" - - "github.com/dexpace/morphic/compilers" -) - -// sniffProbe holds the two discriminating keys read from the source bytes. YAML -// is a JSON superset, so a single yaml decode handles both JSON and YAML specs. -type sniffProbe struct { - OpenAPI string `yaml:"openapi"` - Swagger string `yaml:"swagger"` -} - -// Sniff probe-decodes the source bytes and reports the spec format they declare. -// An `openapi: 3.X.Y` key yields the openapi compiler keyed by the major.minor -// prefix; `swagger: "2.0"` is recognized but unsupported; anything else is an -// error. Undecodable bytes yield a wrapped decode error. -func Sniff(data []byte) (compilers.SourceFormat, error) { - var probe sniffProbe - if err := yaml.Unmarshal(data, &probe); err != nil { - return compilers.SourceFormat{}, fmt.Errorf("sniff: decode source: %w", err) - } - switch { - case probe.OpenAPI != "": - return compilers.SourceFormat{Name: "openapi", Version: majorMinor(probe.OpenAPI)}, nil - case probe.Swagger != "": - return compilers.SourceFormat{}, fmt.Errorf( - "swagger 2.0 is not supported yet (planned: lift into the openapi compiler)") - default: - return compilers.SourceFormat{}, fmt.Errorf("unrecognized spec format") - } -} - -// majorMinor returns the "major.minor" prefix of a dotted version string, -// e.g. "3.1.0" → "3.1". Strings with fewer than two dots — a bare major -// version, or a version already in major.minor form — are returned unchanged. -func majorMinor(version string) string { - firstDot := -1 - for i := 0; i < len(version); i++ { - if version[i] != '.' { - continue - } - if firstDot < 0 { - firstDot = i - continue - } - return version[:i] - } - return version -} diff --git a/engine/sniff_test.go b/engine/sniff_test.go deleted file mode 100644 index f5aff368..00000000 --- a/engine/sniff_test.go +++ /dev/null @@ -1,45 +0,0 @@ -package engine_test - -import ( - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/dexpace/morphic/compilers" - "github.com/dexpace/morphic/engine" -) - -func TestSniff_Formats(t *testing.T) { - t.Parallel() - cases := []struct { - name, src string - want compilers.SourceFormat - wantErr string - }{ - {"openapi 3.1 yaml", "openapi: 3.1.0\ninfo: {}\n", compilers.SourceFormat{Name: "openapi", Version: "3.1"}, ""}, - {"openapi 3.0 json", `{"openapi": "3.0.3"}`, compilers.SourceFormat{Name: "openapi", Version: "3.0"}, ""}, - {"openapi 3.2 yaml", "openapi: 3.2.0\ninfo: {}\n", compilers.SourceFormat{Name: "openapi", Version: "3.2"}, ""}, - // A version already in major.minor form (single dot) exercises majorMinor's - // unchanged-passthrough return. - {"openapi major.minor only", "openapi: \"3.1\"\n", compilers.SourceFormat{Name: "openapi", Version: "3.1"}, ""}, - // A bare-major version (no dot) also reaches the passthrough return. - {"openapi bare major", "openapi: \"4\"\n", compilers.SourceFormat{Name: "openapi", Version: "4"}, ""}, - {"swagger", "swagger: \"2.0\"\n", compilers.SourceFormat{}, "swagger 2.0 is not supported yet"}, - {"unknown", "hello: world\n", compilers.SourceFormat{}, "unrecognized spec format"}, - {"undecodable yaml", "openapi: [unterminated\n", compilers.SourceFormat{}, "sniff: decode source"}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - got, err := engine.Sniff([]byte(tc.src)) - if tc.wantErr != "" { - require.Error(t, err) - assert.Contains(t, err.Error(), tc.wantErr) - return - } - require.NoError(t, err) - assert.Equal(t, tc.want, got) - }) - } -} diff --git a/internal/archtest/arch_test.go b/internal/archtest/arch_test.go index bec94f78..10ff3355 100644 --- a/internal/archtest/arch_test.go +++ b/internal/archtest/arch_test.go @@ -176,8 +176,12 @@ var rules = map[string][]string{ module + "/compilers/openapi/internal/value", "github.com/speakeasy-api/openapi" + subtreeSuffix, "gopkg.in/yaml.v3"}, "pass": {module + "/ir"}, + // The orchestration. It reaches the compiler package to compose the default + // registry and nothing of any source format: what a spec looks like and what + // its options are called are answered through the contract, so no parser is + // named here. "engine": {module + "/ir", module + "/compilers", module + "/compilers/openapi", - module + "/pass", "gopkg.in/yaml.v3"}, + module + "/pass"}, "cmd/morphic": {module + "/ir", module + "/engine"}, "cmd/morphic-harness": {module + "/internal/harness"}, "internal/testspec": {}, From bf5db497c40ff558dedef46e67231bd0154c89d5 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Mon, 10 Aug 2026 07:00:46 +0300 Subject: [PATCH 02/11] fix(compilers): keep detecting past a compiler that names no format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Registry.Detect ended its search on any compiler answering ok, including one that named no format at all. That answer says nothing — a compiler recognizing a source names what it recognized — and acting on it hides every compiler registered after: a source the next one would have taken comes back unrecognized, with nothing in the output naming the compiler that swallowed it. Skip such a compiler and keep asking. Its diagnostics are not collected, since the contract reads those only from a compiler that declined, and this one did not say it declined. Also pin the bound the key search shares with the decode. declaresProbeKey reads the same bounded prefix sniff does, so a source declaring an OpenAPI key only past the cap is declined silently rather than reported as this compiler's own and broken — claiming it would assert something about bytes detection never read. The truncation ran under the existing tests without any of them observing its effect; a source whose prefix fails to parse and whose key falls past the cap is the case that separates the two. --- compilers/compilers.go | 10 ++++++++++ compilers/compilers_test.go | 33 ++++++++++++++++++++++++++++++++ compilers/openapi/detect_test.go | 7 +++++++ 3 files changed, 50 insertions(+) diff --git a/compilers/compilers.go b/compilers/compilers.go index 7a725af5..63d6e68f 100644 --- a/compilers/compilers.go +++ b/compilers/compilers.go @@ -164,6 +164,16 @@ func (r *Registry) Detect(src Source) (Compiler, SourceFormat, []ir.Diagnostic, declined = append(declined, diags...) continue } + // A compiler that recognizes a source names the format it recognized. One + // that claims a source and names nothing has answered no question, and + // letting it end the search would hide every compiler registered after it + // — a source another compiler would have taken becomes unrecognized, with + // nothing to say which compiler swallowed it. Its diags are not collected: + // the contract reads them only when a compiler declines, and this one did + // not say it declined. + if format == (SourceFormat{}) { + continue + } owner, registered := r.byFormat[format] return owner, format, nil, registered } diff --git a/compilers/compilers_test.go b/compilers/compilers_test.go index f98c0743..9c471cbf 100644 --- a/compilers/compilers_test.go +++ b/compilers/compilers_test.go @@ -250,3 +250,36 @@ func TestRegistry_DetectDropsDeclinesOnceClaimed(t *testing.T) { assert.Equal(t, compilers.SourceFormat{Name: "beta", Version: "2"}, format) assert.Empty(t, diags, "the source found a compiler, so nothing declined is worth saying") } + +// claimsNothing recognizes every source and names no format, which the contract +// does not allow. It exists to pin what the registry does with a compiler that +// breaks it. +type claimsNothing struct{ stubCompiler } + +func (claimsNothing) Detect(compilers.Source) (compilers.SourceFormat, []ir.Diagnostic, bool) { + return compilers.SourceFormat{}, nil, true +} + +// TestRegistry_DetectSkipsACompilerThatClaimsWithoutNaming pins that a compiler +// answering "mine" while naming no format does not end the search. Ending it +// there would make a source the next compiler would have taken come back +// unrecognized, with nothing in the output naming the compiler that swallowed it. +func TestRegistry_DetectSkipsACompilerThatClaimsWithoutNaming(t *testing.T) { + t.Parallel() + broken := &claimsNothing{stubCompiler{ + formats: []compilers.SourceFormat{{Name: "alpha", Version: "1"}}, + }} + claims := &stubCompiler{ + formats: []compilers.SourceFormat{{Name: "beta", Version: "2"}}, + marker: "beta", + } + reg := compilers.NewRegistry() + require.NoError(t, reg.Register(broken)) + require.NoError(t, reg.Register(claims)) + + got, format, _, ok := reg.Detect(compilers.Source{Path: "s.txt", Data: []byte("beta")}) + + require.True(t, ok, "the compiler after the broken one must still be asked") + assert.Same(t, compilers.Compiler(claims), got) + assert.Equal(t, compilers.SourceFormat{Name: "beta", Version: "2"}, format) +} diff --git a/compilers/openapi/detect_test.go b/compilers/openapi/detect_test.go index 5429c4f1..c2bc3063 100644 --- a/compilers/openapi/detect_test.go +++ b/compilers/openapi/detect_test.go @@ -71,6 +71,13 @@ func TestDetect_Formats(t *testing.T) { {"unparseable past the cap", "api.yaml", padTo("openapi: [unterminated\n", "filler: x\n"), compilers.SourceFormat{}, false, []string{diag.UndecodableSource}}, + // Declares the key only past the cap, on a prefix that does not parse. The + // key search reads the same bounded prefix the decode did and so does not + // see it either; claiming the source would assert something about bytes + // detection never read. + {"key past the cap on an unparseable prefix", "api.yaml", + padTo("bad: [unterminated\n", "filler: x\n") + "openapi: 3.1.0\n", + compilers.SourceFormat{}, false, nil}, {"empty", "empty.yaml", "", compilers.SourceFormat{}, false, nil}, } for _, tc := range cases { From 86521144fb082fe2ff8c5851cb6cd555be3eeb2f Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Mon, 10 Aug 2026 07:02:27 +0300 Subject: [PATCH 03/11] docs(compilers): state that an ok detection must name a format --- compilers/compilers.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/compilers/compilers.go b/compilers/compilers.go index 63d6e68f..2c558fd9 100644 --- a/compilers/compilers.go +++ b/compilers/compilers.go @@ -76,6 +76,10 @@ type Compiler interface { // format it does not serve — a version it has yet to implement — so that the // caller can say so rather than report the source as unrecognized. // + // An ok answer must name a format. Recognizing a source is knowing what it + // is, so the zero format with ok true is no answer, and Registry.Detect + // passes over a compiler that gives one rather than let it end the search. + // // diags is what this compiler can say about a source it declines, and is read // only when ok is false. Bytes of another format are ordinary input here, so // declining them is silent: a compiler that reported every source it did not From ce7f5463cca41d3b6d08569c125790eb91f970a4 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Mon, 10 Aug 2026 07:10:48 +0300 Subject: [PATCH 04/11] feat(cli)!: accept --opt on validate as well as compile validate is compile's pipeline with the document dropped, so the two have to configure that pipeline the same way. --opt was bound only by compile, which left a spec that needs an option to compile impossible to check the way it would be built: `validate spec.yaml` and `compile spec.yaml --opt overlay=o.yaml` read different documents, and a gate could pass a spec the build then rejects. Bind it in bindSpecFlags with the rest of the shared flags, which also puts its spelling, default and help text under TestSpecFlags_SharedFlagsAgree rather than leaving two definitions to drift. bindSpecFlags now makes the settings map, so neither constructor has to remember to. Registry.Detect resolves its owner through Lookup rather than reaching into byFormat, which is the same read and leaves the registry's format-keyed lookup with a caller in the pipeline again now that Run detects instead of looking up. --- README.md | 2 +- cmd/morphic/command_test.go | 2 +- cmd/morphic/compile.go | 15 +++++++------ cmd/morphic/testdata/validate-help.txt | 5 +++++ cmd/morphic/validate.go | 4 +++- cmd/morphic/validate_test.go | 29 ++++++++++++++++++++++++++ compilers/compilers.go | 2 +- 7 files changed, 49 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 185f7465..562bd663 100644 --- a/README.md +++ b/README.md @@ -122,7 +122,7 @@ Help always prints to stdout and exits `0`. | `-o ` | `compile` | Write IR JSON to `` instead of stdout, compact rather than indented. | | `--pretty` | `compile` | Indent the JSON `-o` writes; stdout is indented either way. | | `--explain ` | `compile` | Report what compiling produced at this source coordinate instead of writing the document. | -| `--opt =` | `compile` | Set one option on the compiler the spec selects. Repeatable; a repeated key is refused. | +| `--opt =` | both | Set one option on the compiler the spec selects. Repeatable; a repeated key is refused. | Diagnostics print one per line as ` : `, where `` is `#` for a finding in a spec file, a bare pointer for one an IR pass made about the diff --git a/cmd/morphic/command_test.go b/cmd/morphic/command_test.go index b8fcf36b..19da727f 100644 --- a/cmd/morphic/command_test.go +++ b/cmd/morphic/command_test.go @@ -109,7 +109,7 @@ func TestSpecFlags_SharedFlagsAgree(t *testing.T) { // cannot drift. var ( compileFlagNames = []string{"o", "fail-on", "skip-validate", "explain", "pretty", "opt"} - validateFlagNames = []string{"fail-on", "skip-validate"} + validateFlagNames = []string{"fail-on", "skip-validate", "opt"} ) func TestCommand_PrintFlagsDocumentsTheCommandsOwnFlags(t *testing.T) { diff --git a/cmd/morphic/compile.go b/cmd/morphic/compile.go index 5d3ebbb0..b47e230e 100644 --- a/cmd/morphic/compile.go +++ b/cmd/morphic/compile.go @@ -87,9 +87,10 @@ func newCompileCommand() command { } } -// specOptions holds what the shared pipeline runner reads. Most of it is parsed -// by flags every spec-taking command defines; settings is the exception, bound -// only by compile, and nil for a command that offers no way to set it. +// specOptions holds what the shared pipeline runner reads, parsed by the flags +// every spec-taking command defines. validate is compile's pipeline with the +// document dropped, so the two configure that pipeline the same way: a spec that +// needs an option to compile needs it to be validated as it will be compiled. type specOptions struct { failOn string skipValidate bool @@ -108,10 +109,14 @@ type compileOptions struct { // Sharing the registration is what keeps their spellings, defaults and help // text identical across commands rather than identical-looking. func bindSpecFlags(fs *flag.FlagSet, opts *specOptions) { + opts.settings = settingFlag{} + fs.StringVar(&opts.failOn, "fail-on", "error", "fail (exit 1) on diagnostics at or above this severity: error|warning") fs.BoolVar(&opts.skipValidate, "skip-validate", false, "skip the referential-integrity validate pass") + fs.Var(opts.settings, "opt", + "set one `key=value` option on the compiler the spec selects (repeatable)") } // newCompileFlags returns compile's FlagSet and the options its flags write @@ -123,13 +128,11 @@ func newCompileFlags() (*flag.FlagSet, *compileOptions) { fs := flag.NewFlagSet("compile", flag.ContinueOnError) fs.SetOutput(io.Discard) - opts := compileOptions{specOptions: specOptions{settings: settingFlag{}}} + var opts compileOptions bindSpecFlags(fs, &opts.specOptions) fs.StringVar(&opts.outPath, "o", "", "write IR JSON to this file instead of stdout") fs.StringVar(&opts.explain, "explain", "", "report what compiling produced at this source pointer instead of writing IR JSON") - fs.Var(opts.settings, "opt", - "set one `key=value` option on the compiler the spec selects (repeatable)") fs.BoolVar(&opts.pretty, "pretty", false, "indent the IR JSON -o writes; stdout is indented either way") diff --git a/cmd/morphic/testdata/validate-help.txt b/cmd/morphic/testdata/validate-help.txt index 90c6e723..5465053a 100644 --- a/cmd/morphic/testdata/validate-help.txt +++ b/cmd/morphic/testdata/validate-help.txt @@ -13,8 +13,13 @@ The diagnostics and the exit code are compile's: 0 when nothing reaches --skip-validate names the referential-integrity pass, not this command: it drops that pass's diagnostics and keeps the compiler's. +--opt is compile's, and takes the same settings: a spec that needs one to +compile needs it here too, or what is checked is not what would be built. + flags: -fail-on string fail (exit 1) on diagnostics at or above this severity: error|warning (default "error") + -opt key=value + set one key=value option on the compiler the spec selects (repeatable) -skip-validate skip the referential-integrity validate pass diff --git a/cmd/morphic/validate.go b/cmd/morphic/validate.go index 99d6aae4..34215acd 100644 --- a/cmd/morphic/validate.go +++ b/cmd/morphic/validate.go @@ -20,7 +20,9 @@ func newValidateCommand() command { "--fail-on, 1 when something does or the spec lowered to no document at all,\n" + "2 for a misuse or an I/O error.\n\n" + "--skip-validate names the referential-integrity pass, not this command: it\n" + - "drops that pass's diagnostics and keeps the compiler's.", + "drops that pass's diagnostics and keeps the compiler's.\n\n" + + "--opt is compile's, and takes the same settings: a spec that needs one to\n" + + "compile needs it here too, or what is checked is not what would be built.", printFlags: func(w io.Writer) { fs, _ := newValidateFlags() fs.SetOutput(w) diff --git a/cmd/morphic/validate_test.go b/cmd/morphic/validate_test.go index 3c614e95..3c50fb39 100644 --- a/cmd/morphic/validate_test.go +++ b/cmd/morphic/validate_test.go @@ -226,3 +226,32 @@ func TestRun_ValidateNilDocumentReturnsOne(t *testing.T) { assert.Empty(t, stdout.String(), "validate must write nothing to stdout") assert.Contains(t, stderr.String(), "openapi/unsupported-version") } + +// TestValidate_CompilerOptionReachesThePipeline pins that validate configures +// the compiler the way compile does. validate is compile's pipeline with the +// document dropped, so a spec needing an option to compile has to be checkable +// under that same option — otherwise what validate checks is not what compile +// would build, and the gate passes a spec the build then rejects. +// +// The overlay targets nothing, so the failure can only be the overlay's doing: +// the same spec validates clean without it, which the first assertion holds. +func TestValidate_CompilerOptionReachesThePipeline(t *testing.T) { + t.Parallel() + spec := writeFile(t, "spec.yaml", testspec.Tiny) + overlay := writeFile(t, "overlay.yaml", "overlay: 1.0.0\n"+ + "info: {title: O, version: \"1\"}\n"+ + "actions:\n - target: $.nonexistent\n update: {x: y}\n") + + var cleanOut, cleanErr bytes.Buffer + require.Equal(t, 0, run([]string{"validate", spec}, &cleanOut, &cleanErr), + "the spec must validate clean on its own, or the assertion below proves nothing: %s", + cleanErr.String()) + + var stdout, stderr bytes.Buffer + code := run([]string{"validate", spec, "--opt", "overlay=" + overlay}, &stdout, &stderr) + + assert.Equal(t, 1, code, "stderr: %s", stderr.String()) + assert.Contains(t, stderr.String(), "openapi/overlay-failed", + "the overlay must have been applied, which only --opt can have done") + assert.Empty(t, stdout.String(), "validate writes no document") +} diff --git a/compilers/compilers.go b/compilers/compilers.go index 2c558fd9..37e5755c 100644 --- a/compilers/compilers.go +++ b/compilers/compilers.go @@ -178,7 +178,7 @@ func (r *Registry) Detect(src Source) (Compiler, SourceFormat, []ir.Diagnostic, if format == (SourceFormat{}) { continue } - owner, registered := r.byFormat[format] + owner, registered := r.Lookup(format) return owner, format, nil, registered } return nil, SourceFormat{}, declined, false From 587805603e10e1073e6cced75a3db9d20fc641a2 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Mon, 10 Aug 2026 16:24:24 +0300 Subject: [PATCH 05/11] feat(engine)!: name the formats a build compiles, and refuse an empty engine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two answers the detection rework left less useful than it found them. A refusal named what failed and not what would have worked. "no compiler registered for format swagger@2.0" reads as a configuration slip — as though a compiler had been left unregistered — when it is a fact about the build, and it covered a format morphic does not serve and a version outside the supported range with one wording that helps neither. Registry.Formats reports what the registry holds, sorted so the same set never reads as two, and both refusals now end with "this build compiles openapi@3.0, openapi@3.1, openapi@3.2". The engine still knows nothing about any format; it is reading its own registry. The compiler's own report, where there is one, is left alone: a parse error with a line number does not need a list appended to it. NewWith accepted an empty compiler set. Nothing can be added to a built engine, so that engine could never compile anything, and every source handed to it came back "unrecognized spec format" — blaming the document for a misconfiguration of the caller, which is the same category error the exit codes were untangled to avoid. It is refused at construction, where the mistake is. That reverses a note asking for exactly this precondition not to be added, because an empty engine was then the only way to reach Run's nothing-recognized branch. It was true while the engine sniffed formats itself and named one for every parseable spec. Detection belongs to the compilers now, so a source none of them claims reaches that branch with a full registry; removing the test that note protected leaves the coverage gate at 100%. The reasoning is recorded in both places rather than deleted. --- compilers/compilers.go | 23 ++++++++++++++ compilers/compilers_test.go | 31 +++++++++++++++++++ engine/engine.go | 46 +++++++++++++++++++++++----- engine/engine_test.go | 60 ++++++++++++++++++++++++++----------- 4 files changed, 134 insertions(+), 26 deletions(-) diff --git a/compilers/compilers.go b/compilers/compilers.go index 37e5755c..728d42b2 100644 --- a/compilers/compilers.go +++ b/compilers/compilers.go @@ -5,6 +5,8 @@ import ( "errors" "fmt" "reflect" + "slices" + "strings" "github.com/dexpace/morphic/ir" ) @@ -198,6 +200,27 @@ func isNilCompiler(c Compiler) bool { return rv.Kind() == reflect.Pointer && rv.IsNil() } +// Formats returns every format some registered compiler serves. +// +// It is sorted rather than in registration order, because this answers "what +// does this build accept" for a reader, and a set rendered in a different order +// from run to run reads as a different set. Sorting is by name then version +// string, so versions order lexically — enough while a version is major.minor, +// and a display order either way. +func (r *Registry) Formats() []SourceFormat { + formats := make([]SourceFormat, 0, len(r.byFormat)) + for format := range r.byFormat { + formats = append(formats, format) + } + slices.SortFunc(formats, func(a, b SourceFormat) int { + if byName := strings.Compare(a.Name, b.Name); byName != 0 { + return byName + } + return strings.Compare(a.Version, b.Version) + }) + return formats +} + // Lookup returns the compiler registered for format. func (r *Registry) Lookup(format SourceFormat) (Compiler, bool) { c, ok := r.byFormat[format] diff --git a/compilers/compilers_test.go b/compilers/compilers_test.go index 9c471cbf..0eb062db 100644 --- a/compilers/compilers_test.go +++ b/compilers/compilers_test.go @@ -283,3 +283,34 @@ func TestRegistry_DetectSkipsACompilerThatClaimsWithoutNaming(t *testing.T) { assert.Same(t, compilers.Compiler(claims), got) assert.Equal(t, compilers.SourceFormat{Name: "beta", Version: "2"}, format) } + +// TestRegistry_FormatsAreSortedNotRegistrationOrder pins both halves of what +// Formats answers: every format some compiler serves, and in an order that does +// not depend on how the registry was built. Registration order is deliberately +// not it — this answers "what does this build accept" for a reader, and the same +// set rendered two ways reads as two sets. +func TestRegistry_FormatsAreSortedNotRegistrationOrder(t *testing.T) { + t.Parallel() + reg := compilers.NewRegistry() + require.NoError(t, reg.Register(&stubCompiler{formats: []compilers.SourceFormat{ + {Name: "smithy", Version: "2.0"}, + {Name: "openapi", Version: "3.1"}, + }})) + require.NoError(t, reg.Register(&stubCompiler{formats: []compilers.SourceFormat{ + {Name: "openapi", Version: "3.0"}, + }})) + + assert.Equal(t, []compilers.SourceFormat{ + {Name: "openapi", Version: "3.0"}, + {Name: "openapi", Version: "3.1"}, + {Name: "smithy", Version: "2.0"}, + }, reg.Formats()) +} + +// TestRegistry_FormatsOfAnEmptyRegistryIsEmpty pins that the zero registry +// answers rather than panicking on its nil map. +func TestRegistry_FormatsOfAnEmptyRegistryIsEmpty(t *testing.T) { + t.Parallel() + assert.Empty(t, compilers.NewRegistry().Formats()) + assert.Empty(t, new(compilers.Registry).Formats()) +} diff --git a/engine/engine.go b/engine/engine.go index c2b7a818..dc36a7fe 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "os" + "strings" "github.com/dexpace/morphic/compilers" "github.com/dexpace/morphic/compilers/openapi" @@ -61,11 +62,24 @@ func New() (*Engine, error) { // compiler and a register failure (a compiler reporting no formats, or two // compilers claiming the same format) alike surface as a Go error rather than a // panic, and the error names the argument position so a caller passing several -// compilers can tell which one was rejected. Calling NewWith with no compilers -// is legal: the resulting engine's Run always fails at detection, with nobody -// to ask and so nothing to report but that the format went unrecognized, which -// is the seam TestEngine_RunNothingRegistered relies on to reach that branch. +// compilers can tell which one was rejected. +// +// An empty set is refused. There is no way to add a compiler to a built engine, +// so an engine with none can never compile anything, and every source handed to +// it would come back reported as unrecognized — blaming the document for a +// misconfiguration of the caller. Refusing here puts the error at the mistake. +// +// This reverses a note that once stood here, that an empty set had to stay legal +// because it was the only way to reach Run's nothing-recognized branch. That was +// true while the engine sniffed formats itself and named one for every parseable +// spec. Detection belongs to the compilers now, so a source none of them claims +// reaches that branch with a full registry, and the coverage the note protected +// no longer depends on being able to build an engine that cannot work. func NewWith(fronts ...compilers.Compiler) (*Engine, error) { + if len(fronts) == 0 { + return nil, errors.New("engine: no compilers; an engine with none can compile nothing") + } + reg := compilers.NewRegistry() for i, front := range fronts { if err := reg.Register(front); err != nil { @@ -94,7 +108,7 @@ func (e *Engine) Run(ctx context.Context, specPath string, opts RunOptions) (*Re front, format, declined, ok := e.registry.Detect(source) if !ok { - return &Result{Format: format, Diagnostics: undetected(format, declined)}, nil + return &Result{Format: format, Diagnostics: e.undetected(format, declined)}, nil } formatOpts, err := formatOptions(front, opts) if err != nil { @@ -130,15 +144,31 @@ func (e *Engine) Run(ctx context.Context, specPath string, opts RunOptions) (*Re // account of why is whatever the compilers that declined chose to give — // preferred over the engine's own, because the engine parses nothing and can // say no more than that nobody claimed it. -func undetected(format compilers.SourceFormat, declined []ir.Diagnostic) []ir.Diagnostic { +func (e *Engine) undetected(format compilers.SourceFormat, declined []ir.Diagnostic) []ir.Diagnostic { if format.Name != "" { return []ir.Diagnostic{specProblem(codeNoCompilerForFormat, - "no compiler registered for format %s", format)} + "no compiler registered for format %s; %s", format, e.served())} } if len(declined) > 0 { return declined } - return []ir.Diagnostic{specProblem(codeUnrecognizedFormat, "unrecognized spec format")} + return []ir.Diagnostic{specProblem(codeUnrecognizedFormat, + "unrecognized spec format; %s", e.served())} +} + +// served names the formats this build compiles, for a reader who has just been +// told theirs is not one of them. Naming what failed without naming what would +// have worked leaves the next step to guesswork, and the engine can answer it +// from its own registry without knowing what any of the names mean. +// +// NewWith refuses an empty compiler set, so there is always at least one. +func (e *Engine) served() string { + formats := e.registry.Formats() + names := make([]string, 0, len(formats)) + for _, format := range formats { + names = append(names, format.String()) + } + return "this build compiles " + strings.Join(names, ", ") } // formatOptions resolves the compiler options for one run, decoding the textual diff --git a/engine/engine_test.go b/engine/engine_test.go index c09495ba..755888f6 100644 --- a/engine/engine_test.go +++ b/engine/engine_test.go @@ -187,27 +187,23 @@ func TestEngine_RunDetectionProblemsAreDiagnostics(t *testing.T) { } } -func TestEngine_RunNothingRegistered(t *testing.T) { +// TestNewWith_RefusesAnEmptyCompilerSet pins that an engine which can compile +// nothing cannot be built. There is no way to add a compiler to a built engine, +// so the alternative is one that reports every source it is handed as +// unrecognized — blaming the document for a misconfiguration of the caller. +// +// An earlier note here asked that this precondition not be added, on the grounds +// that an empty engine was the only way to reach Run's nothing-recognized +// branch. Detection belongs to the compilers now, so an ordinary source none of +// them claims reaches that branch with a full registry; the tests above do it. +func TestNewWith_RefusesAnEmptyCompilerSet(t *testing.T) { t.Parallel() - // NewWith() with zero compilers is load-bearing here: it is the only way to - // reach an engine that has nothing to ask about a source every registered - // compiler would otherwise claim. Don't add a len(fronts) == 0 precondition - // to NewWith — doing so would make this branch unreachable. - eng, err := engine.NewWith() - require.NoError(t, err) - res, err := eng.Run(t.Context(), writeSpec(t, testspec.Tiny), engine.RunOptions{}) + eng, err := engine.NewWith() - require.NoError(t, err, "a spec no compiler claims is not a Go error") - require.NotNil(t, res) - assert.Nil(t, res.Document) - require.Len(t, res.Diagnostics, 1) - // Unrecognized, not no-compiler-for-format: with nothing registered there is - // nobody to read the source, so the engine cannot name the format the way it - // could when it sniffed the bytes itself. It parses nothing now, and says so. - assert.Equal(t, "engine/unrecognized-format", res.Diagnostics[0].Code) - assert.Equal(t, compilers.SourceFormat{}, res.Format, - "no compiler was there to name a format") + require.Error(t, err) + assert.Nil(t, eng, "nothing usable comes back from a refused construction") + assert.Contains(t, err.Error(), "no compilers") } // TestEngine_RunEmptySource pins that an empty file is nobody's spec, without a @@ -554,3 +550,31 @@ paths: assert.Equal(t, "zoo", byTag.Document.Services[0].Groups[0].Name.Source) assert.Equal(t, "a", byPath.Document.Services[0].Groups[0].Name.Source) } + +// TestEngine_RunNamesWhatItCanCompile pins that a source the engine will not +// take is told what it would have taken. Naming the failure without naming the +// alternative leaves the next step to guesswork, and this is the whole of what +// separates "morphic cannot read your file" from "morphic reads OpenAPI 3.x". +func TestEngine_RunNamesWhatItCanCompile(t *testing.T) { + t.Parallel() + tests := []struct{ name, spec string }{ + {"unrecognized", "hello: world\n"}, + {"recognized but unserved", "swagger: \"2.0\"\ninfo: {}\n"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + eng, err := engine.New() + require.NoError(t, err) + + res, err := eng.Run(t.Context(), writeSpec(t, tt.spec), engine.RunOptions{}) + + require.NoError(t, err) + require.Len(t, res.Diagnostics, 1) + for _, format := range []string{"openapi@3.0", "openapi@3.1", "openapi@3.2"} { + assert.Contains(t, res.Diagnostics[0].Message, format, + "the refusal must name the formats this build serves") + } + }) + } +} From 6d2fe60763bb2e0bdbb483a18ed4e4bbcdcc5dc5 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Mon, 10 Aug 2026 16:28:52 +0300 Subject: [PATCH 06/11] docs(compilers): correct what the format sort actually guarantees MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Formats sorts versions as strings, and the comment said that was "enough while a version is major.minor". It is not: 3.10 is major.minor and sorts ahead of 3.2, so the stated condition is satisfied by exactly the case it fails on. The real condition is single-digit minors. State it that way, and say why the deviation is left standing rather than fixed — the order is read and never compared against, and a version comparator guessing at every format's scheme is a larger thing to get wrong than a list one line out of order. Also narrow served's claim to what makes it true: NewWith builds every Engine, so the list it renders is never empty. --- compilers/compilers.go | 11 ++++++++--- engine/engine.go | 3 ++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/compilers/compilers.go b/compilers/compilers.go index 728d42b2..62ef1bf3 100644 --- a/compilers/compilers.go +++ b/compilers/compilers.go @@ -204,9 +204,14 @@ func isNilCompiler(c Compiler) bool { // // It is sorted rather than in registration order, because this answers "what // does this build accept" for a reader, and a set rendered in a different order -// from run to run reads as a different set. Sorting is by name then version -// string, so versions order lexically — enough while a version is major.minor, -// and a display order either way. +// from run to run reads as a different set. +// +// Sorting is by name, then by version as a string. That matches numeric order +// only while minor versions stay single-digit: a 3.10 would sort ahead of 3.2, +// not behind it. The deviation is left rather than fixed because this order is +// read, never compared against — nothing selects a compiler by position here — +// and a version comparator that guessed at every format's scheme would be a +// larger thing to get wrong than a list one line out of order. func (r *Registry) Formats() []SourceFormat { formats := make([]SourceFormat, 0, len(r.byFormat)) for format := range r.byFormat { diff --git a/engine/engine.go b/engine/engine.go index dc36a7fe..5d2f99db 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -161,7 +161,8 @@ func (e *Engine) undetected(format compilers.SourceFormat, declined []ir.Diagnos // have worked leaves the next step to guesswork, and the engine can answer it // from its own registry without knowing what any of the names mean. // -// NewWith refuses an empty compiler set, so there is always at least one. +// NewWith builds every Engine and refuses an empty compiler set, so there is +// always at least one format to name and the sentence never trails off. func (e *Engine) served() string { formats := e.registry.Formats() names := make([]string, 0, len(formats)) From b34b272c980e0b7274cb9f9d36944273fb2d7913 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Mon, 10 Aug 2026 16:43:01 +0300 Subject: [PATCH 07/11] fix(engine): report an unbuilt engine instead of panicking on it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An Engine that never went through New or NewWith carries a nil registry, and Run dereferenced it during detection: `var e engine.Engine` followed by e.Run panicked with a nil pointer dereference rather than returning the Go error a misuse of the API should produce. A panic out of a library is not a report — the caller cannot tell it from a bug in the compiler they were calling — and Run already reserves its error return for exactly this class. Guard the nil receiver and the nil registry alike, ahead of the file read: an engine that was never built is the caller's mistake whatever the path turns out to say, and reporting the path first would send them to look at the file. Closes #386 --- engine/engine.go | 9 +++++++++ engine/engine_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/engine/engine.go b/engine/engine.go index 5d2f99db..f89e85b3 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -100,6 +100,15 @@ func NewWith(fronts ...compilers.Compiler) (*Engine, error) { // error as "the pipeline was invoked wrongly" therefore stays correct, which is // what lets the CLI keep its usage exit code for actual misuse. func (e *Engine) Run(ctx context.Context, specPath string, opts RunOptions) (*Result, error) { + // Ahead of the read, because an engine that never went through a constructor + // is the caller's mistake whatever the path turns out to say, and reporting + // the path first would send them to look at the file. Without this the nil + // registry is dereferenced during detection and the package panics, which is + // no way to report a misuse of it. + if e == nil || e.registry == nil { + return nil, errors.New("engine: uninitialized; build one with New or NewWith") + } + data, err := os.ReadFile(specPath) if err != nil { return nil, fmt.Errorf("engine: read spec %q: %w", specPath, err) diff --git a/engine/engine_test.go b/engine/engine_test.go index 755888f6..82456397 100644 --- a/engine/engine_test.go +++ b/engine/engine_test.go @@ -578,3 +578,31 @@ func TestEngine_RunNamesWhatItCanCompile(t *testing.T) { }) } } + +// TestEngine_RunOnAnUnbuiltEngine pins that an Engine which never went through a +// constructor reports the caller's mistake instead of crashing on the nil +// registry it carries. Run reserves its Go error for programmer errors and this +// is one; a panic out of a library is not a report, and the caller cannot tell +// it from a bug in the compiler it was calling. +func TestEngine_RunOnAnUnbuiltEngine(t *testing.T) { + t.Parallel() + spec := writeSpec(t, testspec.Tiny) + tests := []struct { + name string + eng *engine.Engine + }{ + {"zero value", &engine.Engine{}}, + {"nil receiver", nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + res, err := tt.eng.Run(t.Context(), spec, engine.RunOptions{}) + + require.Error(t, err, "an unbuilt engine must not panic") + assert.Nil(t, res) + assert.Contains(t, err.Error(), "uninitialized") + }) + } +} From ce46e6e7191c7930de1efa89d3536715431e8250 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Mon, 10 Aug 2026 16:49:41 +0300 Subject: [PATCH 08/11] fix(compilers/openapi): refuse an empty overlay path instead of ignoring it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--opt overlay=` decoded to no overlay at all and exited 0: the caller asked for one, got none, and was told nothing. That is the case DecodeOptions exists to prevent — its contract calls an unusable value an error precisely so a setting is never silently dropped — and an empty path is unusable, not a way to spell "no overlay". It also mislaid the blame when laxness came with it. `--opt overlay= --opt overlay-lax=true` reported that overlay-lax applies only with overlay, naming a flag the caller had in fact passed and sending them to look at the wrong one. Refuse it where the value is read, alongside the other per-setting checks, so finish's empty overlayPath keeps its one meaning: no overlay was named. --- compilers/openapi/options.go | 7 +++++++ compilers/openapi/options_test.go | 8 ++++++++ 2 files changed, 15 insertions(+) diff --git a/compilers/openapi/options.go b/compilers/openapi/options.go index 3ba06e62..9b6779d0 100644 --- a/compilers/openapi/options.go +++ b/compilers/openapi/options.go @@ -152,6 +152,13 @@ func (d *optionDecode) set(key, value string) error { case optAllowExternalRefs: return decodeBool(&d.opts.AllowExternalRefs, key, value) case optOverlay: + // An empty path is refused rather than read as "no overlay". Taking it + // would apply none while the caller believes one was applied, and would + // then blame optOverlayLax for applying without an overlay the caller + // did in fact name. + if value == "" { + return fmt.Errorf("openapi: option %q: want a file path, got an empty value", optOverlay) + } d.overlayPath = value return nil case optOverlayLax: diff --git a/compilers/openapi/options_test.go b/compilers/openapi/options_test.go index fb347b8c..43621474 100644 --- a/compilers/openapi/options_test.go +++ b/compilers/openapi/options_test.go @@ -92,6 +92,14 @@ func TestDecodeOptions_Refusals(t *testing.T) { readsNothing, `want a boolean, got "sort of"`}, {"laxness with no overlay", map[string]string{"overlay-lax": "true"}, nil, `"overlay-lax" applies only with "overlay"`}, + // An empty path is a value that cannot be honoured, not a way to ask for + // no overlay. Read as the latter it applies none while the caller believes + // one was applied — and, with laxness set, blames overlay-lax for an + // overlay the caller did name. + {"empty overlay path", map[string]string{"overlay": ""}, readsNothing, + `"overlay": want a file path, got an empty value`}, + {"empty overlay path with laxness", map[string]string{"overlay": "", "overlay-lax": "true"}, + readsNothing, `"overlay": want a file path, got an empty value`}, {"no reader for a file", map[string]string{"overlay": "p.yaml"}, nil, "names a file and the caller supplied no reader"}, {"unreadable file", map[string]string{"overlay": "p.yaml"}, readsNothing, From 7c92226e71483a4c15635aa9964087a9f4d4bc67 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Mon, 10 Aug 2026 17:03:42 +0300 Subject: [PATCH 09/11] fix(compilers/openapi): keep a diagnostic to the one line it is rendered on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A diagnostic prints as one line — severity, code, location, message — so a message carrying a newline splits one report into several, and every line after the first has no severity, code or location. A reader takes it for another finding; anything parsing stderr takes it for a malformed one. Two messages embed an error raised by a library that writes several lines. `openapi: []` reported "cannot unmarshal !!seq into string" on a second line of its own, and an overlay failing validation on two counts printed the second count as a line with nothing in front of it. The overlay path is not new, but --opt overlay is the first way to reach it without writing Go, so this is where it becomes something a user sees. diag.OneLine collapses the text: parts join with "; " so a flat list of findings reads as a list, except after a part ending in a colon, where the next line is that header's content and a semicolon would read as a break in it. The undecodable-source message also said a source "does not parse" when what failed was the shape of its version key — `openapi: []` parses perfectly well. It now says the source cannot be read, which covers both. TestEngine_RunDiagnosticsAreOneLineEach holds the rendering contract across the detection, parse, overlay and validation paths. The invariant was already asserted, but only inside the one test where a multi-line message had been found before, so it could not reach either of these. --- compilers/openapi/detect.go | 14 ++--- compilers/openapi/internal/diag/diag.go | 33 ++++++++++++ compilers/openapi/internal/diag/diag_test.go | 28 ++++++++++ compilers/openapi/internal/overlay/overlay.go | 4 +- engine/engine_test.go | 52 +++++++++++++++++++ 5 files changed, 123 insertions(+), 8 deletions(-) diff --git a/compilers/openapi/detect.go b/compilers/openapi/detect.go index b21e8b9d..a5befa0a 100644 --- a/compilers/openapi/detect.go +++ b/compilers/openapi/detect.go @@ -39,11 +39,13 @@ type sniffProbe struct { // path is not consulted — an OpenAPI document is what it declares itself to be, // under any extension. // -// Bytes that do not parse are declined silently unless they declare one of the -// discriminating keys, in which case the parse error is reported: a source that -// says `openapi:` and will not parse is this compiler's own and broken, which -// nothing else is in a position to say. Bytes that declare neither key are -// another format's business, and a YAML parser's complaint about them describes +// Bytes the probe cannot read are declined silently unless they declare one of +// the discriminating keys, in which case the reader's complaint is reported: a +// source that says `openapi:` and will not read is this compiler's own and +// broken, which nothing else is in a position to say. That covers a document +// that does not parse and one that parses with a version key of the wrong shape +// alike — both are unreadable here, and neither is another format's. Bytes that +// declare neither key are, and a YAML parser's complaint about them describes // only the parser that was wrong to be asked. func (*Compiler) Detect(src compilers.Source) (compilers.SourceFormat, []ir.Diagnostic, bool) { probe, err := sniff(src.Data) @@ -57,7 +59,7 @@ func (*Compiler) Detect(src compilers.Source) (compilers.SourceFormat, []ir.Diag // there is no source table for a provenance to index into. return compilers.SourceFormat{}, []ir.Diagnostic{diag.Newf( ir.SeverityError, diag.UndecodableSource, ir.Provenance{Source: ir.NoSource}, - "source declares an OpenAPI or Swagger key and does not parse: %v", err)}, false + "source declares an OpenAPI or Swagger key and cannot be read: %s", diag.OneLine(err))}, false default: return compilers.SourceFormat{}, nil, false } diff --git a/compilers/openapi/internal/diag/diag.go b/compilers/openapi/internal/diag/diag.go index a4d92b4c..e93e2fd2 100644 --- a/compilers/openapi/internal/diag/diag.go +++ b/compilers/openapi/internal/diag/diag.go @@ -12,6 +12,7 @@ package diag import ( "fmt" + "strings" "github.com/dexpace/morphic/ir" ) @@ -225,3 +226,35 @@ func Newf(sev ir.Severity, code string, prov ir.Provenance, format string, args func HasError(diags []ir.Diagnostic) bool { return ir.HasError(diags) } + +// OneLine collapses err's text onto a single line, for a diagnostic that carries +// an error raised by something else. +// +// A diagnostic is rendered one per line, so an embedded newline splits one +// report into several — and every line after the first carries no severity, code +// or location, which reads as a malformed diagnostic to anything parsing stderr. +// Both libraries this compiler reports through write multi-line errors: yaml.v3 +// as a header plus one indented line per finding, the overlay validator as a +// flat list of sentences. +// +// Parts are joined with "; " so a flat list reads as a list, except after a part +// that already ends in a colon, where the next line is that header's content and +// a semicolon would read as a break in it. +func OneLine(err error) string { + var out strings.Builder + for _, line := range strings.Split(err.Error(), "\n") { + part := strings.Join(strings.Fields(line), " ") + if part == "" { + continue + } + if out.Len() > 0 { + if strings.HasSuffix(out.String(), ":") { + out.WriteString(" ") + } else { + out.WriteString("; ") + } + } + out.WriteString(part) + } + return out.String() +} diff --git a/compilers/openapi/internal/diag/diag_test.go b/compilers/openapi/internal/diag/diag_test.go index c4e3ce0a..3e5c2eb6 100644 --- a/compilers/openapi/internal/diag/diag_test.go +++ b/compilers/openapi/internal/diag/diag_test.go @@ -2,6 +2,7 @@ package diag_test import ( "encoding/json" + "errors" "go/ast" "go/parser" "go/token" @@ -220,3 +221,30 @@ func constNamesIn(decl ast.Decl) int { } return n } + +// TestOneLine_CollapsesWhatALibraryWrote pins both join rules and the reason for +// each: a flat list of findings reads as a list, while a header that ends in a +// colon owns the line after it and must not be cut from it by a semicolon. +func TestOneLine_CollapsesWhatALibraryWrote(t *testing.T) { + t.Parallel() + tests := []struct { + name, in, want string + }{ + {"already one line", "plain failure", "plain failure"}, + {"flat list", "first problem\nsecond problem", "first problem; second problem"}, + {"header and items", "yaml: unmarshal errors:\n line 1: bad\n line 2: worse", + "yaml: unmarshal errors: line 1: bad; line 2: worse"}, + {"blank lines dropped", "one\n\n\ntwo", "one; two"}, + {"indentation normalized", "one\n\t two three", "one; two three"}, + {"trailing newline", "only\n", "only"}, + {"empty", "", ""}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := diag.OneLine(errors.New(tc.in)) + assert.Equal(t, tc.want, got) + assert.NotContains(t, got, "\n", "the whole point is that nothing survives as a newline") + }) + } +} diff --git a/compilers/openapi/internal/overlay/overlay.go b/compilers/openapi/internal/overlay/overlay.go index ce64ec8f..22473409 100644 --- a/compilers/openapi/internal/overlay/overlay.go +++ b/compilers/openapi/internal/overlay/overlay.go @@ -124,11 +124,11 @@ func applyWithin(index int, root *yaml.Node, opts Options, budget int) (Origin, doc, err := soaoverlay.ParseReader(bytes.NewReader(opts.Data)) if err != nil { return Origin{}, []ir.Diagnostic{diag.Newf(ir.SeverityError, diag.OverlayInvalid, at, - "cannot parse overlay: %s", err)} + "cannot parse overlay: %s", diag.OneLine(err))} } if err := doc.Validate(); err != nil { return Origin{}, []ir.Diagnostic{diag.Newf(ir.SeverityError, diag.OverlayInvalid, at, - "invalid overlay: %s", err)} + "invalid overlay: %s", diag.OneLine(err))} } before, complete := snapshot(root, budget) diff --git a/engine/engine_test.go b/engine/engine_test.go index 82456397..65070653 100644 --- a/engine/engine_test.go +++ b/engine/engine_test.go @@ -606,3 +606,55 @@ func TestEngine_RunOnAnUnbuiltEngine(t *testing.T) { }) } } + +// TestEngine_RunDiagnosticsAreOneLineEach pins the rendering contract the README +// states — one diagnostic per line — across the ways a source can fail, rather +// than at the one site where a multi-line message was first noticed. +// +// A message carrying a newline splits one report into several, and every line +// after the first has no severity, code or location: a reader takes it for +// another finding, and a wrapper parsing stderr takes it for a malformed one. +// Both libraries reported through here write multi-line errors, so the sites +// that embed one are where this keeps breaking; the inputs below reach the +// detection, parse, overlay and validation paths in turn. +func TestEngine_RunDiagnosticsAreOneLineEach(t *testing.T) { + t.Parallel() + const okSpec = "openapi: 3.1.0\ninfo: {title: T, version: \"1\"}\npaths: {}\n" + tests := []struct { + name, spec string + settings map[string]string + }{ + {"version key of the wrong shape", "openapi: []\ninfo: {}\npaths: {}\n", nil}, + {"unparseable", "openapi: [unterminated\n", nil}, + {"unrecognized", "hello: world\n", nil}, + {"unsupported version", "openapi: 4.0.0\ninfo: {title: T, version: \"1\"}\npaths: {}\n", nil}, + {"invalid overlay", okSpec, map[string]string{"overlay": "overlay.yaml"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + spec := filepath.Join(dir, "spec.yaml") + require.NoError(t, os.WriteFile(spec, []byte(tt.spec), 0o600)) + settings := tt.settings + if settings != nil { + overlay := filepath.Join(dir, "overlay.yaml") + require.NoError(t, os.WriteFile(overlay, + []byte("overlay: 1.0.0\ninfo: {title: O}\nactions: []\n"), 0o600)) + settings = map[string]string{"overlay": overlay} + } + eng, err := engine.New() + require.NoError(t, err) + + res, err := eng.Run(t.Context(), spec, engine.RunOptions{CompilerOptions: settings}) + + require.NoError(t, err) + require.NotNil(t, res) + require.NotEmpty(t, res.Diagnostics, "the case must produce a diagnostic to be worth checking") + for _, d := range res.Diagnostics { + assert.NotContains(t, d.Message, "\n", + "one diagnostic is one line: %s / %s", d.Code, d.Message) + } + }) + } +} From 26b723f593dbac1b98d71be1e843d295c5f8b0d5 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Mon, 10 Aug 2026 17:15:16 +0300 Subject: [PATCH 10/11] test(engine): key the one-line diagnostic cases on a flag, not a dead literal --- engine/engine_test.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/engine/engine_test.go b/engine/engine_test.go index 65070653..466abb18 100644 --- a/engine/engine_test.go +++ b/engine/engine_test.go @@ -622,13 +622,15 @@ func TestEngine_RunDiagnosticsAreOneLineEach(t *testing.T) { const okSpec = "openapi: 3.1.0\ninfo: {title: T, version: \"1\"}\npaths: {}\n" tests := []struct { name, spec string - settings map[string]string + // withOverlay applies an overlay that fails validation on two counts, + // which is the input that made the overlay reporter write two lines. + withOverlay bool }{ - {"version key of the wrong shape", "openapi: []\ninfo: {}\npaths: {}\n", nil}, - {"unparseable", "openapi: [unterminated\n", nil}, - {"unrecognized", "hello: world\n", nil}, - {"unsupported version", "openapi: 4.0.0\ninfo: {title: T, version: \"1\"}\npaths: {}\n", nil}, - {"invalid overlay", okSpec, map[string]string{"overlay": "overlay.yaml"}}, + {"version key of the wrong shape", "openapi: []\ninfo: {}\npaths: {}\n", false}, + {"unparseable", "openapi: [unterminated\n", false}, + {"unrecognized", "hello: world\n", false}, + {"unsupported version", "openapi: 4.0.0\ninfo: {title: T, version: \"1\"}\npaths: {}\n", false}, + {"invalid overlay", okSpec, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -636,8 +638,8 @@ func TestEngine_RunDiagnosticsAreOneLineEach(t *testing.T) { dir := t.TempDir() spec := filepath.Join(dir, "spec.yaml") require.NoError(t, os.WriteFile(spec, []byte(tt.spec), 0o600)) - settings := tt.settings - if settings != nil { + var settings map[string]string + if tt.withOverlay { overlay := filepath.Join(dir, "overlay.yaml") require.NoError(t, os.WriteFile(overlay, []byte("overlay: 1.0.0\ninfo: {title: O}\nactions: []\n"), 0o600)) From 76435637e0b018d0f63c6e799fdec2143f63c803 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Mon, 10 Aug 2026 17:19:09 +0300 Subject: [PATCH 11/11] test(cli): cover an empty option value end to end The refusal of `--opt overlay=` was pinned in the compiler's own decode table but not through the CLI, which is where a user meets it. It is a fourth class the existing rows do not reach: the pair is well-formed and the name is known, and the value is still unusable. Reverting the guard leaves every other row green and reddens only this one. --- cmd/morphic/compile_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cmd/morphic/compile_test.go b/cmd/morphic/compile_test.go index e3d305fb..6e9cb62c 100644 --- a/cmd/morphic/compile_test.go +++ b/cmd/morphic/compile_test.go @@ -216,6 +216,10 @@ func TestRun_BadCompilerOptionIsRefused(t *testing.T) { {"malformed pair", "grouping", "want key=value"}, {"unknown name", "gruoping=tags", `unknown option "gruoping"`}, {"unusable value", "grouping=alphabetical", `want "tags" or "path-prefix"`}, + // A fourth class: the pair is well-formed and the name is known, and the + // value is still unusable. Read as "no overlay" it would exit 0 having + // applied none, which is the one outcome --opt exists to rule out. + {"empty value", "overlay=", `"overlay": want a file path`}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) {