Repository navigation
fix: repair reference resolution, bundling, and comparison regressions - #686
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #686 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 310 313 +3
Lines 39401 39557 +156
==========================================
+ Hits 39401 39557 +156
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
daveshanley
marked this pull request as ready for review
October 31, 2026 15:21
daveshanley
commented
Oct 31, 2026
daveshanley
left a comment
daveshanley
left a comment
Member
Author
There was a problem hiding this comment.
Nine issues closed, two contributors' work rescued, and a schema called
examplethat kills the process. Le sigh.
The bones of this PR are good. Merged with latest main, index, what-changed, bundler, datamodel, utils and root all pass, and Codecov is green at 4bf027e9. Joseph's and Luca's work holds up. But it can't ship yet.
Agent working this PR: this is your worklist. Do it in order. Each inline comment has the exact change and the test I want.
The worklist
- Blocker. Stop treating a schema named
exampleas example data (index/resolver_relatives.go). Circular references go undetected. - Blocker. Guard
scoreagainst recursion (what-changed/model/schema_comparison.go). Today it can die with a fatal stack overflow, andrecover()can't save us. - Blocker, performance. Gate the ancestry map in
extractRefs(index/extract_refs.go). Tested fix, index cost drops to 1-4%. - Blocker, performance. Bring concurrency back to compare (
what-changed/model/schema.go). Single core we're 11% faster thanmain, and with all cores we're 6-11% slower. - Blocker. Give the alias limit an opt-out (
index/extract_refs.go). A valid 607KB spec now returns a nil model. - Fatal that already exists on
main. See below. - Nit in
datamodel/low/integer_decimal.go, then refresh this PR description, because the CI link and head SHA are stale.
Already broken on main: fix it here
A path item $ref loop kills the process in both v2 and v3, at seekRefEnd in index/utility_methods.go. It's outside this diff, so it gets no inline comment. It's the same family of bug this PR repairs.
x-a: {$ref: '#/x-b'}
x-b: {$ref: '#/x-a'}
paths:
/a:
$ref: '#/x-a'Bound the recursion with a depth limit or a seen set, and report a build error instead. Add a v2 and a v3 test that both return an error and a process that's still alive.
How we keep this fast
Speed is the whole point of libopenapi. No merge until every number below is at or under main. "Noisy" is not an excuse, and I've used it once already on this PR.
| Benchmark (median, ms) | main base |
This PR | Change |
|---|---|---|---|
| Index, 4.4MB YAML fixture | 27.86 | 31.64 | +13.6% |
| Index, 4.7MB JSON fixture | 15.97 | 16.89 | +5.8% |
| Index, 400KB YAML fixture | 3.52 | 3.79 | +7.7% |
CompareDocuments, hashes cached |
96.4 | 106.7 | +10.7% |
CompareDocuments, fresh models |
178.5 | 189.8 | +6.3% |
Full model build came out flat on medians and +2-3% on best runs, so that one gets re-measured after the index fix. Compare memory is up 5%.
The rules for measuring:
- Benchmark the merge-base against this branch, with nothing else running on the machine.
- Alternate the runs (base, branch, base, branch) for at least 6 rounds. Short runs lie. A 2-iteration run told me +61% on the same code that measures +14% here.
- Report median and best run, plus B/op and allocs/op.
- Use
-raceand a repeated-run determinism check for anything concurrent. - Post the final table in this PR.
Worth a release note
TotalBreakingChanges no longer double counts allOf, and a retargeted $ref now reports nested changes. Both are correct, and both will move the numbers people see in openapi-changes and vacuum.
Get that list done and she's ready.
When a schema $ref was replaced by an inline schema, an inline schema by a $ref, or a $ref pointed somewhere else, CompareSchemas recorded a non-breaking $ref change and returned, so changes in the new schema were never reported. A $ref to a schema with type: int becoming an inline type: string was a single non-breaking change. Keep the $ref change, then compare the two schemas as it would compare two inline schemas. It still stops at the $ref change, now keeping that change instead of returning nil, when either side doesn't resolve, is on a reference loop, or comes from a document that was not checked for circular references (a loop can't be recognised there), and when a 3.1 $ref with sibling keywords is involved, since its internal allOf form doesn't line up with the referenced schema field by field. Rolodex gains HasCheckedForCircularReferences for that check. The helpers that recognise equivalent forms now look through a reference, so a change made through a $ref is reported like the same inline change: the object composition handling from #650 ($ref: Thing becoming anyOf: [$ref: Thing, type: 'null']), the simple allOf view, and a type array matching an anyOf of scalar types. Fixes #663
daveshanley
force-pushed
the
codex/october-issue-repairs
branch
from
October 31, 2026 21:05
4bf027e to
90e6e07
Compare
Member
Author
|
Performance results for Six alternating main/PR pairs, separate processes, Go 1.26.4, Apple M4 Max, 16 logical CPUs, 2 seconds per benchmark. Both binaries use the same harness and fixtures. Interrupted samples were discarded; the final samples all passed.
Indexing is faster on all three fixtures. Fresh and cached comparison are faster and use less memory than main, which meets the comparison review's time/memory requirement. The full-build results are mixed: Kubernetes is 0.29% faster; Stripe's median is 0.59% slower, although its best sample is faster. Asana indexing uses approximately 95 more B/op and 2 more allocs/op. These measurements do not establish that every operation is at or below main on every metric. The full-build harness uses Swagger 2 for Kubernetes and OpenAPI 3 for Stripe. Stripe's known circular-reference warning is allowed only when a model is returned. Fresh comparison clears the hash cache and builds both models outside each timed iteration; its one-time fixture preparation is included. Cached comparison warms the comparison before resetting the timer. Validation on this head: all 31,372 statements covered, zero uncovered blocks; Linux coverage, full race suite, and Windows tests pass. CodeQL passes. Codecov reports 100.00% project and 100.00% patch coverage. Identical benchmark harness used for both revisionsRun each binary from its corresponding checkout: ./benchmark.test -test.run '^$' -test.bench '^BenchmarkP(Index|Build|CompareFresh|CompareCached)$' -test.benchmem -test.benchtime=2s -test.count=1package libopenapi
import (
"os"
"strings"
"testing"
"github.com/pb33f/go-yaml"
"github.com/pb33f/libopenapi/datamodel/low"
"github.com/pb33f/libopenapi/index"
"github.com/pb33f/libopenapi/what-changed/model"
)
func BenchmarkPIndex(b *testing.B) {
for _, f := range []string{"stripe.yaml", "k8s.json", "asana.yaml"} {
data, _ := os.ReadFile("test_specs/" + f)
b.Run(f, func(b *testing.B) {
var root yaml.Node
_ = yaml.Unmarshal(data, &root)
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
_ = index.NewSpecIndexWithConfig(&root, index.CreateClosedAPIIndexConfig())
}
})
}
}
func BenchmarkPBuild(b *testing.B) {
for _, f := range []string{"stripe.yaml", "k8s.json"} {
data, _ := os.ReadFile("test_specs/" + f)
b.Run(f, func(b *testing.B) {
b.ReportAllocs()
for i := 0; i < b.N; i++ {
d, err := NewDocument(data)
if err != nil {
b.Fatal(err)
}
var built bool
if f == "k8s.json" {
model, buildErr := d.BuildV2Model()
built, err = model != nil, buildErr
} else {
model, buildErr := d.BuildV3Model()
built, err = model != nil, buildErr
}
// Stripe reports known circular references with a usable model.
if !built { b.Fatal(err) }
}
})
}
}
func BenchmarkPCompareFresh(b *testing.B) {
data, _ := os.ReadFile("test_specs/stripe.yaml")
mod := []byte(strings.ReplaceAll(string(data), "maxLength: 5000", "maxLength: 4000"))
b.ReportAllocs()
for i := 0; i < b.N; i++ {
b.StopTimer()
low.ClearHashCache()
l, _ := NewDocument(data)
r, _ := NewDocument(mod)
lm, _ := l.BuildV3Model()
rm, _ := r.BuildV3Model()
b.StartTimer()
_ = model.CompareDocuments(lm.Model.GoLow(), rm.Model.GoLow())
}
}
func BenchmarkPCompareCached(b *testing.B) {
data, _ := os.ReadFile("test_specs/stripe.yaml")
mod := []byte(strings.ReplaceAll(string(data), "maxLength: 5000", "maxLength: 4000"))
l, _ := NewDocument(data)
r, _ := NewDocument(mod)
lm, _ := l.BuildV3Model()
rm, _ := r.BuildV3Model()
_ = model.CompareDocuments(lm.Model.GoLow(), rm.Model.GoLow())
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
_ = model.CompareDocuments(lm.Model.GoLow(), rm.Model.GoLow())
}
} |
Member
Author
|
Reran the benchmarks on the requested Main:
Remaining measured regression: Stripe rendering is 191.406 → 193.743 ms (+1.22%, p=0.015, n=6). This is the only timing increase that benchstat marks significant at its default 0.05 threshold. It is not evidence of across-the-board parity. Significant timing improvements: Mistral indexing (−4.92%), Stripe indexing (−5.45%), Stripe building (−1.74%), and comparison on Mistral (−2.93%), Neon (−2.01%), and Plaid (−0.88%). Other timing differences did not meet the default significance threshold. Comparison allocations in bytes fall on all six fixtures, by 0.51–5.67%. These are pipeline workloads: indexing excludes YAML parsing; Build includes parsing and model construction; Render repeatedly renders a built model; Compare includes parsing and building both documents; Bundle includes the full
Full benchstat outputReproducer: identical Go benchmark for both revisionsSet LIBOPENAPI_DEMO_SPEED_DIR=/path/to/demo/speed-test ./benchmark.test \
-test.run '^$' -test.bench '^BenchmarkDemoSpeed$' \
-test.benchmem -test.benchtime=1s -test.count=1package libopenapi_test
import (
"bytes"
"io"
"log/slog"
"os"
"path/filepath"
"testing"
"github.com/pb33f/go-yaml"
"github.com/pb33f/libopenapi"
"github.com/pb33f/libopenapi/bundler"
"github.com/pb33f/libopenapi/datamodel"
"github.com/pb33f/libopenapi/datamodel/low"
"github.com/pb33f/libopenapi/index"
)
func demoSpeedConfig() *datamodel.DocumentConfiguration {
return &datamodel.DocumentConfiguration{
IgnorePolymorphicCircularReferences: true,
IgnoreArrayCircularReferences: true,
Logger: slog.New(slog.NewTextHandler(io.Discard, nil)),
}
}
func demoSpeedDocument(b *testing.B, data []byte) libopenapi.Document {
b.Helper()
doc, err := libopenapi.NewDocumentWithConfiguration(data, demoSpeedConfig())
if err != nil {
b.Fatal(err)
}
return doc
}
func demoSpeedBuild(b *testing.B, data []byte) libopenapi.Document {
b.Helper()
doc := demoSpeedDocument(b, data)
m, err := doc.BuildV3Model()
if m == nil {
b.Fatalf("nil model: %v", err)
}
return doc
}
// BenchmarkDemoSpeed uses the user's full demo corpus and the same public
// pipeline operations as pipeline_bench_test.go. No fixture is changed on disk.
func BenchmarkDemoSpeed(b *testing.B) {
dir := os.Getenv("LIBOPENAPI_DEMO_SPEED_DIR")
if dir == "" {
b.Fatal("LIBOPENAPI_DEMO_SPEED_DIR must name demo/speed-test")
}
for _, file := range []string{"petstore.yaml", "mistral.yaml", "neon.yaml", "ld.yaml", "plaid.yml", "stripe.yaml"} {
data, err := os.ReadFile(filepath.Join(dir, file))
if err != nil {
b.Fatal(err)
}
b.Run(file, func(b *testing.B) {
b.Run("Index", func(b *testing.B) {
var root yaml.Node
if err := yaml.Unmarshal(data, &root); err != nil {
b.Fatal(err)
}
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
idx := index.NewSpecIndexWithConfig(&root, index.CreateClosedAPIIndexConfig())
if idx == nil {
b.Fatal("nil index")
}
}
})
b.Run("Build", func(b *testing.B) {
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
demoSpeedBuild(b, data)
}
})
b.Run("Render", func(b *testing.B) {
doc := demoSpeedBuild(b, data)
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
out, err := doc.Render()
if err != nil || len(out) == 0 {
b.Fatalf("render: %v", err)
}
}
})
b.Run("Compare", func(b *testing.B) {
// Use the mutation from the existing pipeline benchmark. Assert that
// every corpus member produces real changes, not an identity comparison.
right := bytes.Replace(data, []byte("type: string"), []byte("type: integer"), 200)
right = bytes.Replace(right, []byte("nullable: true"), []byte("nullable: false"), 300)
if bytes.Equal(data, right) {
b.Fatal("mutation made no changes")
}
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
b.StopTimer()
low.ClearHashCache()
b.StartTimer()
leftDoc, rightDoc := demoSpeedDocument(b, data), demoSpeedDocument(b, right)
changes, err := libopenapi.CompareDocuments(leftDoc, rightDoc)
b.StopTimer()
if changes == nil || changes.TotalChanges() == 0 {
b.Fatalf("no changes: %v", err)
}
b.ReportMetric(float64(changes.TotalChanges()), "changes/op")
b.StartTimer()
}
b.StopTimer()
low.ClearHashCache()
})
b.Run("Bundle", func(b *testing.B) {
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
out, err := bundler.BundleBytes(data, demoSpeedConfig())
if len(out) == 0 {
b.Fatalf("empty bundle: %v", err)
}
}
})
})
}
}Fixture sizes and SHA-256 hashes{
"ld.yaml": {
"bytes": 1927409,
"sha256": "6e282f8852fe1d38527dfb7b6e90c27fe9505d66ed90f87c5b6a30fd8594e86c"
},
"mistral.yaml": {
"bytes": 292380,
"sha256": "37659aafcad6a8219609636db74a7481b24b6cbb0a54f6d184278b584f5269df"
},
"neon.yaml": {
"bytes": 369544,
"sha256": "958d5c6f86970418ad104dd87e7efb2185ef0ef16bd25c2502c382f82cd64a1b"
},
"petstore.yaml": {
"bytes": 21302,
"sha256": "ef2b2dea0ef6a09ba6247017ffd5a6405c689ec9b11e73d1b61b6e2047ce35e8"
},
"plaid.yml": {
"bytes": 2870618,
"sha256": "8addecd0b74aeb23ae844b349d8a4a2e1cbbb7ed2210f57d31cf644c37af7fa3"
},
"stripe.yaml": {
"bytes": 6051965,
"sha256": "ae4d4352c52ef5246904e15bacc639fd381494022f19eab6f74b36f47c85a1b4"
}
}
|
Member
Author
|
Vacuum compatibility is fixed at Unmodified Vacuum main All six CI passes Linux coverage, the full race suite, and Windows tests. CodeQL passes. Codecov confirms 100% project and 100% patch coverage. Local coverage is 31,437/31,437 statements. Fresh performance comparison: main Geometric means of per-spec median time ratios: indexing −4.02%, building −1.31%, rendering −0.33%, end-to-end comparison −2.42%, bundling −0.81%. These are descriptive aggregates, not significance claims. Stripe rendering is +1.02% (191.361 → 193.303 ms, p=0.394); Stripe comparison is +0.04% and bundling +0.57%. Some accepted runs contain large timing outliers, especially Neon and Stripe indexing; they are retained in the statistics. This run does not prove every operation is faster or establish tight performance parity. It supersedes earlier timing numbers for the previous PR head.
Complete benchstat results |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reference-heavy documents can lose schema fields, produce incomplete bundles, or panic. This repairs the remaining October reports and the additional defects found during review.
The original contributor commits from #668 and #673 are retained. This PR replaces those two proposals after their review findings are fixed. PRs #669, #675, #677, and #685 were merged separately and are now included in this branch's base.
Fixes #663.
Fixes #672.
Fixes #678.
Fixes #679.
Fixes #680.
Fixes #681.
Fixes #682.
Fixes #683.
Fixes #684.
Repeated resolved schema pairs are reported once within a comparison; local reference changes remain visible. Separate public comparisons do not share this state. Excessive YAML alias or inherited-merge expansion returns a build error instead of an incomplete model.
Review repairs:
AllowExcessiveAliasesto both configuration types for trusted documents; cycle guards remain enabled.Release notes:
TotalBreakingChanges()counts eachallOfchange once. Retargeted references now report changes inside the target as well as the reference change; repeated resolved pairs contribute details once per public comparison. Empty schema-change maps may be nil; JSON/YAML output and change counts are unchanged.Validation at
9d96a62c: all 35 local library test packages pass, covering all 31,437 statements with zero uncovered blocks. Index, bundler, and root document race tests pass. Hosted CI passes Linux coverage, the full race suite, and Windows tests; CodeQL also passes. Codecov confirms 100% project and 100% patch coverage. Independent correctness and code-quality reviews are complete, and their confirmed findings are fixed.Downstream compatibility is verified against unmodified Vacuum main
1fd76af2and openapi-changes main1750dae3, using isolated module replacements. Vacuum passes all 36 test packages, race checks formotorandfunctions/openapi, and 20 repeated race-enabled runs of the original LazyFS failure. openapi-changes passes all seven test packages and race checks forcmd,git, andinternal/changecounts.CLI checks cover all six
demo/speed-testspecs. Vacuum produces the same 39,248 findings as main;--originalfiltering matches exactly for Plaid (257) and Stripe (889). Circular-reference journeys reflect the corrected cycle paths. openapi-changes preserves change records apart from existing path-selection variability; Plaid's breaking summary removes duplicated allOf counts. Canonical JSON, summary, Markdown, and HTML workflows pass.Final-head performance and downstream report: six balanced pairs compare
9d96a62cwith mainc6910f28on the requested corpus. Geometric mean median time changes are indexing −4.02%, building −1.31%, rendering −0.33%, comparison −2.42%, and bundling −0.81%. Stripe rendering is 1.02% slower (191.361 → 193.303 ms; p=0.394). Timing outliers remain in the statistics, so the aggregate improvements do not establish uniform gains or tight performance parity. All 12 accepted runs pass and comparison change counts match. This report supersedes performance measurements for earlier PR heads.Risk: high because reference resolution and comparison affect several library entry points. Tests cover source context, repeated calls, cycles, alias reuse, shared filesystem ownership, and generated SDK behavior. No deployment or migration is required.