diff --git a/internal/corpus/corpus.go b/internal/corpus/corpus.go index 6c97c94..a1d3828 100644 --- a/internal/corpus/corpus.go +++ b/internal/corpus/corpus.go @@ -331,9 +331,11 @@ func verifyAgainstPin(id string, doc []byte, pin Pin, source string) error { return nil } - // Parsed only to describe the mismatch. What a reviewer needs is what - // changed, not that a hash differed. - version, paths, operations := describe(doc) + // The mismatch is named by digest, which is what proves it. A reader + // reaching this has no parser to hand — Ensure is called from a test + // helper, not from a caller that decoded the document first — so the + // description degrades rather than the failure being withheld. + version, paths, operations := unparsed(doc) return fmt.Errorf(`the pinned %s document is not what %s served. pinned sha256:%s version %s %d path(s) / %d operation(s) @@ -348,22 +350,25 @@ internal/corpus/testdata/corpus.lock.json`, id) } -// Describer parses a document enough to say what it is, for failure messages. +// Describer parses a document enough to say what it is. // -// A hook rather than a direct call on the OpenAPI parsing package, because -// that package's own tests are in-package and are among this package's most -// important consumers: importing it here would make them an import cycle. The -// CLI installs the real parser; anything that has not is describing a failure -// it is already reporting by digest. -var Describer = func([]byte) (version string, paths, operations int) { +// A parameter rather than a direct call on the OpenAPI parsing package, +// because that package's own tests are in-package and are among this +// package's most important consumers: importing it here would make them an +// import cycle. specmodel.Describe is the implementation to pass. +// +// Every caller that writes a measurement down takes one, so a pin cannot +// record what nothing measured. +type Describer func(doc []byte) (version string, paths, operations int) + +// unparsed is the describer for a caller with no parser to hand: it reports +// only what it can, which is nothing. Reserved for a failure message that +// already names the change by digest, and never for a value written to the +// lock. +func unparsed([]byte) (version string, paths, operations int) { return "unparsed", 0, 0 } -// describe reports what a document says about itself, best effort. -func describe(doc []byte) (version string, paths, operations int) { - return Describer(doc) -} - func shortSHA(s string) string { if len(s) <= 12 { return s diff --git a/internal/corpus/corpus_test.go b/internal/corpus/corpus_test.go index ea027f2..0064694 100644 --- a/internal/corpus/corpus_test.go +++ b/internal/corpus/corpus_test.go @@ -101,7 +101,35 @@ func TestUnit_Corpus_LockPinsTheDocumentsTheTestsRead(t *testing.T) { if pin.PinnedAt.IsZero() { t.Errorf("%s: no pinnedAt, so the cache directory name is not stable", id) } + if pin.Version == "" || pin.Version == "unparsed" { + t.Errorf("%s: version %q, so the pin records no measurement of what it pinned", id, pin.Version) + } + if ref := movingRef(pin.UpstreamURL); ref != "" { + t.Errorf("%s: the upstream URL names %q, a ref that moves; pin a commit or a tag instead", id, ref) + } + } +} + +// movingRefs are the branch names a source-hosting URL carries when it names +// the tip of a branch rather than a fixed revision. +var movingRefs = []string{"main", "master", "HEAD", "latest", "trunk", "develop"} + +// movingRef reports the moving ref an upstream URL names, empty when it names +// none. +// +// A pin whose URL follows a branch re-pins itself every time the vendor +// publishes: the hash stops matching, every test reading it fails, and the +// only remedy is to restate the pin, which is the review the lock exists to +// force. Pinning a revision makes the pin mean something. +func movingRef(upstream string) string { + for _, segment := range strings.Split(upstream, "/") { + for _, ref := range movingRefs { + if segment == ref { + return ref + } + } } + return "" } func slicesEqual(a, b []string) bool { @@ -434,3 +462,30 @@ func TestUnit_Corpus_NoUserCacheDirFallsBackToTempNotToRelative(t *testing.T) { t.Errorf("the fallback cache %q is not under tfpfgen/corpus", dir) } } + +// TestUnit_Corpus_AMovingRefIsRejected proves the guard above catches the +// shape it exists for, so it cannot pass by accident on a lock that happens +// to hold no branch URL. +func TestUnit_Corpus_AMovingRefIsRejected(t *testing.T) { + t.Parallel() + + for _, url := range []string{ + "https://raw.githubusercontent.com/o/r/main/spec.json", + "https://raw.githubusercontent.com/o/r/master/spec.json", + "https://example.invalid/latest/openapi.yaml", + } { + if movingRef(url) == "" { + t.Errorf("%s names a moving ref and was accepted", url) + } + } + + for _, url := range []string{ + "https://raw.githubusercontent.com/o/r/67c14c7efb01cdeeac0ecd8cee9fae8d7a80e2aa/spec.json", + "https://raw.githubusercontent.com/o/r/v2.1.0/spec.json", + "https://pubhub.devnetcloud.com/media/000-v7-apis/docs/reference/unified-oas/api.yaml", + } { + if ref := movingRef(url); ref != "" { + t.Errorf("%s names a fixed revision and was rejected as %q", url, ref) + } + } +} diff --git a/internal/corpus/refresh.go b/internal/corpus/refresh.go index 1b2b8df..74e1f3f 100644 --- a/internal/corpus/refresh.go +++ b/internal/corpus/refresh.go @@ -57,18 +57,18 @@ func (u Upstream) Describe() string { // It never writes and never consults the cache: the question is what the // vendor is serving right now, which a cache would answer wrongly by // construction. -func CheckUpstream(id string) (Upstream, error) { +func CheckUpstream(id string, describe Describer) (Upstream, error) { pin, err := PinFor(id) if err != nil { return Upstream{}, err } - return checkPin(pin) + return checkPin(pin, describe) } // checkPin is CheckUpstream with the pin supplied, so RewriteLock can measure // against the on-disk lock rather than the embedded one. -func checkPin(pin Pin) (Upstream, error) { +func checkPin(pin Pin, describe Describer) (Upstream, error) { doc, source, err := fetch(pin) if err != nil { return Upstream{}, err @@ -100,7 +100,7 @@ func checkPin(pin Pin) (Upstream, error) { // Pins are measured against the on-disk lock rather than the embedded copy: // the embedded bytes are whatever was compiled in, and a rewrite must not // silently discard an edit made since. -func RewriteLock(ids []string) error { +func RewriteLock(ids []string, describe Describer) error { path := LockPath() raw, err := os.ReadFile(path) //nolint:gosec // a fixed name under the repository @@ -134,7 +134,7 @@ func RewriteLock(ids []string) error { return fmt.Errorf("%s has no pin for %q", path, id) } - up, err := checkPin(pin) + up, err := checkPin(pin, describe) if err != nil { return fmt.Errorf("%s: %w", id, err) } diff --git a/internal/corpus/refresh_test.go b/internal/corpus/refresh_test.go index ed5e0a9..397b2a8 100644 --- a/internal/corpus/refresh_test.go +++ b/internal/corpus/refresh_test.go @@ -31,7 +31,7 @@ func TestUnit_Refresh_CheckUpstreamMeasuresWithoutJudging(t *testing.T) { url, _ := serve(t, aDocument) - matching, err := checkPin(pinFor(t, url, aDocument)) + matching, err := checkPin(pinFor(t, url, aDocument), unparsed) if err != nil { t.Fatalf("checkPin: %v", err) } @@ -40,7 +40,7 @@ func TestUnit_Refresh_CheckUpstreamMeasuresWithoutJudging(t *testing.T) { } moved := pinFor(t, url, "what the lock used to pin") - differing, err := checkPin(moved) + differing, err := checkPin(moved, unparsed) if err != nil { t.Fatalf("checkPin: %v", err) } @@ -60,12 +60,19 @@ func TestUnit_Refresh_CheckUpstreamMeasuresWithoutJudging(t *testing.T) { func TestUnit_Refresh_CheckUpstreamRefusesAnUnpinnedID(t *testing.T) { t.Parallel() - if _, err := CheckUpstream("no-such-document"); err == nil || + if _, err := CheckUpstream("no-such-document", unparsed); err == nil || !strings.Contains(err.Error(), "pins no document") { t.Fatalf("CheckUpstream of an unpinned id: %v", err) } } +// stubDescriber stands where a real parser goes, answering values no fixture +// could produce by accident so a pin carrying them proves the describer was +// consulted. +func stubDescriber([]byte) (version string, paths, operations int) { + return "9.9.9", 7, 11 +} + // writeLock writes a lock file for RewriteLock tests and points EnvLockPath at // it. func writeLock(t *testing.T, content string) string { @@ -118,7 +125,7 @@ func TestUnit_Refresh_RewriteLockRestatesOnlyThePinsThatMoved(t *testing.T) { } }`) - if err := RewriteLock([]string{"moved", "unmoved"}); err != nil { + if err := RewriteLock([]string{"moved", "unmoved"}, stubDescriber); err != nil { t.Fatalf("RewriteLock: %v", err) } @@ -147,9 +154,15 @@ func TestUnit_Refresh_RewriteLockRestatesOnlyThePinsThatMoved(t *testing.T) { if moved["unknownEntryKey"] != "kept too" { t.Error("rewriting dropped an entry key it does not own") } - // The default Describer cannot parse, and says so rather than guessing. - if moved["version"] != "unparsed" { - t.Errorf("the moved pin's version = %v", moved["version"]) + // The pin records what the describer measured. A rewrite that ignored it + // would write the old version back, or none at all, and either reads as + // a document that did not move. + if moved["version"] != "9.9.9" { + t.Errorf("the moved pin's version = %v, want the describer's measurement", moved["version"]) + } + if moved["pathCount"] != float64(7) || moved["operationCount"] != float64(11) { + t.Errorf("the moved pin's counts = %v/%v, want the describer's measurement", + moved["pathCount"], moved["operationCount"]) } unmoved := entries["unmoved"].(map[string]any) @@ -163,22 +176,22 @@ func TestUnit_Refresh_RewriteLockRestatesOnlyThePinsThatMoved(t *testing.T) { // and an unreachable upstream. None may write. func TestUnit_Refresh_RewriteLockFailsClosed(t *testing.T) { t.Setenv(EnvLockPath, filepath.Join(t.TempDir(), "absent", LockFile)) - if err := RewriteLock(nil); err == nil || !strings.Contains(err.Error(), "reading") { + if err := RewriteLock(nil, unparsed); err == nil || !strings.Contains(err.Error(), "reading") { t.Errorf("a missing lock: %v", err) } writeLock(t, "{not json") - if err := RewriteLock(nil); err == nil || !strings.Contains(err.Error(), "parsing") { + if err := RewriteLock(nil, unparsed); err == nil || !strings.Contains(err.Error(), "parsing") { t.Errorf("an unparsable lock: %v", err) } writeLock(t, `{"formatVersion": "1"}`) - if err := RewriteLock(nil); err == nil || !strings.Contains(err.Error(), "no openapi object") { + if err := RewriteLock(nil, unparsed); err == nil || !strings.Contains(err.Error(), "no openapi object") { t.Errorf("a lock with no openapi object: %v", err) } writeLock(t, `{"openapi": {}}`) - if err := RewriteLock([]string{"ghost"}); err == nil || !strings.Contains(err.Error(), `no pin for "ghost"`) { + if err := RewriteLock([]string{"ghost"}, unparsed); err == nil || !strings.Contains(err.Error(), `no pin for "ghost"`) { t.Errorf("an id the lock does not pin: %v", err) } @@ -194,7 +207,7 @@ func TestUnit_Refresh_RewriteLockFailsClosed(t *testing.T) { if err != nil { t.Fatal(err) } - if err := RewriteLock([]string{"dead"}); err == nil || !strings.Contains(err.Error(), "dead:") { + if err := RewriteLock([]string{"dead"}, unparsed); err == nil || !strings.Contains(err.Error(), "dead:") { t.Errorf("an unreachable upstream: %v", err) } after, err := os.ReadFile(path) //nolint:gosec // a path this test built diff --git a/internal/corpus/testdata/corpus.lock.json b/internal/corpus/testdata/corpus.lock.json index 4b5e2f0..5018aa3 100644 --- a/internal/corpus/testdata/corpus.lock.json +++ b/internal/corpus/testdata/corpus.lock.json @@ -7,9 +7,9 @@ "mirrorUrl": "", "operationCount": 1220, "pathCount": 808, - "pinnedAt": "2026-08-07T14:30:00Z", - "sha256": "80850db290cde4eb487e0efb587cf27f305e77b6bef96933ed8a09b5169d5b1d", - "upstreamUrl": "https://raw.githubusercontent.com/github/rest-api-description/main/descriptions/api.github.com/api.github.com.json", + "pinnedAt": "2026-08-14T16:14:25.482999Z", + "sha256": "81c0ff2bbff9099b569058ef387e0ef83c33ef00ca0346e8a092777a71c192e3", + "upstreamUrl": "https://raw.githubusercontent.com/github/rest-api-description/67c14c7efb01cdeeac0ecd8cee9fae8d7a80e2aa/descriptions/api.github.com/api.github.com.json", "version": "1.1.4" }, "thousandeyes": { diff --git a/internal/specmodel/describe.go b/internal/specmodel/describe.go new file mode 100644 index 0000000..d08db1f --- /dev/null +++ b/internal/specmodel/describe.go @@ -0,0 +1,18 @@ +package specmodel + +// Describe reports what a document says about itself: the version its info +// object declares, and how much surface it carries. +// +// It satisfies corpus.Describer, which is where a pin's recorded counts come +// from. Those counts are what catch a truncated download that happens to +// parse, so a document this cannot read reports zero rather than a guess. +func Describe(doc []byte) (version string, paths, operations int) { + loaded, err := Load(doc) + if err != nil { + return "", 0, 0 + } + for _, path := range loaded.Paths { + operations += len(path.Operations) + } + return loaded.Info.Version, len(loaded.Paths), operations +} diff --git a/internal/specmodel/describe_test.go b/internal/specmodel/describe_test.go new file mode 100644 index 0000000..51aa3b6 --- /dev/null +++ b/internal/specmodel/describe_test.go @@ -0,0 +1,34 @@ +package specmodel + +import "testing" + +// TestUnit_Specmodel_DescribeMeasuresWhatAPinRecords proves Describe reports +// the three values a corpus pin is written from, and reports nothing rather +// than a guess for bytes it cannot read — a truncated download must not pass +// as a document with a plausible shape. +func TestUnit_Specmodel_DescribeMeasuresWhatAPinRecords(t *testing.T) { + const doc = `openapi: 3.0.3 +info: {title: T, version: "4.5.6"} +paths: + /widgets: + get: + responses: + "200": {description: ok} + post: + responses: + "201": {description: made} + /widgets/{id}: + delete: + responses: + "204": {description: gone} +` + version, paths, operations := Describe([]byte(doc)) + if version != "4.5.6" || paths != 2 || operations != 3 { + t.Errorf("Describe = %q, %d paths, %d operations; want 4.5.6, 2, 3", version, paths, operations) + } + + version, paths, operations = Describe([]byte("\t{[ not a document")) + if version != "" || paths != 0 || operations != 0 { + t.Errorf("unreadable bytes described as %q, %d, %d", version, paths, operations) + } +}