Skip to content

Commit facf5c1

Browse files
pashagolubpashagolub
authored
[-] apply non-recursive yaml dir reading, fixes #1368 (#1382)
* apply non-recursive yaml dir reading, fixes #1368 replace `WalkDir` with `filepath.Glob`, those functions never intended to be recursive in the first place * fix test * fix unsupported extensions * add tests for error paths
1 parent cf27d73 commit facf5c1

4 files changed

Lines changed: 127 additions & 28 deletions

File tree

‎internal/metrics/yaml.go‎

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package metrics
33
import (
44
"context"
55
_ "embed"
6-
"io/fs"
76
"maps"
87
"os"
98
"path/filepath"
@@ -66,21 +65,21 @@ func (fmr *fileMetricReader) getMetrics() (metrics *Metrics, err error) {
6665
}
6766
switch mode := fi.Mode(); {
6867
case mode.IsDir():
69-
err = filepath.WalkDir(fmr.path, func(path string, d fs.DirEntry, err error) error {
70-
if err != nil {
71-
return err
72-
}
73-
ext := strings.ToLower(filepath.Ext(d.Name()))
74-
if d.IsDir() || ext != ".yaml" && ext != ".yml" {
75-
return nil
68+
var entries []os.DirEntry
69+
if entries, err = os.ReadDir(fmr.path); err != nil {
70+
return nil, err
71+
}
72+
for _, entry := range entries {
73+
if ext := strings.ToLower(filepath.Ext(entry.Name())); ext != ".yaml" && ext != ".yml" {
74+
continue
7675
}
7776
var m *Metrics
78-
if m, err = fmr.loadMetricsFromFile(path); err == nil {
79-
maps.Copy(metrics.PresetDefs, m.PresetDefs)
80-
maps.Copy(metrics.MetricDefs, m.MetricDefs)
77+
if m, err = fmr.loadMetricsFromFile(filepath.Join(fmr.path, entry.Name())); err != nil {
78+
return nil, err
8179
}
82-
return err
83-
})
80+
maps.Copy(metrics.PresetDefs, m.PresetDefs)
81+
maps.Copy(metrics.MetricDefs, m.MetricDefs)
82+
}
8483
case mode.IsRegular():
8584
metrics, err = fmr.loadMetricsFromFile(fmr.path)
8685
}

‎internal/metrics/yaml_test.go‎

Lines changed: 52 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"fmt"
55
"os"
66
"path/filepath"
7+
"runtime"
78
"sync"
89
"testing"
910
"time"
@@ -222,9 +223,9 @@ func TestErrorHandlingToFile(t *testing.T) {
222223
err = fmr.WriteMetrics(&metrics.Metrics{})
223224
assert.Error(t, err)
224225

225-
// Test GetMetrics
226+
// Test GetMetrics - root dir has no yaml files, returns empty metrics without error
226227
_, err = fmr.GetMetrics()
227-
assert.Error(t, err)
228+
assert.NoError(t, err)
228229

229230
// Test DeleteMetric
230231
err = fmr.DeleteMetric("test")
@@ -375,11 +376,11 @@ func TestMetricsDir(t *testing.T) {
375376
metrics2File, err := yaml.Marshal(metrics2)
376377
a.NoError(err)
377378

378-
// write data to different files in a folder
379+
// write data to different files in a folder: one .yaml and one .yml
379380
tempDir := t.TempDir()
380381
err = os.WriteFile(filepath.Join(tempDir, "metrics1.yaml"), metrics1File, 0644)
381382
a.NoError(err)
382-
err = os.WriteFile(filepath.Join(tempDir, "metrics2.yaml"), metrics2File, 0644)
383+
err = os.WriteFile(filepath.Join(tempDir, "metrics2.yml"), metrics2File, 0644)
383384
a.NoError(err)
384385

385386
// use folder of yaml files for metrics configs
@@ -474,3 +475,50 @@ func TestConcurrentPresetUpdates(t *testing.T) {
474475
a.NoError(err)
475476
a.Equal(numGoroutines, len(finalMetrics.PresetDefs), "Some updates were lost due to race condition!")
476477
}
478+
479+
func TestGetMetricsNonExistentPath(t *testing.T) {
480+
nonExistent := filepath.Join(t.TempDir(), "does_not_exist", "metrics.yaml")
481+
fmr, err := metrics.NewYAMLMetricReaderWriter(ctx, nonExistent)
482+
assert.NoError(t, err)
483+
_, err = fmr.GetMetrics()
484+
assert.Error(t, err)
485+
}
486+
487+
func TestGetMetricsDirWithInvalidYAML(t *testing.T) {
488+
dir := t.TempDir()
489+
err := os.WriteFile(filepath.Join(dir, "bad.yaml"), []byte("invalid: yaml: {unclosed"), 0644)
490+
assert.NoError(t, err)
491+
fmr, err := metrics.NewYAMLMetricReaderWriter(ctx, dir)
492+
assert.NoError(t, err)
493+
_, err = fmr.GetMetrics()
494+
assert.Error(t, err)
495+
}
496+
497+
func TestMutationsGetMetricsError(t *testing.T) {
498+
nonExistent := filepath.Join(t.TempDir(), "does_not_exist", "metrics.yaml")
499+
fmr, err := metrics.NewYAMLMetricReaderWriter(ctx, nonExistent)
500+
assert.NoError(t, err)
501+
502+
assert.Error(t, fmr.DeleteMetric("x"))
503+
assert.Error(t, fmr.UpdateMetric("x", metrics.Metric{}))
504+
assert.Error(t, fmr.CreateMetric("x", metrics.Metric{}))
505+
assert.Error(t, fmr.DeletePreset("x"))
506+
assert.Error(t, fmr.UpdatePreset("x", metrics.Preset{}))
507+
assert.Error(t, fmr.CreatePreset("x", metrics.Preset{}))
508+
}
509+
510+
func TestLoadMetricsUnreadableFile(t *testing.T) {
511+
if runtime.GOOS == "windows" {
512+
t.Skip("cannot reliably test file permissions on Windows")
513+
}
514+
if os.Getuid() == 0 {
515+
t.Skip("running as root, permission checks do not apply")
516+
}
517+
f := filepath.Join(t.TempDir(), "metrics.yaml")
518+
err := os.WriteFile(f, []byte(""), 0000)
519+
assert.NoError(t, err)
520+
fmr, err := metrics.NewYAMLMetricReaderWriter(ctx, f)
521+
assert.NoError(t, err)
522+
_, err = fmr.GetMetrics()
523+
assert.Error(t, err)
524+
}

‎internal/sources/yaml.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -103,20 +103,20 @@ func (fcr *fileSourcesReaderWriter) getSources() (dbs Sources, err error) {
103103
}
104104
switch mode := fi.Mode(); {
105105
case mode.IsDir():
106-
err = filepath.WalkDir(fcr.path, func(path string, d fs.DirEntry, err error) error {
107-
if err != nil {
108-
return err
109-
}
110-
ext := strings.ToLower(filepath.Ext(d.Name()))
111-
if d.IsDir() || ext != ".yaml" && ext != ".yml" {
112-
return nil
106+
var entries []os.DirEntry
107+
if entries, err = os.ReadDir(fcr.path); err != nil {
108+
return nil, err
109+
}
110+
for _, entry := range entries {
111+
if ext := strings.ToLower(filepath.Ext(entry.Name())); ext != ".yaml" && ext != ".yml" {
112+
continue
113113
}
114114
var mdbs Sources
115-
if mdbs, err = fcr.loadSourcesFromFile(path); err == nil {
116-
dbs = append(dbs, mdbs...)
115+
if mdbs, err = fcr.loadSourcesFromFile(filepath.Join(fcr.path, entry.Name())); err != nil {
116+
return
117117
}
118-
return err
119-
})
118+
dbs = append(dbs, mdbs...)
119+
}
120120
case mode.IsRegular():
121121
dbs, err = fcr.loadSourcesFromFile(fcr.path)
122122
}

‎internal/sources/yaml_test.go‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,27 @@ func TestYAMLGetMonitoredDatabases(t *testing.T) {
9999
a.Error(err)
100100
a.Nil(dbs)
101101
})
102+
103+
t.Run("directory with yaml and yml files", func(t *testing.T) {
104+
tmpDir := t.TempDir()
105+
yamlContent1 := `
106+
- name: dir_test1
107+
conn_str: postgresql://localhost/test1
108+
`
109+
yamlContent2 := `
110+
- name: dir_test2
111+
conn_str: postgresql://localhost/test2
112+
`
113+
err := os.WriteFile(filepath.Join(tmpDir, "sources.yaml"), []byte(yamlContent1), 0644)
114+
a.NoError(err)
115+
err = os.WriteFile(filepath.Join(tmpDir, "sources.yml"), []byte(yamlContent2), 0644)
116+
a.NoError(err)
117+
yamlrw, err := sources.NewYAMLSourcesReaderWriter(ctx, tmpDir)
118+
a.NoError(err)
119+
dbs, err := yamlrw.GetSources()
120+
a.NoError(err)
121+
a.Len(dbs, 2)
122+
})
102123
}
103124

104125
func TestYAMLDeleteDatabase(t *testing.T) {
@@ -364,3 +385,34 @@ func TestConcurrentSourceUpdates(t *testing.T) {
364385
a.NoError(err)
365386
a.Equal(numGoroutines, len(finalSources), "Some updates were lost due to race condition!")
366387
}
388+
389+
func TestGetSourcesDirWithInvalidYAML(t *testing.T) {
390+
dir := t.TempDir()
391+
err := os.WriteFile(filepath.Join(dir, "bad.yaml"), []byte("invalid: yaml: {unclosed"), 0644)
392+
assert.NoError(t, err)
393+
yamlrw, err := sources.NewYAMLSourcesReaderWriter(ctx, dir)
394+
assert.NoError(t, err)
395+
_, err = yamlrw.GetSources()
396+
assert.Error(t, err)
397+
}
398+
399+
func TestCreateSourceGetSourcesError(t *testing.T) {
400+
nonExistent := filepath.Join(t.TempDir(), "does_not_exist", "sources.yaml")
401+
yamlrw, err := sources.NewYAMLSourcesReaderWriter(ctx, nonExistent)
402+
assert.NoError(t, err)
403+
assert.Error(t, yamlrw.CreateSource(sources.Source{Name: "x"}))
404+
}
405+
406+
// TestMutationsWriteError verifies that UpdateSource, DeleteSource, and CreateSource
407+
// propagate write errors when the path is a directory (cannot be overwritten as a file).
408+
func TestMutationsWriteError(t *testing.T) {
409+
// Using a dir as the file path: getSources succeeds (reads empty dir),
410+
// but writeSources fails because os.WriteFile cannot write to a directory.
411+
dir := t.TempDir()
412+
yamlrw, err := sources.NewYAMLSourcesReaderWriter(ctx, dir)
413+
assert.NoError(t, err)
414+
415+
assert.Error(t, yamlrw.UpdateSource(sources.Source{Name: "x"}))
416+
assert.Error(t, yamlrw.DeleteSource("x"))
417+
assert.Error(t, yamlrw.CreateSource(sources.Source{Name: "x"}))
418+
}

0 commit comments

Comments
 (0)