From ecf61db1972b6471dee4cbb4d77f84fe983a253a Mon Sep 17 00:00:00 2001 From: Pavlo Golub Date: Wed, 29 Apr 2026 14:10:18 +0200 Subject: [PATCH 1/4] [-] apply non-recursive yaml dir reading, fixes #1368 replace `WalkDir` with `filepath.Glob`, those functions never intended to be recursive in the first place --- internal/metrics/yaml.go | 25 ++++++++++--------------- internal/metrics/yaml_test.go | 4 ++-- internal/sources/yaml.go | 21 +++++++++------------ internal/sources/yaml_test.go | 21 +++++++++++++++++++++ 4 files changed, 42 insertions(+), 29 deletions(-) diff --git a/internal/metrics/yaml.go b/internal/metrics/yaml.go index a298dcc0c9..1095c5b514 100644 --- a/internal/metrics/yaml.go +++ b/internal/metrics/yaml.go @@ -3,11 +3,9 @@ package metrics import ( "context" _ "embed" - "io/fs" "maps" "os" "path/filepath" - "strings" "sync" "gopkg.in/yaml.v3" @@ -66,21 +64,18 @@ func (fmr *fileMetricReader) getMetrics() (metrics *Metrics, err error) { } switch mode := fi.Mode(); { case mode.IsDir(): - err = filepath.WalkDir(fmr.path, func(path string, d fs.DirEntry, err error) error { - if err != nil { - return err - } - ext := strings.ToLower(filepath.Ext(d.Name())) - if d.IsDir() || ext != ".yaml" && ext != ".yml" { - return nil - } + var matches []string + if matches, err = filepath.Glob(filepath.Join(fmr.path, "*.y*ml")); err != nil { + return nil, err + } + for _, path := range matches { var m *Metrics - if m, err = fmr.loadMetricsFromFile(path); err == nil { - maps.Copy(metrics.PresetDefs, m.PresetDefs) - maps.Copy(metrics.MetricDefs, m.MetricDefs) + if m, err = fmr.loadMetricsFromFile(path); err != nil { + return nil, err } - return err - }) + maps.Copy(metrics.PresetDefs, m.PresetDefs) + maps.Copy(metrics.MetricDefs, m.MetricDefs) + } case mode.IsRegular(): metrics, err = fmr.loadMetricsFromFile(fmr.path) } diff --git a/internal/metrics/yaml_test.go b/internal/metrics/yaml_test.go index ec2d51a8d1..aed1625cdf 100644 --- a/internal/metrics/yaml_test.go +++ b/internal/metrics/yaml_test.go @@ -375,11 +375,11 @@ func TestMetricsDir(t *testing.T) { metrics2File, err := yaml.Marshal(metrics2) a.NoError(err) - // write data to different files in a folder + // write data to different files in a folder: one .yaml and one .yml tempDir := t.TempDir() err = os.WriteFile(filepath.Join(tempDir, "metrics1.yaml"), metrics1File, 0644) a.NoError(err) - err = os.WriteFile(filepath.Join(tempDir, "metrics2.yaml"), metrics2File, 0644) + err = os.WriteFile(filepath.Join(tempDir, "metrics2.yml"), metrics2File, 0644) a.NoError(err) // use folder of yaml files for metrics configs diff --git a/internal/sources/yaml.go b/internal/sources/yaml.go index 7e2416cb19..46f77f7b7d 100644 --- a/internal/sources/yaml.go +++ b/internal/sources/yaml.go @@ -103,20 +103,17 @@ func (fcr *fileSourcesReaderWriter) getSources() (dbs Sources, err error) { } switch mode := fi.Mode(); { case mode.IsDir(): - err = filepath.WalkDir(fcr.path, func(path string, d fs.DirEntry, err error) error { - if err != nil { - return err - } - ext := strings.ToLower(filepath.Ext(d.Name())) - if d.IsDir() || ext != ".yaml" && ext != ".yml" { - return nil - } + var matches []string + if matches, err = filepath.Glob(filepath.Join(fcr.path, "*.y*ml")); err != nil { + return + } + for _, path := range matches { var mdbs Sources - if mdbs, err = fcr.loadSourcesFromFile(path); err == nil { - dbs = append(dbs, mdbs...) + if mdbs, err = fcr.loadSourcesFromFile(path); err != nil { + return } - return err - }) + dbs = append(dbs, mdbs...) + } case mode.IsRegular(): dbs, err = fcr.loadSourcesFromFile(fcr.path) } diff --git a/internal/sources/yaml_test.go b/internal/sources/yaml_test.go index 7fba3eca8d..57663ffeea 100644 --- a/internal/sources/yaml_test.go +++ b/internal/sources/yaml_test.go @@ -99,6 +99,27 @@ func TestYAMLGetMonitoredDatabases(t *testing.T) { a.Error(err) a.Nil(dbs) }) + + t.Run("directory with yaml and yml files", func(t *testing.T) { + tmpDir := t.TempDir() + yamlContent1 := ` +- name: dir_test1 + conn_str: postgresql://localhost/test1 +` + yamlContent2 := ` +- name: dir_test2 + conn_str: postgresql://localhost/test2 +` + err := os.WriteFile(filepath.Join(tmpDir, "sources.yaml"), []byte(yamlContent1), 0644) + a.NoError(err) + err = os.WriteFile(filepath.Join(tmpDir, "sources.yml"), []byte(yamlContent2), 0644) + a.NoError(err) + yamlrw, err := sources.NewYAMLSourcesReaderWriter(ctx, tmpDir) + a.NoError(err) + dbs, err := yamlrw.GetSources() + a.NoError(err) + a.Len(dbs, 2) + }) } func TestYAMLDeleteDatabase(t *testing.T) { From 49623c94bed25b53a92e9cb9e4b9b426fcd2de30 Mon Sep 17 00:00:00 2001 From: Pavlo Golub Date: Wed, 29 Apr 2026 15:33:20 +0200 Subject: [PATCH 2/4] fix test --- internal/metrics/yaml_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/metrics/yaml_test.go b/internal/metrics/yaml_test.go index aed1625cdf..456189e3bd 100644 --- a/internal/metrics/yaml_test.go +++ b/internal/metrics/yaml_test.go @@ -222,9 +222,9 @@ func TestErrorHandlingToFile(t *testing.T) { err = fmr.WriteMetrics(&metrics.Metrics{}) assert.Error(t, err) - // Test GetMetrics + // Test GetMetrics - root dir has no yaml files, returns empty metrics without error _, err = fmr.GetMetrics() - assert.Error(t, err) + assert.NoError(t, err) // Test DeleteMetric err = fmr.DeleteMetric("test") From 75c77bde3a9ba443204b26f13a084bb2ab560e9c Mon Sep 17 00:00:00 2001 From: Pavlo Golub Date: Thu, 30 Apr 2026 14:53:48 +0200 Subject: [PATCH 3/4] fix unsupported extensions --- internal/metrics/yaml.go | 12 ++++++++---- internal/sources/yaml.go | 13 ++++++++----- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/internal/metrics/yaml.go b/internal/metrics/yaml.go index 1095c5b514..79a3811829 100644 --- a/internal/metrics/yaml.go +++ b/internal/metrics/yaml.go @@ -6,6 +6,7 @@ import ( "maps" "os" "path/filepath" + "strings" "sync" "gopkg.in/yaml.v3" @@ -64,13 +65,16 @@ func (fmr *fileMetricReader) getMetrics() (metrics *Metrics, err error) { } switch mode := fi.Mode(); { case mode.IsDir(): - var matches []string - if matches, err = filepath.Glob(filepath.Join(fmr.path, "*.y*ml")); err != nil { + var entries []os.DirEntry + if entries, err = os.ReadDir(fmr.path); err != nil { return nil, err } - for _, path := range matches { + for _, entry := range entries { + if ext := strings.ToLower(filepath.Ext(entry.Name())); ext != ".yaml" && ext != ".yml" { + continue + } var m *Metrics - if m, err = fmr.loadMetricsFromFile(path); err != nil { + if m, err = fmr.loadMetricsFromFile(filepath.Join(fmr.path, entry.Name())); err != nil { return nil, err } maps.Copy(metrics.PresetDefs, m.PresetDefs) diff --git a/internal/sources/yaml.go b/internal/sources/yaml.go index 46f77f7b7d..d373b46da4 100644 --- a/internal/sources/yaml.go +++ b/internal/sources/yaml.go @@ -103,13 +103,16 @@ func (fcr *fileSourcesReaderWriter) getSources() (dbs Sources, err error) { } switch mode := fi.Mode(); { case mode.IsDir(): - var matches []string - if matches, err = filepath.Glob(filepath.Join(fcr.path, "*.y*ml")); err != nil { - return + var entries []os.DirEntry + if entries, err = os.ReadDir(fcr.path); err != nil { + return nil, err } - for _, path := range matches { + for _, entry := range entries { + if ext := strings.ToLower(filepath.Ext(entry.Name())); ext != ".yaml" && ext != ".yml" { + continue + } var mdbs Sources - if mdbs, err = fcr.loadSourcesFromFile(path); err != nil { + if mdbs, err = fcr.loadSourcesFromFile(filepath.Join(fcr.path, entry.Name())); err != nil { return } dbs = append(dbs, mdbs...) From c0bba1eb369fc350c782673034bb383add92b79d Mon Sep 17 00:00:00 2001 From: Pavlo Golub Date: Thu, 30 Apr 2026 15:09:27 +0200 Subject: [PATCH 4/4] add tests for error paths --- internal/metrics/yaml_test.go | 48 +++++++++++++++++++++++++++++++++++ internal/sources/yaml_test.go | 31 ++++++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/internal/metrics/yaml_test.go b/internal/metrics/yaml_test.go index 456189e3bd..3864591c6c 100644 --- a/internal/metrics/yaml_test.go +++ b/internal/metrics/yaml_test.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "sync" "testing" "time" @@ -474,3 +475,50 @@ func TestConcurrentPresetUpdates(t *testing.T) { a.NoError(err) a.Equal(numGoroutines, len(finalMetrics.PresetDefs), "Some updates were lost due to race condition!") } + +func TestGetMetricsNonExistentPath(t *testing.T) { + nonExistent := filepath.Join(t.TempDir(), "does_not_exist", "metrics.yaml") + fmr, err := metrics.NewYAMLMetricReaderWriter(ctx, nonExistent) + assert.NoError(t, err) + _, err = fmr.GetMetrics() + assert.Error(t, err) +} + +func TestGetMetricsDirWithInvalidYAML(t *testing.T) { + dir := t.TempDir() + err := os.WriteFile(filepath.Join(dir, "bad.yaml"), []byte("invalid: yaml: {unclosed"), 0644) + assert.NoError(t, err) + fmr, err := metrics.NewYAMLMetricReaderWriter(ctx, dir) + assert.NoError(t, err) + _, err = fmr.GetMetrics() + assert.Error(t, err) +} + +func TestMutationsGetMetricsError(t *testing.T) { + nonExistent := filepath.Join(t.TempDir(), "does_not_exist", "metrics.yaml") + fmr, err := metrics.NewYAMLMetricReaderWriter(ctx, nonExistent) + assert.NoError(t, err) + + assert.Error(t, fmr.DeleteMetric("x")) + assert.Error(t, fmr.UpdateMetric("x", metrics.Metric{})) + assert.Error(t, fmr.CreateMetric("x", metrics.Metric{})) + assert.Error(t, fmr.DeletePreset("x")) + assert.Error(t, fmr.UpdatePreset("x", metrics.Preset{})) + assert.Error(t, fmr.CreatePreset("x", metrics.Preset{})) +} + +func TestLoadMetricsUnreadableFile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("cannot reliably test file permissions on Windows") + } + if os.Getuid() == 0 { + t.Skip("running as root, permission checks do not apply") + } + f := filepath.Join(t.TempDir(), "metrics.yaml") + err := os.WriteFile(f, []byte(""), 0000) + assert.NoError(t, err) + fmr, err := metrics.NewYAMLMetricReaderWriter(ctx, f) + assert.NoError(t, err) + _, err = fmr.GetMetrics() + assert.Error(t, err) +} diff --git a/internal/sources/yaml_test.go b/internal/sources/yaml_test.go index 57663ffeea..857c8742b6 100644 --- a/internal/sources/yaml_test.go +++ b/internal/sources/yaml_test.go @@ -385,3 +385,34 @@ func TestConcurrentSourceUpdates(t *testing.T) { a.NoError(err) a.Equal(numGoroutines, len(finalSources), "Some updates were lost due to race condition!") } + +func TestGetSourcesDirWithInvalidYAML(t *testing.T) { + dir := t.TempDir() + err := os.WriteFile(filepath.Join(dir, "bad.yaml"), []byte("invalid: yaml: {unclosed"), 0644) + assert.NoError(t, err) + yamlrw, err := sources.NewYAMLSourcesReaderWriter(ctx, dir) + assert.NoError(t, err) + _, err = yamlrw.GetSources() + assert.Error(t, err) +} + +func TestCreateSourceGetSourcesError(t *testing.T) { + nonExistent := filepath.Join(t.TempDir(), "does_not_exist", "sources.yaml") + yamlrw, err := sources.NewYAMLSourcesReaderWriter(ctx, nonExistent) + assert.NoError(t, err) + assert.Error(t, yamlrw.CreateSource(sources.Source{Name: "x"})) +} + +// TestMutationsWriteError verifies that UpdateSource, DeleteSource, and CreateSource +// propagate write errors when the path is a directory (cannot be overwritten as a file). +func TestMutationsWriteError(t *testing.T) { + // Using a dir as the file path: getSources succeeds (reads empty dir), + // but writeSources fails because os.WriteFile cannot write to a directory. + dir := t.TempDir() + yamlrw, err := sources.NewYAMLSourcesReaderWriter(ctx, dir) + assert.NoError(t, err) + + assert.Error(t, yamlrw.UpdateSource(sources.Source{Name: "x"})) + assert.Error(t, yamlrw.DeleteSource("x")) + assert.Error(t, yamlrw.CreateSource(sources.Source{Name: "x"})) +}