From 2bc712caa9526d1a2d1e1c1f212cc6a67450d8ee Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 03:55:46 +0300 Subject: [PATCH] fix(engine): keep compiler diagnostics and exit on spec problems MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run replaced the compiler's returned diagnostics with the document's own list, so a compiler that reports without also storing lost every finding at every severity. It now merges the two and writes the union to both. An undecodable file, an unrecognized format, Swagger 2.0 and a version no compiler claims all left Run as Go errors, which the CLI renders as exit 2 — the code a bad flag gets. They are problems with the spec, so they now come back as diagnostics with a nil document and exit 1, and the usage code is left to actual misuse. --- README.md | 12 ++- cmd/morphic/compile.go | 33 ++++++--- cmd/morphic/compile_test.go | 42 +++++++++-- cmd/morphic/edgecases_test.go | 15 +++- compilers/compilers.go | 6 ++ engine/diag.go | 79 ++++++++++++++++++++ engine/engine.go | 49 ++++++++----- engine/engine_test.go | 133 +++++++++++++++++++++++++++++++--- engine/sniff.go | 27 ++++--- engine/sniff_test.go | 28 ++++--- 10 files changed, 355 insertions(+), 69 deletions(-) create mode 100644 engine/diag.go diff --git a/README.md b/README.md index ccb1d64c..ae93272c 100644 --- a/README.md +++ b/README.md @@ -119,9 +119,15 @@ The flags below are `compile`'s: | `--skip-validate` | Skip the referential-integrity `validate` pass. | | `--explain ` | Report what compiling produced at this source coordinate instead of writing the document. | -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. +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 +document, and absent for one raised before any document existed. + +Exit codes: `0` clean (and for any help request); `1` the spec has problems — a diagnostic reached +the `--fail-on` threshold, or it could not be lowered at all, which covers an undecodable file, an +unrecognized or unsupported format, and a version no compiler claims; `2` the invocation or the +filesystem was wrong — a bad flag or argument, a spec that could not be read, an output that could +not be written. Nothing about the spec's own contents reaches `2`. ### Library diff --git a/cmd/morphic/compile.go b/cmd/morphic/compile.go index bad5c41f..8f4c5aa9 100644 --- a/cmd/morphic/compile.go +++ b/cmd/morphic/compile.go @@ -200,19 +200,32 @@ func parseArgs(fs *flag.FlagSet, args []string) ([]string, error) { } // renderDiagnostics writes each diagnostic to w, one per line, as -// " #: ". This is the sole place in -// the pipeline where diagnostics are rendered for a human. +// " : ". This is the sole place in the +// pipeline where diagnostics are rendered for a human. func renderDiagnostics(w io.Writer, res *engine.Result) { for _, d := range res.Diagnostics { - if path := sourcePath(res.Document, d.Provenance.Source); path != "" { - emitf(w, "%s %s %s#%s: %s\n", - d.Severity, d.Code, path, d.Provenance.Pointer, d.Message) - continue - } - // No source file (e.g. a pass diagnostic whose pointer is an IR id): show - // the bare pointer rather than fabricating a location in the spec file. - emitf(w, "%s %s %s: %s\n", d.Severity, d.Code, d.Provenance.Pointer, d.Message) + emitf(w, "%s %s%s: %s\n", + d.Severity, d.Code, location(res.Document, d.Provenance), d.Message) + } +} + +// location renders the "where" of a diagnostic line, leading space included: +// " #" when the provenance resolves to a source file, +// " " when it names only an IR-space position (a pass diagnostic whose +// pointer is an IR id), and nothing at all when it names neither. +// +// The empty case is what a diagnostic raised before any document existed +// carries — an unrecognized spec format has no position inside a spec that was +// never lowered — and printing nothing is the point: any location shown there +// would be one the finding is not about. +func location(doc *ir.Document, prov ir.Provenance) string { + if path := sourcePath(doc, prov.Source); path != "" { + return " " + path + "#" + prov.Pointer + } + if prov.Pointer != "" { + return " " + prov.Pointer } + return "" } // sourcePath resolves a diagnostic's source index to its file path, returning diff --git a/cmd/morphic/compile_test.go b/cmd/morphic/compile_test.go index 0ec71571..8ef62aaa 100644 --- a/cmd/morphic/compile_test.go +++ b/cmd/morphic/compile_test.go @@ -39,13 +39,43 @@ func TestRun_ParseWritesIRToFile(t *testing.T) { assert.True(t, bytes.HasSuffix(raw, []byte("\n"))) } -func TestRun_ParseUnknownSpecFails(t *testing.T) { +// TestRun_SpecProblemsExitOne covers the classes of bad spec the pipeline +// rejects before a compiler ever runs. Every one of them is a problem with the +// file the user named, so every one is exit 1 with a coded diagnostic — the same +// answer a bad `$ref` inside a spec the compiler does accept already gets. +// +// Exit 2 is reserved for a misuse of the CLI and for I/O that failed, so a CI +// wrapper can tell "your spec is broken" from "you invoked morphic wrong". A +// spec the tool read and understood well enough to name the problem in is +// neither of those, and reporting it as one made the compiler's own +// openapi/unsupported-version diagnostic unreachable from the shipped binary. +func TestRun_SpecProblemsExitOne(t *testing.T) { t.Parallel() - spec := writeFile(t, "junk.yaml", "hello: world\n") - 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") + tests := []struct { + name, contents, code string + }{ + {"unrecognized format", "hello: world\n", "engine/unrecognized-format"}, + {"swagger 2.0", "swagger: \"2.0\"\n", "engine/unsupported-format"}, + {"undecodable source", "openapi: [unterminated\n", "engine/undecodable-source"}, + {"no compiler for version", "openapi: 4.0.0\ninfo: {title: T, version: \"1\"}\npaths: {}\n", + "engine/no-compiler-for-format"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + spec := writeFile(t, "spec.yaml", tt.contents) + var stdout, stderr bytes.Buffer + + code := run([]string{"compile", spec}, &stdout, &stderr) + + assert.Equal(t, 1, code, "stderr: %s", stderr.String()) + assert.Contains(t, stderr.String(), tt.code) + assert.Empty(t, stdout.String(), "no IR JSON for a spec that produced no document") + assert.NotContains(t, stderr.String(), "usage:", + "a bad spec is not a misuse of the CLI, so no usage block") + }) + } } func TestRun_DiagnosticsGateExitCode(t *testing.T) { diff --git a/cmd/morphic/edgecases_test.go b/cmd/morphic/edgecases_test.go index 3e3c36d7..1da06c21 100644 --- a/cmd/morphic/edgecases_test.go +++ b/cmd/morphic/edgecases_test.go @@ -258,15 +258,24 @@ func TestRenderDiagnostics_WithAndWithoutSourcePath(t *testing.T) { Message: "no source file", Provenance: ir.Provenance{Source: 99, Pointer: "type:abc"}, }, + { + Severity: ir.SeverityError, + Code: "engine/unrecognized-format", + Message: "no position at all", + Provenance: ir.Provenance{Source: ir.NoSource}, + }, }, } var buf bytes.Buffer renderDiagnostics(&buf, res) - out := buf.String() - assert.Contains(t, out, "error openapi/bad spec.yaml#/paths/~1x: resolved location") - assert.Contains(t, out, "warning ir/dangling type:abc: no source file") + // Whole lines, not fragments: the location is what varies between these three + // forms, and a containment check on the message alone passes for a line that + // renders the location wrongly or fabricates one. + assert.Equal(t, "error openapi/bad spec.yaml#/paths/~1x: resolved location\n"+ + "warning ir/dangling type:abc: no source file\n"+ + "error engine/unrecognized-format: no position at all\n", buf.String()) } func TestWriteDocument_MarshalError(t *testing.T) { diff --git a/compilers/compilers.go b/compilers/compilers.go index dc8401b2..c782bb8f 100644 --- a/compilers/compilers.go +++ b/compilers/compilers.go @@ -35,6 +35,12 @@ type Options struct { // 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. +// +// A compiler reports through the returned slice. It may also store the same +// findings on the Document it returns — that copy is what the persisted IR JSON +// carries — but nothing obliges it to, and one that fills both must fill them +// alike. Neither list is guaranteed to hold the other, so a caller holding both +// unions them, as the engine does, rather than take one for the whole set. type Compiler interface { Formats() []SourceFormat Compile(ctx context.Context, sources []Source, opts Options) (*ir.Document, []ir.Diagnostic, error) diff --git a/engine/diag.go b/engine/diag.go new file mode 100644 index 00000000..4180ffa1 --- /dev/null +++ b/engine/diag.go @@ -0,0 +1,79 @@ +package engine + +import ( + "fmt" + + "github.com/dexpace/morphic/ir" +) + +// Diagnostic codes the engine itself raises, in its own stable namespace beside +// the compilers' (openapi/*) and the passes' (ir/*), so a CI wrapper can +// allowlist them by name. +// +// Each reports a problem with the source the caller named, not with the call. +// They exist because the pipeline can reject a spec before any compiler runs, +// and a rejection is a finding about the spec whichever stage makes it. A Go +// error out of Run means something other than the spec went wrong: the file +// could not be read, or a compiler broke its own contract. +const ( + // codeUndecodableSource: the bytes parse as neither YAML nor JSON, so nothing + // can be read out of them, a format key included. + codeUndecodableSource = "engine/undecodable-source" + // codeUnrecognizedFormat: the source decoded but declares no key any compiler + // in this tree announces itself by. + codeUnrecognizedFormat = "engine/unrecognized-format" + // codeUnsupportedFormat: the source declares a format Morphic recognizes and + // cannot lower yet — Swagger 2.0 today. + codeUnsupportedFormat = "engine/unsupported-format" + // codeNoCompilerForFormat: the format was read, but no registered compiler + // claims it. An OpenAPI version outside the supported range lands here. + codeNoCompilerForFormat = "engine/no-compiler-for-format" +) + +// specProblem builds an error-severity diagnostic about a source as a whole. +// Error is the severity because nothing was lowered: the spec reached the IR in +// no form at all. +// +// The provenance is deliberately positionless. These are raised before anything +// is lowered, so there is no document whose Sources table an index could +// address, and ir.NoSource is the IR's value for a finding that names no entry +// in one. An index invented here would have a renderer point at a file the +// finding is not about. +func specProblem(code, format string, args ...any) ir.Diagnostic { + return ir.NewDiagnostic(ir.SeverityError, code, fmt.Sprintf(format, args...), + ir.Provenance{Source: ir.NoSource}) +} + +// mergeDiagnostics returns stored followed by every diagnostic in produced that +// stored does not already hold. Identity is the whole value — severity, code, +// message and provenance — the same identity compilers/compile dedupes on. +// +// It exists because a compiler hands its findings back on two channels and is +// obliged to fill only one of them. Assigning either list over the other loses +// whatever the loser held, in silence and at every severity; merging keeps both +// and hands a compiler that fills them alike, as the OpenAPI one does, back +// exactly what it gave. +// +// The merged slice is always freshly allocated when there is anything to merge: +// stored and produced routinely alias one another, and appending into a shared +// backing array would overwrite entries still to be read. +func mergeDiagnostics(stored, produced []ir.Diagnostic) []ir.Diagnostic { + if len(produced) == 0 { + return stored + } + + held := make(map[ir.Diagnostic]struct{}, len(stored)) + for _, d := range stored { + held[d] = struct{}{} + } + + merged := make([]ir.Diagnostic, len(stored), len(stored)+len(produced)) + copy(merged, stored) + for _, d := range produced { + if _, dup := held[d]; dup { + continue + } + merged = append(merged, d) + } + return merged +} diff --git a/engine/engine.go b/engine/engine.go index 2885cf07..a0e93f9f 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -19,8 +19,11 @@ type RunOptions struct { } // 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. +// is a legal outcome and the shape every refusal takes — a source no compiler +// claims, or one a compiler declined to lower; the caller decides what is fatal. +// +// Diagnostics is the whole list for the run. When Document is non-nil it holds +// the same values, so a caller reading either channel sees every finding. type Result struct { Document *ir.Document `json:"document,omitempty"` Diagnostics []ir.Diagnostic `json:"diagnostics,omitempty"` @@ -58,20 +61,28 @@ func NewWith(fronts ...compilers.Compiler) (*Engine, error) { // 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. +// validate pass's diagnostics. +// +// The Go error return is reserved for I/O and programmer errors: the file could +// not be read, or a compiler failed in a way its own contract calls an error. +// Everything wrong with the spec itself comes back as a diagnostic in the +// Result, a source no compiler can lower included. A caller that treats a Go +// 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) { 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) + format, problem, ok := Sniff(data) if !ok { - return nil, fmt.Errorf("engine: no compiler registered for format %s", format) + return &Result{Diagnostics: []ir.Diagnostic{problem}}, nil + } + front, registered := e.registry.Lookup(format) + if !registered { + return &Result{Format: format, Diagnostics: []ir.Diagnostic{ + specProblem(codeNoCompilerForFormat, "no compiler registered for format %s", format), + }}, nil } doc, diags, err := front.Compile(ctx, []compilers.Source{{Path: specPath, Data: data}}, @@ -79,12 +90,16 @@ func (e *Engine) Run(ctx context.Context, specPath string, opts RunOptions) (*Re if err != nil { return nil, fmt.Errorf("engine: parse %q: %w", specPath, err) } - if !opts.SkipValidate && doc != nil { - // Land the pass diagnostics in the document too, so the persisted IR JSON - // (golden snapshots, IR diff, caches, emitters) carries them and does not - // silently lose error-level validation findings. - doc.Diagnostics = append(doc.Diagnostics, pass.Validate(doc)...) - diags = doc.Diagnostics + if doc == nil { + return &Result{Diagnostics: diags, Format: format}, nil + } + if !opts.SkipValidate { + diags = append(diags, pass.Validate(doc)...) } - return &Result{Document: doc, Diagnostics: diags, Format: format}, nil + // Both channels end up carrying the whole list: the Result is what a caller + // gates on, and the document is what gets persisted (golden snapshots, IR + // diff, caches, emitters). Merging rather than picking one is what keeps a + // finding its compiler put on only one of them — see mergeDiagnostics. + doc.Diagnostics = mergeDiagnostics(doc.Diagnostics, diags) + return &Result{Document: doc, Diagnostics: doc.Diagnostics, Format: format}, nil } diff --git a/engine/engine_test.go b/engine/engine_test.go index fd7fe6c5..8f8e2a88 100644 --- a/engine/engine_test.go +++ b/engine/engine_test.go @@ -2,10 +2,13 @@ package engine_test import ( "context" + "fmt" "os" "path/filepath" + "slices" "testing" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -97,14 +100,39 @@ func TestEngine_RunMissingFile(t *testing.T) { require.Error(t, err) } -func TestEngine_RunSniffError(t *testing.T) { +// TestEngine_RunSniffProblemsAreDiagnostics covers every way a source can defeat +// the sniff step. None of them is an I/O failure or a programmer error, so none +// may leave Run as a Go error: a caller that maps Go errors to "you invoked me +// wrong" — which the CLI does — would report a spec it read and understood well +// enough to name the problem in as a misuse of itself. +func TestEngine_RunSniffProblemsAreDiagnostics(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{}) - require.Error(t, err) - assert.Contains(t, err.Error(), "engine: sniff") + tests := []struct { + name, spec, code string + }{ + {"swagger 2.0", "swagger: \"2.0\"\n", "engine/unsupported-format"}, + {"unrecognized", "hello: world\n", "engine/unrecognized-format"}, + {"undecodable", "openapi: [unterminated\n", "engine/undecodable-source"}, + } + 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, "a spec problem is not a Go error") + require.NotNil(t, res) + assert.Nil(t, res.Document, "nothing was lowered") + require.Len(t, res.Diagnostics, 1) + assert.Equal(t, tt.code, res.Diagnostics[0].Code) + assert.Equal(t, ir.SeverityError, res.Diagnostics[0].Severity) + assert.Equal(t, ir.NoSource, res.Diagnostics[0].Provenance.Source, + "the engine read a file it never lowered, so it can index no source table") + assert.Equal(t, compilers.SourceFormat{}, res.Format, "no format was determined") + }) + } } func TestEngine_RunLookupMiss(t *testing.T) { @@ -115,9 +143,17 @@ func TestEngine_RunLookupMiss(t *testing.T) { // 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") + + res, err := eng.Run(t.Context(), writeSpec(t, testspec.Tiny), engine.RunOptions{}) + + 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) + assert.Equal(t, "engine/no-compiler-for-format", res.Diagnostics[0].Code) + assert.Contains(t, res.Diagnostics[0].Message, "openapi@3.1") + assert.Equal(t, compilers.SourceFormat{Name: "openapi", Version: "3.1"}, res.Format, + "the format that found no compiler is still the one the source declared") } // collidingCompiler claims a single fixed format. Two of them registered @@ -163,6 +199,11 @@ 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. +// +// It is not a stand-in for a compiler whose two diagnostic channels disagree: +// a nil Document short-circuits the validate step entirely, so this stub never +// reaches the code that folds the two together. splitDiagCompiler is what +// covers that. type nilDocCompiler struct{} func (nilDocCompiler) Formats() []compilers.SourceFormat { @@ -184,6 +225,78 @@ func TestEngine_RunNilDocument(t *testing.T) { assert.True(t, hasDiagCode(res.Diagnostics, "x/none")) } +// splitDiagCompiler claims openapi 3.1 and lowers to a non-nil Document whose +// stored and returned diagnostics are set independently, so one finding can be +// put on either channel alone. +// +// All three splits the test below builds are conforming compilers: the +// compilers.Compiler contract names the returned slice as what a compiler +// reports through and leaves storing the same values on the document optional. +// The one compiler in the tree happens to do both, which is why nothing else +// here notices when the engine keeps only one of the two lists. +type splitDiagCompiler struct{ stored, returned []ir.Diagnostic } + +func (splitDiagCompiler) Formats() []compilers.SourceFormat { + return []compilers.SourceFormat{{Name: "openapi", Version: "3.1"}} +} + +func (c splitDiagCompiler) Compile(context.Context, []compilers.Source, compilers.Options) (*ir.Document, []ir.Diagnostic, error) { + // A fresh document and fresh slices per call: Run writes the merged list onto + // the document, so shared ones would carry one run's result into the next. + return &ir.Document{ + IRVersion: "1.0.0", + Types: ir.TypeRegistry{}, + Diagnostics: slices.Clone(c.stored), + }, slices.Clone(c.returned), nil +} + +// TestEngine_RunKeepsDiagnosticsFromEitherChannel pins that neither diagnostic +// channel is dropped in favour of the other. Three of these six rows lost a +// finding before this was fixed — an error-severity one, in silence, on a +// different combination of channel and validate mode each time. +// +// The returned-only row with the validate pass enabled — the default — is the +// worst of them, and the reason the modes are a loop rather than a single case. +// Assigning Document.Diagnostics over the returned list emptied the Result the +// CLI gates its exit code on, so turning validation on *removed* findings and +// the tool exited 0 on a spec its compiler had refused outright. +func TestEngine_RunKeepsDiagnosticsFromEitherChannel(t *testing.T) { + t.Parallel() + want := ir.Diagnostic{ + Severity: ir.SeverityError, + Code: "stub/spec-problem", + Message: "the compiler reported this", + } + fronts := []struct { + name string + front splitDiagCompiler + }{ + {"mirrored", splitDiagCompiler{stored: []ir.Diagnostic{want}, returned: []ir.Diagnostic{want}}}, + {"returned only", splitDiagCompiler{returned: []ir.Diagnostic{want}}}, + {"stored only", splitDiagCompiler{stored: []ir.Diagnostic{want}}}, + } + for _, tt := range fronts { + for _, skipValidate := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/skip-validate=%v", tt.name, skipValidate), func(t *testing.T) { + t.Parallel() + eng, err := engine.NewWith(tt.front) + require.NoError(t, err) + + res, err := eng.Run(t.Context(), writeSpec(t, testspec.Tiny), + engine.RunOptions{SkipValidate: skipValidate}) + + require.NoError(t, err) + require.NotNil(t, res.Document, + "a non-nil document is the whole point: a nil one skips the fold under test") + assert.Empty(t, cmp.Diff([]ir.Diagnostic{want}, res.Diagnostics), + "Result.Diagnostics is what the CLI renders and gates its exit code on") + assert.Empty(t, cmp.Diff([]ir.Diagnostic{want}, res.Document.Diagnostics), + "Document.Diagnostics is what the persisted IR JSON carries") + }) + } + } +} + // danglingCompiler is a stub that always lowers to a Document containing a // 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 diff --git a/engine/sniff.go b/engine/sniff.go index c1f8edd7..78cf7d67 100644 --- a/engine/sniff.go +++ b/engine/sniff.go @@ -1,11 +1,10 @@ package engine import ( - "fmt" - yaml "gopkg.in/yaml.v3" "github.com/dexpace/morphic/compilers" + "github.com/dexpace/morphic/ir" ) // sniffProbe holds the two discriminating keys read from the source bytes. YAML @@ -17,21 +16,29 @@ type sniffProbe struct { // 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) { +// prefix; `swagger: "2.0"` is recognized but not lowerable yet; anything else, +// undecodable bytes included, yields no format. +// +// ok reports whether a format was read, and diag says why not when it was not. +// There is no Go error return because there is nothing here for one to carry: +// every way a source can defeat Sniff is a problem with that source, and this +// pipeline reports those as diagnostics. +func Sniff(data []byte) (format compilers.SourceFormat, diag ir.Diagnostic, ok bool) { var probe sniffProbe if err := yaml.Unmarshal(data, &probe); err != nil { - return compilers.SourceFormat{}, fmt.Errorf("sniff: decode source: %w", err) + return compilers.SourceFormat{}, + specProblem(codeUndecodableSource, "decode source: %v", err), false } switch { case probe.OpenAPI != "": - return compilers.SourceFormat{Name: "openapi", Version: majorMinor(probe.OpenAPI)}, nil + return compilers.SourceFormat{Name: "openapi", Version: majorMinor(probe.OpenAPI)}, + ir.Diagnostic{}, true case probe.Swagger != "": - return compilers.SourceFormat{}, fmt.Errorf( - "swagger 2.0 is not supported yet (planned: lift into the openapi compiler)") + return compilers.SourceFormat{}, specProblem(codeUnsupportedFormat, + "swagger 2.0 is not supported yet (planned: lift into the openapi compiler)"), false default: - return compilers.SourceFormat{}, fmt.Errorf("unrecognized spec format") + return compilers.SourceFormat{}, + specProblem(codeUnrecognizedFormat, "unrecognized spec format"), false } } diff --git a/engine/sniff_test.go b/engine/sniff_test.go index f5aff368..5e242365 100644 --- a/engine/sniff_test.go +++ b/engine/sniff_test.go @@ -8,6 +8,7 @@ import ( "github.com/dexpace/morphic/compilers" "github.com/dexpace/morphic/engine" + "github.com/dexpace/morphic/ir" ) func TestSniff_Formats(t *testing.T) { @@ -15,7 +16,7 @@ func TestSniff_Formats(t *testing.T) { cases := []struct { name, src string want compilers.SourceFormat - wantErr string + wantCode 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"}, ""}, @@ -23,23 +24,30 @@ func TestSniff_Formats(t *testing.T) { // 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. + // A bare-major version (no dot) also reaches the passthrough return. Sniff + // reports what the source declared and judges none of it; an out-of-range + // version is a spec problem the registry lookup names, not this step's. {"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"}, + {"swagger", "swagger: \"2.0\"\n", compilers.SourceFormat{}, "engine/unsupported-format"}, + {"unknown", "hello: world\n", compilers.SourceFormat{}, "engine/unrecognized-format"}, + {"undecodable yaml", "openapi: [unterminated\n", compilers.SourceFormat{}, "engine/undecodable-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) + got, diag, ok := engine.Sniff([]byte(tc.src)) + + if tc.wantCode != "" { + require.False(t, ok, "a source Sniff cannot read a format from is not ok") + assert.Equal(t, tc.wantCode, diag.Code) + assert.Equal(t, ir.SeverityError, diag.Severity) + assert.NotEmpty(t, diag.Message, "a diagnostic has to say what is wrong") + assert.Equal(t, tc.want, got, "no format is reported alongside a refusal") return } - require.NoError(t, err) + require.True(t, ok, "diag: %+v", diag) assert.Equal(t, tc.want, got) + assert.Equal(t, ir.Diagnostic{}, diag, "a format that was read leaves nothing to report") }) } }