diff --git a/README.md b/README.md index ccb1d64c..fe21dcd0 100644 --- a/README.md +++ b/README.md @@ -123,6 +123,13 @@ Diagnostics print one per line as ` #: `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. +`1` and `2` can both be earned by one run — a spec that reached the threshold whose `-o` +destination then refused the write. The verdict on the spec wins, so `1` means what it says +whatever `-o` pointed at, and `2` means the run failed for a reason outside the spec. The write +error is printed on stderr either way. Note that `-o` publishes by rename, so a destination whose +directory will not take a temp file — `/dev/null`, a read-only directory — cannot be written to at +all. + ### Library The same pipeline is available as a package. `engine.New` builds the default registry (OpenAPI diff --git a/cmd/morphic/compile.go b/cmd/morphic/compile.go index bad5c41f..3f9a28be 100644 --- a/cmd/morphic/compile.go +++ b/cmd/morphic/compile.go @@ -167,15 +167,35 @@ func compileSpec(specPath string, opts compileOptions, stdout, stderr io.Writer) if res.Document == nil { return 1 } + + diagCode := exitCodeFor(res.Diagnostics, opts.failOn) if opts.explain != "" { explainDocument(stdout, res.Document, res.Diagnostics, opts.explain) - return exitCodeFor(res.Diagnostics, opts.failOn) + return diagCode } if err := writeCompiled(opts.outPath, stdout, res.Document); err != nil { emitf(stderr, "morphic: %v\n", err) - return 2 + return writeFailureExit(diagCode) } - return exitCodeFor(res.Diagnostics, opts.failOn) + return diagCode +} + +// writeFailureExit returns the exit code for a run whose diagnostics earned +// diagCode and whose output then could not be written. +// +// A failed write does not overwrite a non-zero diagCode. Whether a destination +// can be written at all is a property of the destination — /dev/null and a +// read-only directory both refuse the temp file replaceFile publishes through — +// not of the spec, so letting it decide the exit code made "the spec reached the +// --fail-on threshold" report 1 or 2 depending on where -o pointed. The verdict +// on the spec is the same either way and the write failure is on stderr either +// way, so the exit code keeps the verdict, and 2 is left to mean a run that +// failed for a reason outside the spec. +func writeFailureExit(diagCode int) int { + if diagCode != 0 { + return diagCode + } + return 2 } // parseArgs binds fs and collects positional arguments, tolerating flags that @@ -276,9 +296,16 @@ func writeCompiled(outPath string, stdout io.Writer, doc *ir.Document) error { // it, which is what makes the swap atomic and which costs four things a // truncating write gave for free. All four are accepted deliberately: // -// - Writing needs permission on the destination's directory, not just on the -// destination. Rewriting an existing writable file inside a read-only -// directory used to succeed and now fails at temp-file creation. +// - Writing needs a directory that will accept a new entry, not just a +// writable destination. Rewriting an existing writable file inside a +// read-only directory used to succeed and now fails at temp-file creation, +// and so does any destination whose directory refuses one — -o /dev/null +// most visibly, since /dev takes no temp file. Writing through to such a +// destination instead is deliberately not done here: honouring what a name +// points at rather than replacing the name is the same trade the symlink and +// hard-link entries below decline, and opening a reader-less FIFO for +// writing blocks indefinitely. What the failure must not do is decide the +// exit code — see writeFailureExit. // - A symlink at outPath is replaced by a regular file instead of being // followed and written through, so its target keeps its old content. // - Other hard links to outPath keep pointing at the old inode, and so keep diff --git a/cmd/morphic/edgecases_test.go b/cmd/morphic/edgecases_test.go index 3e3c36d7..f46fe291 100644 --- a/cmd/morphic/edgecases_test.go +++ b/cmd/morphic/edgecases_test.go @@ -187,6 +187,94 @@ func TestRunParse_OutputCreateError(t *testing.T) { assert.Contains(t, stderr.String(), "create output") } +// unresolvedRefSpec is an OpenAPI 3.1 document whose only $ref names a schema +// that is not there. It matters that it still lowers to a document: a spec that +// lowers to nil returns 1 before the -o write is ever attempted, so it could not +// exercise the combination writeFailureExit decides. +const unresolvedRefSpec = `openapi: 3.1.0 +info: {title: Bad, version: "1"} +paths: + /x: + get: + responses: + "200": + description: ok + content: + application/json: + schema: {$ref: "#/components/schemas/Missing"} +` + +// missingParentDest returns a destination inside a directory that is not there, +// so creating the temp file beside it fails. +func missingParentDest(t *testing.T) string { + t.Helper() + return filepath.Join(t.TempDir(), "missing-dir", "ir.json") +} + +// readOnlyDirDest returns a writable file inside a read-only directory — the +// shape TestWriteParsed_ReadOnlyDirFails pins, where the destination itself +// could be rewritten but its directory will not take the temp file. +func readOnlyDirDest(t *testing.T) string { + t.Helper() + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permission checks, so the write would succeed") + } + dir := t.TempDir() + out := filepath.Join(dir, "ir.json") + require.NoError(t, os.WriteFile(out, []byte("PREVIOUS\n"), 0o644)) + require.NoError(t, os.Chmod(dir, 0o555)) + // Restore write permission so t.TempDir's own cleanup can remove the tree. + t.Cleanup(func() { _ = os.Chmod(dir, 0o755) }) + return out +} + +// TestRunParse_WriteFailureDoesNotMaskDiagnostics pins that a destination which +// refuses the write does not decide the exit code. A spec that reached the +// --fail-on threshold exits 1 wherever -o pointed; only a run with nothing to +// report at the threshold exits 2 for the write. Both destinations here are +// refused for the same reason — replaceFile publishes by rename and their +// directories will not take a temp file — which is also why -o /dev/null fails, +// untestable here because a root /dev would take the temp file and the rename +// would then replace /dev/null itself. +func TestRunParse_WriteFailureDoesNotMaskDiagnostics(t *testing.T) { + t.Parallel() + tests := []struct { + name string + spec string + dest func(t *testing.T) string + wantCode int + // wantDiag is a diagnostic code stderr must carry, or "" when the spec is + // clean and stderr must carry no diagnostic at all. + wantDiag string + }{ + {"threshold reached, missing parent dir", unresolvedRefSpec, missingParentDest, 1, "openapi/unresolved-ref"}, + {"threshold reached, read-only dir", unresolvedRefSpec, readOnlyDirDest, 1, "openapi/unresolved-ref"}, + {"nothing reported, missing parent dir", testspec.Tiny, missingParentDest, 2, ""}, + {"nothing reported, read-only dir", testspec.Tiny, readOnlyDirDest, 2, ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + spec := writeFile(t, "spec.yaml", tt.spec) + var stdout, stderr bytes.Buffer + + code := run([]string{"compile", spec, "-o", tt.dest(t)}, &stdout, &stderr) + + assert.Equal(t, tt.wantCode, code, "stderr: %s", stderr.String()) + assert.Contains(t, stderr.String(), "create output", + "the destination must really refuse the write, or this case proves nothing") + if tt.wantDiag == "" { + assert.NotContains(t, stderr.String(), "openapi/", + "the clean spec must reach the write with no diagnostic behind it") + return + } + assert.Contains(t, stderr.String(), tt.wantDiag, + "a failed write must not swallow the diagnostics it was reported alongside") + }) + } +} + func TestExitCodeFor_SeverityMatrix(t *testing.T) { t.Parallel() tests := []struct {