diff --git a/README.md b/README.md index 7a23284..5a47629 100644 --- a/README.md +++ b/README.md @@ -120,9 +120,15 @@ Help always prints to stdout and exits `0`. | `-o ` | `compile` | Write IR JSON to `` instead of stdout. | | `--explain ` | `compile` | Report what compiling produced at this source coordinate instead of writing the document. | -Diagnostics print one per line as ` #: `. Both commands -use the same 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. + +Both commands use the same 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 7e6619b..a41a4d4 100644 --- a/cmd/morphic/compile.go +++ b/cmd/morphic/compile.go @@ -239,19 +239,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 0ec7157..8ef62aa 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 e582542..7cf3156 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/cmd/morphic/validate_test.go b/cmd/morphic/validate_test.go index cc51c4b..3c614e9 100644 --- a/cmd/morphic/validate_test.go +++ b/cmd/morphic/validate_test.go @@ -181,13 +181,17 @@ func TestRun_ValidateUsageErrors(t *testing.T) { func TestRun_ValidateEngineFailures(t *testing.T) { t.Parallel() + // The two rows are different classes and exit differently. A format nothing + // recognizes is a problem with the spec, so it is a diagnostic and exits 1; a + // file that cannot be read is a problem with the invocation, so it stays a Go + // error and exits 2. Nothing about a spec's own contents reaches 2. tests := []struct { name string spec string wantCode int wantErr string }{ - {"unrecognized spec format", "hello: world\n", 2, "unrecognized spec format"}, + {"unrecognized spec format", "hello: world\n", 1, "engine/unrecognized-format"}, {"unreadable spec file", "", 2, "morphic:"}, } diff --git a/compilers/compilers.go b/compilers/compilers.go index d0d9d12..f0078e2 100644 --- a/compilers/compilers.go +++ b/compilers/compilers.go @@ -37,6 +37,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 0000000..4180ffa --- /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 fcc1c74..6b52266 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"` @@ -59,20 +62,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}}, @@ -80,12 +91,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 39c19bd..a707c44 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 @@ -176,6 +212,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 { @@ -197,6 +238,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 c1f8edd..78cf7d6 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 f5aff36..5e24236 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") }) } }