Skip to content

Commit 30c869c

Browse files
erraggyclaude
andcommitted
refactor: consolidate equality helpers, fix silent limits, centralize file permissions
- Extract generic EqualPtr[T comparable] into internal/equalutil, replacing 5 type-specific pointer-equality functions across parser/ and joiner/ - Add depth-limit warnings: RefCollector.Warnings field in fixer/refs.go, slog.Warn in internal/jsonpath/eval.go (previously silent data truncation) - Centralize file permissions in internal/fileutil (OwnerReadWrite 0o600, ReadableByAll 0o644), replacing hardcoded literals across 12 production files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 39b310d commit 30c869c

27 files changed

Lines changed: 403 additions & 308 deletions

builder/builder.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"path/filepath"
99
"strings"
1010

11+
"github.com/erraggy/oastools/internal/fileutil"
1112
"github.com/erraggy/oastools/internal/schemautil"
1213
"github.com/erraggy/oastools/joiner"
1314
"github.com/erraggy/oastools/parser"
@@ -621,9 +622,6 @@ func (b *Builder) MarshalJSON() ([]byte, error) {
621622
return json.MarshalIndent(doc, "", " ")
622623
}
623624

624-
// outputFileMode is the file permission mode for output files (owner read/write only)
625-
const outputFileMode = 0600
626-
627625
// WriteFile writes the document to a file.
628626
// The format is inferred from the file extension (.json for JSON, .yaml/.yml for YAML).
629627
func (b *Builder) WriteFile(path string) error {
@@ -645,7 +643,7 @@ func (b *Builder) WriteFile(path string) error {
645643
return fmt.Errorf("builder: failed to marshal document: %w", err)
646644
}
647645

648-
if err := os.WriteFile(path, data, outputFileMode); err != nil {
646+
if err := os.WriteFile(path, data, fileutil.OwnerReadWrite); err != nil {
649647
return fmt.Errorf("builder: failed to write file: %w", err)
650648
}
651649

cmd/oastools/commands/convert.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010

1111
"github.com/erraggy/oastools"
1212
"github.com/erraggy/oastools/converter"
13+
"github.com/erraggy/oastools/internal/fileutil"
1314
"github.com/erraggy/oastools/parser"
1415
)
1516

@@ -203,7 +204,7 @@ func HandleConvert(args []string) error {
203204
if err := RejectSymlinkOutput(cleanedOutput); err != nil {
204205
return err
205206
}
206-
if err := os.WriteFile(cleanedOutput, data, 0600); err != nil { //nolint:gosec // G703 - output path is user-provided CLI flag
207+
if err := os.WriteFile(cleanedOutput, data, fileutil.OwnerReadWrite); err != nil { //nolint:gosec // G703 - output path is user-provided CLI flag
207208
return fmt.Errorf("writing output file: %w", err)
208209
}
209210
if !flags.Quiet {

cmd/oastools/commands/fix.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010

1111
"github.com/erraggy/oastools"
1212
"github.com/erraggy/oastools/fixer"
13+
"github.com/erraggy/oastools/internal/fileutil"
1314
"github.com/erraggy/oastools/parser"
1415
)
1516

@@ -348,7 +349,7 @@ func HandleFix(args []string) error {
348349
if err := RejectSymlinkOutput(cleanedOutput); err != nil {
349350
return err
350351
}
351-
if err := os.WriteFile(cleanedOutput, data, 0600); err != nil { //nolint:gosec // G703 - output path is user-provided CLI flag
352+
if err := os.WriteFile(cleanedOutput, data, fileutil.OwnerReadWrite); err != nil { //nolint:gosec // G703 - output path is user-provided CLI flag
352353
return fmt.Errorf("writing output file: %w", err)
353354
}
354355
if !flags.Quiet {

cmd/oastools/commands/join.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"time"
1212

1313
"github.com/erraggy/oastools"
14+
"github.com/erraggy/oastools/internal/fileutil"
1415
"github.com/erraggy/oastools/joiner"
1516
"github.com/erraggy/oastools/parser"
1617
)
@@ -389,11 +390,11 @@ func HandleJoin(args []string) error {
389390
if symlinkErr := RejectSymlinkOutput(cleanedOutput); symlinkErr != nil {
390391
return symlinkErr
391392
}
392-
if writeErr := os.WriteFile(cleanedOutput, data, 0600); writeErr != nil { //nolint:gosec // G703 - output path is user-provided CLI flag
393+
if writeErr := os.WriteFile(cleanedOutput, data, fileutil.OwnerReadWrite); writeErr != nil { //nolint:gosec // G703 - output path is user-provided CLI flag
393394
return fmt.Errorf("writing output file: %w", writeErr)
394395
}
395396
// Ensure correct permissions even if file pre-existed with different permissions
396-
if chmodErr := os.Chmod(cleanedOutput, 0600); chmodErr != nil {
397+
if chmodErr := os.Chmod(cleanedOutput, fileutil.OwnerReadWrite); chmodErr != nil {
397398
return fmt.Errorf("setting output file permissions: %w", chmodErr)
398399
}
399400
if !flags.Quiet {

cmd/oastools/commands/overlay.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"time"
1010

1111
"github.com/erraggy/oastools"
12+
"github.com/erraggy/oastools/internal/fileutil"
1213
"github.com/erraggy/oastools/overlay"
1314
"github.com/erraggy/oastools/parser"
1415
)
@@ -241,7 +242,7 @@ func handleOverlayApply(args []string) error {
241242
if err := RejectSymlinkOutput(cleanedOutput); err != nil {
242243
return err
243244
}
244-
if err := os.WriteFile(cleanedOutput, data, 0600); err != nil { //nolint:gosec // G703 - output path is user-provided CLI flag
245+
if err := os.WriteFile(cleanedOutput, data, fileutil.OwnerReadWrite); err != nil { //nolint:gosec // G703 - output path is user-provided CLI flag
245246
return fmt.Errorf("writing output file: %w", err)
246247
}
247248
if !flags.Quiet {

fixer/oas2.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package fixer
44

55
import (
66
"fmt"
7+
"log/slog"
78
"sort"
89

910
"github.com/erraggy/oastools/parser"
@@ -187,6 +188,9 @@ func (f *Fixer) pruneUnusedSchemasOAS2(doc *parser.OAS2Document, result *FixResu
187188
// Collect all refs in the document
188189
collector := NewRefCollector()
189190
collector.CollectOAS2(doc)
191+
for _, w := range collector.Warnings {
192+
slog.Warn("ref collection warning", "detail", w)
193+
}
190194

191195
// Prune unreferenced schemas
192196
f.pruneSchemas(doc.Definitions, collector, doc, result)

fixer/oas3.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package fixer
44

55
import (
66
"fmt"
7+
"log/slog"
78

89
"github.com/erraggy/oastools/parser"
910
)
@@ -264,6 +265,9 @@ func (f *Fixer) pruneUnusedSchemasOAS3(doc *parser.OAS3Document, result *FixResu
264265
// Collect all refs in the document
265266
collector := NewRefCollector()
266267
collector.CollectOAS3(doc)
268+
for _, w := range collector.Warnings {
269+
slog.Warn("ref collection warning", "detail", w)
270+
}
267271

268272
// Prune unreferenced schemas
269273
f.pruneSchemas(doc.Components.Schemas, collector, doc, result)

fixer/refs.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,10 @@ type RefCollector struct {
131131

132132
// visited tracks processed schemas for circular reference handling.
133133
visited map[*parser.Schema]bool
134+
135+
// Warnings contains non-fatal issues encountered during collection,
136+
// such as depth limit truncation.
137+
Warnings []string
134138
}
135139

136140
// NewRefCollector creates a new RefCollector instance.
@@ -648,7 +652,7 @@ func (c *RefCollector) collectRefsFromMap(m map[string]any, path string) {
648652
// collectRefsFromMapWithDepth is the internal implementation with depth tracking.
649653
func (c *RefCollector) collectRefsFromMapWithDepth(m map[string]any, path string, depth int) {
650654
if depth > maxRefCollectionDepth {
651-
// Extremely deep nesting or circular structure - stop to prevent stack overflow
655+
c.Warnings = append(c.Warnings, fmt.Sprintf("ref collection truncated at depth %d (path: %s): possible circular structure", depth, path))
652656
return
653657
}
654658

fixer/refs_test.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -894,6 +894,35 @@ components:
894894
assert.False(t, collector.IsPathItemReferenced("UnusedPath"))
895895
}
896896

897+
// TestRefCollector_DepthLimitWarning tests that exceeding maxRefCollectionDepth
898+
// produces a warning instead of silently truncating.
899+
func TestRefCollector_DepthLimitWarning(t *testing.T) {
900+
// Build a deeply nested map structure exceeding maxRefCollectionDepth (100).
901+
// Use "items" as the nesting key since collectRefsFromMapWithDepth recurses
902+
// into recognized OAS keys like "items", "properties", etc.
903+
leaf := map[string]any{
904+
"$ref": "#/components/schemas/TooDeep",
905+
}
906+
current := leaf
907+
for range maxRefCollectionDepth + 2 {
908+
current = map[string]any{
909+
"items": current,
910+
}
911+
}
912+
913+
collector := NewRefCollector()
914+
collector.collectRefsFromMap(current, "$.root")
915+
916+
// The collector should have recorded at least one warning about truncation.
917+
require.NotEmpty(t, collector.Warnings, "expected depth limit warning but got none")
918+
assert.Contains(t, collector.Warnings[0], "truncated",
919+
"warning should mention truncation")
920+
921+
// The deeply nested $ref should NOT have been collected (it's beyond the depth limit).
922+
_, found := collector.Refs["#/components/schemas/TooDeep"]
923+
assert.False(t, found, "refs beyond the depth limit should not be collected")
924+
}
925+
897926
// TestRefCollector_JSONSchema202012 tests ref collection from JSON Schema Draft 2020-12 keywords
898927
// like unevaluatedProperties, unevaluatedItems, and contentSchema.
899928
func TestRefCollector_JSONSchema202012(t *testing.T) {

fixer/stub_missing_refs.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package fixer
66

77
import (
88
"fmt"
9+
"log/slog"
910
"sort"
1011
"strings"
1112

@@ -59,6 +60,9 @@ func (f *Fixer) stubMissingRefsOAS2(doc *parser.OAS2Document, result *FixResult)
5960
// Collect all refs in the document
6061
collector := NewRefCollector()
6162
collector.CollectOAS2(doc)
63+
for _, w := range collector.Warnings {
64+
slog.Warn("ref collection warning", "detail", w)
65+
}
6266

6367
// Check schema refs and stub missing ones
6468
// Sort refs for deterministic output order
@@ -173,6 +177,9 @@ func (f *Fixer) stubMissingRefsOAS3(doc *parser.OAS3Document, result *FixResult)
173177
// Collect all refs in the document
174178
collector := NewRefCollector()
175179
collector.CollectOAS3(doc)
180+
for _, w := range collector.Warnings {
181+
slog.Warn("ref collection warning", "detail", w)
182+
}
176183

177184
// Check schema refs and stub missing ones
178185
// Sort refs for deterministic output order

0 commit comments

Comments
 (0)