Skip to content

Commit 566da0b

Browse files
committed
Address #227 and #262
Addressed a bottlenecking issue in the doctor causing a freeze and skipping broken specs in a report when using a timeline.
1 parent 186abcc commit 566da0b

14 files changed

Lines changed: 658 additions & 135 deletions

AGENTS.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,8 @@ All `cmd/` implementation files use their canonical names (e.g., `cmd/summary.go
6565
- `report` should emit semantic `path` values whenever possible.
6666
- If semantic normalization rewrites an engine path, preserve the original path in `rawPath`.
6767
- Parameter identity must not depend on array index positions when a stable semantic name exists.
68+
- Historical `report` output must surface partial-success state in `metaData` instead of relying on stderr warnings alone.
69+
- `metaData.skippedCommits` is the machine-readable list of git/GitHub history revisions that were skipped because they failed to normalize or render.
6870

6971
### Left/right behavior
7072

@@ -76,6 +78,7 @@ All `cmd/` implementation files use their canonical names (e.g., `cmd/summary.go
7678

7779
- Mixed-success histories should limp on with warnings when at least one comparable commit renders successfully.
7880
- `summary`, `markdown-report`, and `html-report` should fail only when every candidate commit fails to render/build.
81+
- Historical `report` output should limp on when at least one history item renders, and must set `metaData.partial = true` when any commits were skipped.
7982
- “No prior comparable version” and “no changes found” are distinct states.
8083

8184
### HTML payload integrity

cmd/console_test.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,9 +94,11 @@ func TestWrapConsoleStartError(t *testing.T) {
9494
func TestConsoleCommand_NoPriorVersionText(t *testing.T) {
9595
originalExtract := extractHistoryFromFile
9696
originalPopulate := populateHistory
97+
originalPopulateDetailed := populateHistoryDetailed
9798
t.Cleanup(func() {
9899
extractHistoryFromFile = originalExtract
99100
populateHistory = originalPopulate
101+
populateHistoryDetailed = originalPopulateDetailed
100102
})
101103

102104
extractHistoryFromFile = func(repoDirectory, filePath string,
@@ -110,6 +112,12 @@ func TestConsoleCommand_NoPriorVersionText(t *testing.T) {
110112
) ([]*model.Commit, []error) {
111113
return []*model.Commit{}, nil
112114
}
115+
populateHistoryDetailed = func(commitHistory []*model.Commit,
116+
progressChan chan *model.ProgressUpdate, errorChan chan model.ProgressError, opts git.HistoryOptions,
117+
breakingConfig *whatChangedModel.BreakingRulesConfig,
118+
) (*git.HistoryBuildResult, []error) {
119+
return &git.HistoryBuildResult{}, nil
120+
}
113121

114122
cmd := testRootCmd(GetConsoleCommand(), "--no-logo", "--no-color", "..", "sample-specs/petstorev3.json")
115123
output := captureStdout(t, func() {

cmd/engine.go

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,18 @@
44
package cmd
55

66
import (
7+
"bytes"
78
"fmt"
89
"os"
10+
"strings"
911

1012
"github.com/pb33f/doctor/changerator"
1113
drModel "github.com/pb33f/doctor/model"
1214
v3 "github.com/pb33f/doctor/model/high/v3"
1315
whatChangedModel "github.com/pb33f/libopenapi/what-changed/model"
1416
"github.com/pb33f/openapi-changes/internal/breakingrules"
1517
"github.com/pb33f/openapi-changes/model"
18+
"go.yaml.in/yaml/v4"
1619
)
1720

1821
// changeratorResult owns the doctor-side resources created for a single comparison.
@@ -51,6 +54,9 @@ func runChangerator(commit *model.Commit, breakingConfig *whatChangedModel.Break
5154
if err != nil {
5255
return nil, modelBuildError("original", commitSourceLabel(commit, false), err)
5356
}
57+
if isSelfContainedIdenticalComparison(commit) {
58+
return nil, nil
59+
}
5460

5561
rightDrDoc := drModel.NewDrDocumentAndGraph(rightModel)
5662
leftDrDoc := drModel.NewDrDocumentAndGraph(leftModel)
@@ -92,6 +98,54 @@ func runChangerator(commit *model.Commit, breakingConfig *whatChangedModel.Break
9298
}, nil
9399
}
94100

101+
func isSelfContainedIdenticalComparison(commit *model.Commit) bool {
102+
if commit == nil || !commit.Synthetic || len(commit.Data) == 0 || len(commit.OldData) == 0 {
103+
return false
104+
}
105+
if !bytes.Equal(commit.Data, commit.OldData) {
106+
return false
107+
}
108+
return !rootBytesContainExternalRefs(commit.OldData) && !rootBytesContainExternalRefs(commit.Data)
109+
}
110+
111+
func rootBytesContainExternalRefs(spec []byte) bool {
112+
if len(spec) == 0 {
113+
return false
114+
}
115+
var root yaml.Node
116+
if err := yaml.Unmarshal(spec, &root); err != nil {
117+
return true
118+
}
119+
return nodeContainsExternalRef(&root)
120+
}
121+
122+
func nodeContainsExternalRef(node *yaml.Node) bool {
123+
if node == nil {
124+
return false
125+
}
126+
if node.Kind == yaml.MappingNode {
127+
for i := 0; i+1 < len(node.Content); i += 2 {
128+
key := node.Content[i]
129+
value := node.Content[i+1]
130+
if key != nil && key.Value == "$ref" && value != nil && value.Kind == yaml.ScalarNode {
131+
if value.Value != "" && !strings.HasPrefix(value.Value, "#") {
132+
return true
133+
}
134+
}
135+
if nodeContainsExternalRef(value) {
136+
return true
137+
}
138+
}
139+
return false
140+
}
141+
for _, child := range node.Content {
142+
if nodeContainsExternalRef(child) {
143+
return true
144+
}
145+
}
146+
return false
147+
}
148+
95149
func rewriteOutputLocations(ctr *changerator.Changerator, docChanges *whatChangedModel.DocumentChanges, rewriters []model.DocumentPathRewriter) {
96150
if len(rewriters) == 0 {
97151
return

cmd/loaders.go

Lines changed: 47 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,17 @@ import (
1919
)
2020

2121
var processGithubRepo = git.ProcessGithubRepo
22+
var processGithubRepoDetailed = git.ProcessGithubRepoDetailed
2223
var extractHistoryFromFile = git.ExtractHistoryFromFile
2324
var populateHistory = git.PopulateHistory
25+
var populateHistoryDetailed = git.PopulateHistoryDetailed
2426
var httpGet = http.Get
2527

28+
type loadedHistoryResult struct {
29+
Commits []*model.Commit
30+
SkippedCommits []string
31+
}
32+
2633
// progressDrainer drains git progress channels that use synchronous sends.
2734
// Call close() before reading collected warnings or errors.
2835
type progressDrainer struct {
@@ -116,7 +123,7 @@ func (d *progressDrainer) collectErrors(errs []error) error {
116123
return errors.Join(all...)
117124
}
118125

119-
func loadGitHubCommits(rawURL string, opts summaryOpts, breakingConfig *whatChangedModel.BreakingRulesConfig) ([]*model.Commit, error) {
126+
func loadGitHubCommitsDetailed(rawURL string, opts summaryOpts, breakingConfig *whatChangedModel.BreakingRulesConfig) (*loadedHistoryResult, error) {
120127
specURL, err := url.Parse(rawURL)
121128
if err != nil {
122129
return nil, fmt.Errorf("invalid URL: %w", err)
@@ -127,7 +134,7 @@ func loadGitHubCommits(rawURL string, opts summaryOpts, breakingConfig *whatChan
127134
}
128135

129136
d := makeProgressDrainer()
130-
commits, errs := processGithubRepo(user, repo, filePath,
137+
result, errs := processGithubRepoDetailed(user, repo, filePath,
131138
d.ProgressChan, d.ErrorChan, git.HistoryOptions{
132139
BaseCommit: opts.baseCommit,
133140
Limit: opts.limit,
@@ -141,13 +148,28 @@ func loadGitHubCommits(rawURL string, opts summaryOpts, breakingConfig *whatChan
141148
return nil, err
142149
}
143150

151+
if result == nil {
152+
return nil, nil
153+
}
154+
commits := result.Commits
144155
if opts.latest && len(commits) > 1 {
145156
commits = commits[:1]
146157
}
147-
return commits, nil
158+
return &loadedHistoryResult{
159+
Commits: commits,
160+
SkippedCommits: result.SkippedCommits,
161+
}, nil
148162
}
149163

150-
func loadGitHistoryCommits(gitPath, filePath string, opts summaryOpts, breakingConfig *whatChangedModel.BreakingRulesConfig) ([]*model.Commit, error) {
164+
func loadGitHubCommits(rawURL string, opts summaryOpts, breakingConfig *whatChangedModel.BreakingRulesConfig) ([]*model.Commit, error) {
165+
result, err := loadGitHubCommitsDetailed(rawURL, opts, breakingConfig)
166+
if result == nil {
167+
return nil, err
168+
}
169+
return result.Commits, err
170+
}
171+
172+
func loadGitHistoryCommitsDetailed(gitPath, filePath string, opts summaryOpts, breakingConfig *whatChangedModel.BreakingRulesConfig) (*loadedHistoryResult, error) {
151173
if gitPath == "" || filePath == "" {
152174
return nil, errors.New("please supply a path to a git repo and a path to a file")
153175
}
@@ -178,7 +200,7 @@ func loadGitHistoryCommits(gitPath, filePath string, opts summaryOpts, breakingC
178200
}
179201

180202
populateDrainer := makeProgressDrainer()
181-
commits, errs = populateHistory(commits,
203+
result, errs := populateHistoryDetailed(commits,
182204
populateDrainer.ProgressChan, populateDrainer.ErrorChan, git.HistoryOptions{
183205
LimitTime: opts.limitTime,
184206
Base: opts.base,
@@ -190,13 +212,31 @@ func loadGitHistoryCommits(gitPath, filePath string, opts summaryOpts, breakingC
190212
return nil, err
191213
}
192214

193-
if len(commits) == 0 {
215+
if result == nil {
194216
return nil, nil
195217
}
218+
commits = result.Commits
219+
if len(commits) == 0 {
220+
return &loadedHistoryResult{SkippedCommits: result.SkippedCommits}, nil
221+
}
196222
if opts.latest {
197223
commits = commits[:1]
198224
}
199-
return commits, nil
225+
return &loadedHistoryResult{
226+
Commits: commits,
227+
SkippedCommits: result.SkippedCommits,
228+
}, nil
229+
}
230+
231+
func loadGitHistoryCommits(gitPath, filePath string, opts summaryOpts, breakingConfig *whatChangedModel.BreakingRulesConfig) ([]*model.Commit, error) {
232+
result, err := loadGitHistoryCommitsDetailed(gitPath, filePath, opts, breakingConfig)
233+
if result == nil {
234+
return nil, err
235+
}
236+
if len(result.Commits) == 0 {
237+
return nil, nil
238+
}
239+
return result.Commits, err
200240
}
201241

202242
func loadLeftRightCommits(left, right string, opts summaryOpts) ([]*model.Commit, error) {

0 commit comments

Comments
 (0)