From c735381b97887fb94fe5777c85229aa4d82221bc Mon Sep 17 00:00:00 2001 From: kat <28567881+hk21702@users.noreply.github.com> Date: Wed, 29 Jan 2025 18:04:33 -0500 Subject: [PATCH] Report package for handling report files --- internal/report/export_test.go | 10 + internal/report/report.go | 223 ++++++++++++++++++ .../report_test.go} | 84 ++++--- .../TestGetAll}/golden/empty_directory | 0 .../TestGetAll/golden/files_in_subdir | 12 + .../TestGetAll}/golden/invalid_file_extension | 0 .../TestGetAll}/golden/invalid_file_names | 0 .../golden/mix_of_valid_and_invalid | 18 ++ .../TestGetForPeriod/golden/empty_directory | 3 + .../TestGetForPeriod/golden/empty_subdir | 3 + .../TestGetForPeriod/golden/files_in_subdir | 3 + .../golden/invalid_file_extension | 3 + .../golden/invalid_file_names | 3 + .../golden/negative_timestamp | 3 + .../golden/not_inclusive_period | 3 + .../golden/specific_time_single_valid_report | 3 + .../TestGetPerPeriod}/golden/empty_directory | 0 .../TestGetPerPeriod}/golden/files_in_subdir | 0 .../TestGetPerPeriod/golden/get_all_reports | 28 +++ .../golden/get_newest_of_period | 4 + .../golden/invalid_file_extension | 0 .../golden/invalid_file_names | 0 .../golden/mix_of_valid_and_invalid | 4 + .../golden/multiple_consecutive_windows | 12 + .../golden/multiple_non-consecutive_windows | 12 + .../testdata/TestNew/golden/valid_report | 3 + .../TestNew/golden/valid_report_with_path | 3 + internal/reportutils/reportutils.go | 177 -------------- .../TestGetAllReports/golden/files_in_subdir | 2 - .../golden/mix_of_valid_and_invalid | 3 - .../TestGetReportPath/golden/empty_directory | 0 .../TestGetReportPath/golden/empty_subdir | 0 .../TestGetReportPath/golden/files_in_subdir | 0 .../golden/invalid_file_extension | 0 .../golden/invalid_file_names | 0 .../golden/negative_timestamp | 1 - .../golden/not_inclusive_period | 1 - .../golden/specific_time_single_valid_report | 1 - .../TestGetReportTime/golden/alt_extension | 1 - .../golden/valid_report_time | 1 - .../golden/valid_report_time_with_path | 1 - .../TestGetReports/golden/get_all_reports | 7 - .../golden/get_newest_of_period | 1 - .../golden/mix_of_valid_and_invalid | 1 - .../golden/multiple_consecutive_windows | 3 - .../golden/multiple_consequtive_windows | 3 - .../golden/multiple_non-consecutive_windows | 3 - .../golden/multiple_non-consequtive_windows | 3 - internal/uploader/internal_test.go | 19 +- internal/uploader/upload.go | 53 ++--- 50 files changed, 432 insertions(+), 286 deletions(-) create mode 100644 internal/report/export_test.go create mode 100644 internal/report/report.go rename internal/{reportutils/reportutils_test.go => report/report_test.go} (79%) rename internal/{reportutils/testdata/TestGetAllReports => report/testdata/TestGetAll}/golden/empty_directory (100%) create mode 100644 internal/report/testdata/TestGetAll/golden/files_in_subdir rename internal/{reportutils/testdata/TestGetAllReports => report/testdata/TestGetAll}/golden/invalid_file_extension (100%) rename internal/{reportutils/testdata/TestGetAllReports => report/testdata/TestGetAll}/golden/invalid_file_names (100%) create mode 100644 internal/report/testdata/TestGetAll/golden/mix_of_valid_and_invalid create mode 100644 internal/report/testdata/TestGetForPeriod/golden/empty_directory create mode 100644 internal/report/testdata/TestGetForPeriod/golden/empty_subdir create mode 100644 internal/report/testdata/TestGetForPeriod/golden/files_in_subdir create mode 100644 internal/report/testdata/TestGetForPeriod/golden/invalid_file_extension create mode 100644 internal/report/testdata/TestGetForPeriod/golden/invalid_file_names create mode 100644 internal/report/testdata/TestGetForPeriod/golden/negative_timestamp create mode 100644 internal/report/testdata/TestGetForPeriod/golden/not_inclusive_period create mode 100644 internal/report/testdata/TestGetForPeriod/golden/specific_time_single_valid_report rename internal/{reportutils/testdata/TestGetReports => report/testdata/TestGetPerPeriod}/golden/empty_directory (100%) rename internal/{reportutils/testdata/TestGetReports => report/testdata/TestGetPerPeriod}/golden/files_in_subdir (100%) create mode 100644 internal/report/testdata/TestGetPerPeriod/golden/get_all_reports create mode 100644 internal/report/testdata/TestGetPerPeriod/golden/get_newest_of_period rename internal/{reportutils/testdata/TestGetReports => report/testdata/TestGetPerPeriod}/golden/invalid_file_extension (100%) rename internal/{reportutils/testdata/TestGetReports => report/testdata/TestGetPerPeriod}/golden/invalid_file_names (100%) create mode 100644 internal/report/testdata/TestGetPerPeriod/golden/mix_of_valid_and_invalid create mode 100644 internal/report/testdata/TestGetPerPeriod/golden/multiple_consecutive_windows create mode 100644 internal/report/testdata/TestGetPerPeriod/golden/multiple_non-consecutive_windows create mode 100644 internal/report/testdata/TestNew/golden/valid_report create mode 100644 internal/report/testdata/TestNew/golden/valid_report_with_path delete mode 100644 internal/reportutils/reportutils.go delete mode 100644 internal/reportutils/testdata/TestGetAllReports/golden/files_in_subdir delete mode 100644 internal/reportutils/testdata/TestGetAllReports/golden/mix_of_valid_and_invalid delete mode 100644 internal/reportutils/testdata/TestGetReportPath/golden/empty_directory delete mode 100644 internal/reportutils/testdata/TestGetReportPath/golden/empty_subdir delete mode 100644 internal/reportutils/testdata/TestGetReportPath/golden/files_in_subdir delete mode 100644 internal/reportutils/testdata/TestGetReportPath/golden/invalid_file_extension delete mode 100644 internal/reportutils/testdata/TestGetReportPath/golden/invalid_file_names delete mode 100644 internal/reportutils/testdata/TestGetReportPath/golden/negative_timestamp delete mode 100644 internal/reportutils/testdata/TestGetReportPath/golden/not_inclusive_period delete mode 100644 internal/reportutils/testdata/TestGetReportPath/golden/specific_time_single_valid_report delete mode 100644 internal/reportutils/testdata/TestGetReportTime/golden/alt_extension delete mode 100644 internal/reportutils/testdata/TestGetReportTime/golden/valid_report_time delete mode 100644 internal/reportutils/testdata/TestGetReportTime/golden/valid_report_time_with_path delete mode 100644 internal/reportutils/testdata/TestGetReports/golden/get_all_reports delete mode 100644 internal/reportutils/testdata/TestGetReports/golden/get_newest_of_period delete mode 100644 internal/reportutils/testdata/TestGetReports/golden/mix_of_valid_and_invalid delete mode 100644 internal/reportutils/testdata/TestGetReports/golden/multiple_consecutive_windows delete mode 100644 internal/reportutils/testdata/TestGetReports/golden/multiple_consequtive_windows delete mode 100644 internal/reportutils/testdata/TestGetReports/golden/multiple_non-consecutive_windows delete mode 100644 internal/reportutils/testdata/TestGetReports/golden/multiple_non-consequtive_windows diff --git a/internal/report/export_test.go b/internal/report/export_test.go new file mode 100644 index 0000000..6a8629f --- /dev/null +++ b/internal/report/export_test.go @@ -0,0 +1,10 @@ +package report + +type ReportStash = reportStash + +// ReportStash returns the reportStash of the report. +// +//nolint:revive // This is a false positive as we returned a typed alias and not the private type. +func (r Report) ReportStash() ReportStash { + return r.reportStash +} diff --git a/internal/report/report.go b/internal/report/report.go new file mode 100644 index 0000000..eb22687 --- /dev/null +++ b/internal/report/report.go @@ -0,0 +1,223 @@ +// Package report provides utility functions for handling reports. +package report + +// package report + +import ( + "encoding/json" + "errors" + "fmt" + "log/slog" + "os" + "path/filepath" + "strconv" + "strings" + "time" + + "github.com/ubuntu/ubuntu-insights/internal/constants" +) + +var ( + // ErrInvalidPeriod is returned when a function requiring a period, received an invalid, period that isn't a non-negative integer. + ErrInvalidPeriod = errors.New("invalid period, period should be a positive integer") + + // ErrInvalidReportExt is returned when a report file has an invalid extension. + ErrInvalidReportExt = errors.New("invalid report file extension") + + // ErrInvalidReportName is returned when a report file has an invalid name that can't be parsed. + ErrInvalidReportName = errors.New("invalid report file name") +) + +// Report represents a report file. +type Report struct { + Path string // Path is the path to the report. + Name string // Name is the name of the report file, including extension. + TimeStamp int64 // TimeStamp is the timestamp of the report. + + reportStash reportStash +} + +// reportStash is a helper struct to store a report and its data for movement. +type reportStash struct { + Path string + Data []byte +} + +// New creates a new Report object from a path. +// It does not write to the file system, or validate the path. +func New(path string) (Report, error) { + if filepath.Ext(path) != constants.ReportExt { + return Report{}, ErrInvalidReportExt + } + + rTime, err := getReportTime(filepath.Base(path)) + if err != nil { + return Report{}, err + } + + return Report{Path: path, Name: filepath.Base(path), TimeStamp: rTime}, nil +} + +// ReadJSON reads the JSON data from the report file. +func (r Report) ReadJSON() ([]byte, error) { + // Read the report file + data, err := os.ReadFile(r.Path) + if err != nil { + return nil, fmt.Errorf("failed to read report file: %v", err) + } + + if !json.Valid(data) { + return nil, fmt.Errorf("invalid JSON data in report file") + } + + return data, nil +} + +// getReportTime returns a int64 representation of the report time from the report path. +func getReportTime(path string) (int64, error) { + fileName := filepath.Base(path) + i, err := strconv.ParseInt(strings.TrimSuffix(fileName, filepath.Ext(fileName)), 10, 64) + if err != nil { + return i, fmt.Errorf("%w: %v", ErrInvalidReportName, err) + } + return i, nil +} + +// GetPeriodStart returns the start of the period window for a given period in seconds. +func GetPeriodStart(period int) (int64, error) { + if period <= 0 { + return 0, ErrInvalidPeriod + } + utcTime := time.Now().UTC().Unix() + return utcTime - (utcTime % int64(period)), nil +} + +// GetForPeriod returns the most recent report within a period window for a given directory. +// Not inclusive of the period end (periodStart + period). +// +// For example, given reports 1 and 7, with time 2 and period 7, the function will return the path for report 1. +func GetForPeriod(dir string, time time.Time, period int) (Report, error) { + if period <= 0 { + return Report{}, ErrInvalidPeriod + } + + periodStart := time.Unix() - (time.Unix() % int64(period)) + periodEnd := periodStart + int64(period) + + // Reports names are utc timestamps. Get the most recent report within the period window. + var report Report + err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { + if err != nil { + slog.Error("Failed to access path", "path", path, "error", err) + return err + } + + // Skip subdirectories. + if d.IsDir() && path != dir { + return filepath.SkipDir + } + + r, err := New(path) + if errors.Is(err, ErrInvalidReportExt) || errors.Is(err, ErrInvalidReportName) { + slog.Info("Skipping non-report file", "file", d.Name(), "error", err) + return nil + } else if err != nil { + slog.Error("Failed to create report object", "error", err) + return err + } + + if r.TimeStamp < periodStart { + return nil + } + if r.TimeStamp >= periodEnd { + return filepath.SkipDir + } + + report = r + return nil + }) + + if err != nil { + return Report{}, err + } + + return report, nil +} + +// GetPerPeriod returns the latest report within each period window for a given directory. +// The key of the map is the start of the period window, and the value is a Report object. +// +// If period is 1, then all reports in the dir are returned. +func GetPerPeriod(dir string, period int) (map[int64]Report, error) { + if period <= 0 { + return nil, ErrInvalidPeriod + } + + reports := make(map[int64]Report) + err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { + if err != nil { + slog.Error("Failed to access path", "path", path, "error", err) + return err + } + + if d.IsDir() && path != dir { + return filepath.SkipDir + } + + r, err := New(path) + if errors.Is(err, ErrInvalidReportExt) || errors.Is(err, ErrInvalidReportName) { + slog.Info("Skipping non-report file", "file", d.Name(), "error", err) + return nil + } else if err != nil { + slog.Error("Failed to create report object", "error", err) + return err + } + + periodStart := r.TimeStamp - (r.TimeStamp % int64(period)) + if existingReport, ok := reports[periodStart]; !ok || existingReport.TimeStamp < r.TimeStamp { + reports[periodStart] = r + } + + return nil + }) + + if err != nil { + return nil, err + } + + return reports, nil +} + +// GetAll returns all reports in a given directory. +// Reports are expected to have the correct file extension, and have a name which can be parsed by a timestamp. +// Does not traverse subdirectories. +func GetAll(dir string) ([]Report, error) { + reports := make([]Report, 0) + err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { + if err != nil { + slog.Error("Failed to access path", "path", path, "error", err) + return err + } + + if d.IsDir() && path != dir { + return filepath.SkipDir + } + + r, err := New(path) + if errors.Is(err, ErrInvalidReportExt) || errors.Is(err, ErrInvalidReportName) { + slog.Info("Skipping non-report file", "file", d.Name(), "error", err) + return nil + } else if err != nil { + slog.Error("Failed to create report object", "error", err) + return err + } + + reports = append(reports, r) + return nil + }) + if err != nil { + return nil, err + } + + return reports, nil +} diff --git a/internal/reportutils/reportutils_test.go b/internal/report/report_test.go similarity index 79% rename from internal/reportutils/reportutils_test.go rename to internal/report/report_test.go index 9df7102..96b41be 100644 --- a/internal/reportutils/reportutils_test.go +++ b/internal/report/report_test.go @@ -1,12 +1,13 @@ -package reportutils_test +package report_test import ( "os" "path/filepath" "testing" + "time" "github.com/stretchr/testify/require" - "github.com/ubuntu/ubuntu-insights/internal/reportutils" + report "github.com/ubuntu/ubuntu-insights/internal/report" "github.com/ubuntu/ubuntu-insights/internal/testutils" ) @@ -20,15 +21,15 @@ func TestGetPeriodStart(t *testing.T) { }{ "Valid Period": {period: 500}, - "Invalid Negative Period": {period: -500, wantErr: reportutils.ErrInvalidPeriod}, - "Invalid Zero Period": {period: 0, wantErr: reportutils.ErrInvalidPeriod}, + "Invalid Negative Period": {period: -500, wantErr: report.ErrInvalidPeriod}, + "Invalid Zero Period": {period: 0, wantErr: report.ErrInvalidPeriod}, } for name, tc := range tests { t.Run(name, func(t *testing.T) { t.Parallel() - got, err := reportutils.GetPeriodStart(tc.period) + got, err := report.GetPeriodStart(tc.period) if tc.wantErr != nil { require.ErrorIs(t, err, tc.wantErr) return @@ -40,25 +41,28 @@ func TestGetPeriodStart(t *testing.T) { } } -func TestGetReportTime(t *testing.T) { +func TestNew(t *testing.T) { + t.Parallel() + tests := map[string]struct { path string wantErr bool }{ - "Valid Report Time": {path: "1627847285.json", wantErr: false}, - "Valid Report Time with Path": {path: "/some/dir/1627847285.json", wantErr: false}, - "Alt Extension": {path: "1627847285.txt", wantErr: false}, + "Valid Report": {path: "1627847285.json"}, + "Valid Report with Path": {path: "/some/dir/1627847285.json"}, "Empty File Name": {path: ".json", wantErr: true}, "Invalid Report Time": {path: "invalid.json", wantErr: true}, "Invalid Report Mixed": {path: "i-1.json", wantErr: true}, "Invalid Report Time with Path": {path: "/123/123/invalid.json", wantErr: true}, + "Alt Extension": {path: "1627847285.txt", wantErr: true}, } for name, tc := range tests { t.Run(name, func(t *testing.T) { - got, err := reportutils.GetReportTime(tc.path) + t.Parallel() + got, err := report.New(tc.path) if tc.wantErr { require.Error(t, err, "expected an error but got none") return @@ -66,12 +70,12 @@ func TestGetReportTime(t *testing.T) { require.NoError(t, err, "got an unexpected error") want := testutils.LoadWithUpdateFromGoldenYAML(t, got) - require.Equal(t, want, got, "GetReportTime should return the report time from the report path") + require.Equal(t, want, got, "New should return a new report object") }) } } -func TestGetReportPath(t *testing.T) { +func TestGetForPeriod(t *testing.T) { t.Parallel() tests := map[string]struct { @@ -95,8 +99,8 @@ func TestGetReportPath(t *testing.T) { "Negative Timestamp": {files: []string{"-100.json", "-101.json"}, time: -150, period: 100}, "Not Inclusive Period": {files: []string{"1.json", "7.json"}, time: 2, period: 7}, - "Invalid Negative Period": {files: []string{"1.json", "7.json"}, time: 2, period: -7, wantSpecificErr: reportutils.ErrInvalidPeriod}, - "Invalid Zero Period": {files: []string{"1.json", "7.json"}, time: 2, period: 0, wantSpecificErr: reportutils.ErrInvalidPeriod}, + "Invalid Negative Period": {files: []string{"1.json", "7.json"}, time: 2, period: -7, wantSpecificErr: report.ErrInvalidPeriod}, + "Invalid Zero Period": {files: []string{"1.json", "7.json"}, time: 2, period: 0, wantSpecificErr: report.ErrInvalidPeriod}, "Invalid Dir": {period: 1, invalidDir: true, wantGenericErr: true}, } @@ -107,12 +111,11 @@ func TestGetReportPath(t *testing.T) { dir, err := setupTmpDir(t, tc.files, tc.subDir, tc.subDirFiles) require.NoError(t, err, "Setup: failed to setup temporary directory") - defer os.RemoveAll(dir) if tc.invalidDir { dir = filepath.Join(dir, "invalid dir") } - got, err := reportutils.GetReportPath(dir, tc.time, tc.period) + r, err := report.GetForPeriod(dir, time.Unix(tc.time, 0), tc.period) if tc.wantSpecificErr != nil { require.ErrorIs(t, err, tc.wantSpecificErr) return @@ -121,21 +124,17 @@ func TestGetReportPath(t *testing.T) { require.Error(t, err, "expected an error but got none") return } - require.NoError(t, err, "got an unexpected error") - if got != "" { - got, err = filepath.Rel(dir, got) - require.NoError(t, err, "failed to get relative path") - } + got := sanitizeReportPath(t, r, dir) - want := testutils.LoadWithUpdateFromGolden(t, got) + want := testutils.LoadWithUpdateFromGoldenYAML(t, got) require.Equal(t, want, got, "GetReportPath should return the most recent report within the period window") }) } } -func TestGetReports(t *testing.T) { +func TestGetPerPeriod(t *testing.T) { t.Parallel() tests := map[string]struct { @@ -160,8 +159,8 @@ func TestGetReports(t *testing.T) { "Multiple Non-Consecutive Windows": {files: []string{"1.json", "7.json", "101.json", "107.json", "251.json", "257.json"}, period: 50}, "Get All Reports": {files: []string{"1.json", "2.json", "3.json", "101.json", "107.json", "251.json", "257.json"}, period: 1}, - "Invalid Negative Period": {files: []string{"1.json", "7.json"}, period: -7, wantSpecificErr: reportutils.ErrInvalidPeriod}, - "Invalid Zero Period": {files: []string{"1.json", "7.json"}, period: 0, wantSpecificErr: reportutils.ErrInvalidPeriod}, + "Invalid Negative Period": {files: []string{"1.json", "7.json"}, period: -7, wantSpecificErr: report.ErrInvalidPeriod}, + "Invalid Zero Period": {files: []string{"1.json", "7.json"}, period: 0, wantSpecificErr: report.ErrInvalidPeriod}, "Invalid Dir": {period: 1, invalidDir: true, wantGenericErr: true}, } @@ -172,12 +171,11 @@ func TestGetReports(t *testing.T) { dir, err := setupTmpDir(t, tc.files, tc.subDir, tc.subDirFiles) require.NoError(t, err, "Setup: failed to setup temporary directory") - defer os.RemoveAll(dir) if tc.invalidDir { dir = filepath.Join(dir, "invalid dir") } - got, err := reportutils.GetReports(dir, tc.period) + reports, err := report.GetPerPeriod(dir, tc.period) if tc.wantSpecificErr != nil { require.ErrorIs(t, err, tc.wantSpecificErr) return @@ -188,13 +186,17 @@ func TestGetReports(t *testing.T) { } require.NoError(t, err, "got an unexpected error") + got := make(map[int64]report.Report, len(reports)) + for n, r := range reports { + got[n] = sanitizeReportPath(t, r, dir) + } want := testutils.LoadWithUpdateFromGoldenYAML(t, got) require.Equal(t, want, got, "GetReports should return the most recent report within each period window") }) } } -func TestGetAllReports(t *testing.T) { +func TestGetAll(t *testing.T) { t.Parallel() tests := map[string]struct { @@ -219,18 +221,21 @@ func TestGetAllReports(t *testing.T) { t.Parallel() dir, err := setupTmpDir(t, tc.files, tc.subDir, tc.subDirFiles) require.NoError(t, err, "Setup: failed to setup temporary directory") - defer os.RemoveAll(dir) if tc.invalidDir { dir = filepath.Join(dir, "invalid dir") } - got, err := reportutils.GetAllReports(dir) + reports, err := report.GetAll(dir) if tc.wantErr { require.Error(t, err, "expected an error but got none") return } require.NoError(t, err, "got an unexpected error") + got := make([]report.Report, len(reports)) + for _, r := range reports { + got = append(got, sanitizeReportPath(t, r, dir)) + } want := testutils.LoadWithUpdateFromGoldenYAML(t, got) require.Equal(t, want, got, "GetAllReports should return all reports in the directory") }) @@ -240,10 +245,7 @@ func TestGetAllReports(t *testing.T) { func setupTmpDir(t *testing.T, files []string, subDir string, subDirFiles []string) (string, error) { t.Helper() - dir, err := os.MkdirTemp("", "reportutils-test") - if err != nil { - return "", err - } + dir := t.TempDir() for _, file := range files { path := filepath.Join(dir, file) @@ -268,3 +270,17 @@ func setupTmpDir(t *testing.T, files []string, subDir string, subDirFiles []stri return dir, nil } + +func sanitizeReportPath(t *testing.T, r report.Report, dir string) report.Report { + t.Helper() + if r.Path == "" { + return r + } + + fp, err := filepath.Rel(dir, r.Path) + if err != nil { + require.NoError(t, err, "failed to get relative path") + return report.Report{} + } + return report.Report{Path: fp, Name: r.Name, TimeStamp: r.TimeStamp} +} diff --git a/internal/reportutils/testdata/TestGetAllReports/golden/empty_directory b/internal/report/testdata/TestGetAll/golden/empty_directory similarity index 100% rename from internal/reportutils/testdata/TestGetAllReports/golden/empty_directory rename to internal/report/testdata/TestGetAll/golden/empty_directory diff --git a/internal/report/testdata/TestGetAll/golden/files_in_subdir b/internal/report/testdata/TestGetAll/golden/files_in_subdir new file mode 100644 index 0000000..ccc5761 --- /dev/null +++ b/internal/report/testdata/TestGetAll/golden/files_in_subdir @@ -0,0 +1,12 @@ +- path: "" + name: "" + timestamp: 0 +- path: "" + name: "" + timestamp: 0 +- path: 1.json + name: 1.json + timestamp: 1 +- path: 2.json + name: 2.json + timestamp: 2 diff --git a/internal/reportutils/testdata/TestGetAllReports/golden/invalid_file_extension b/internal/report/testdata/TestGetAll/golden/invalid_file_extension similarity index 100% rename from internal/reportutils/testdata/TestGetAllReports/golden/invalid_file_extension rename to internal/report/testdata/TestGetAll/golden/invalid_file_extension diff --git a/internal/reportutils/testdata/TestGetAllReports/golden/invalid_file_names b/internal/report/testdata/TestGetAll/golden/invalid_file_names similarity index 100% rename from internal/reportutils/testdata/TestGetAllReports/golden/invalid_file_names rename to internal/report/testdata/TestGetAll/golden/invalid_file_names diff --git a/internal/report/testdata/TestGetAll/golden/mix_of_valid_and_invalid b/internal/report/testdata/TestGetAll/golden/mix_of_valid_and_invalid new file mode 100644 index 0000000..e2ad51f --- /dev/null +++ b/internal/report/testdata/TestGetAll/golden/mix_of_valid_and_invalid @@ -0,0 +1,18 @@ +- path: "" + name: "" + timestamp: 0 +- path: "" + name: "" + timestamp: 0 +- path: "" + name: "" + timestamp: 0 +- path: 1.json + name: 1.json + timestamp: 1 +- path: 2.json + name: 2.json + timestamp: 2 +- path: 500.json + name: 500.json + timestamp: 500 diff --git a/internal/report/testdata/TestGetForPeriod/golden/empty_directory b/internal/report/testdata/TestGetForPeriod/golden/empty_directory new file mode 100644 index 0000000..ec9107d --- /dev/null +++ b/internal/report/testdata/TestGetForPeriod/golden/empty_directory @@ -0,0 +1,3 @@ +path: "" +name: "" +timestamp: 0 diff --git a/internal/report/testdata/TestGetForPeriod/golden/empty_subdir b/internal/report/testdata/TestGetForPeriod/golden/empty_subdir new file mode 100644 index 0000000..ec9107d --- /dev/null +++ b/internal/report/testdata/TestGetForPeriod/golden/empty_subdir @@ -0,0 +1,3 @@ +path: "" +name: "" +timestamp: 0 diff --git a/internal/report/testdata/TestGetForPeriod/golden/files_in_subdir b/internal/report/testdata/TestGetForPeriod/golden/files_in_subdir new file mode 100644 index 0000000..ec9107d --- /dev/null +++ b/internal/report/testdata/TestGetForPeriod/golden/files_in_subdir @@ -0,0 +1,3 @@ +path: "" +name: "" +timestamp: 0 diff --git a/internal/report/testdata/TestGetForPeriod/golden/invalid_file_extension b/internal/report/testdata/TestGetForPeriod/golden/invalid_file_extension new file mode 100644 index 0000000..ec9107d --- /dev/null +++ b/internal/report/testdata/TestGetForPeriod/golden/invalid_file_extension @@ -0,0 +1,3 @@ +path: "" +name: "" +timestamp: 0 diff --git a/internal/report/testdata/TestGetForPeriod/golden/invalid_file_names b/internal/report/testdata/TestGetForPeriod/golden/invalid_file_names new file mode 100644 index 0000000..ec9107d --- /dev/null +++ b/internal/report/testdata/TestGetForPeriod/golden/invalid_file_names @@ -0,0 +1,3 @@ +path: "" +name: "" +timestamp: 0 diff --git a/internal/report/testdata/TestGetForPeriod/golden/negative_timestamp b/internal/report/testdata/TestGetForPeriod/golden/negative_timestamp new file mode 100644 index 0000000..431aa05 --- /dev/null +++ b/internal/report/testdata/TestGetForPeriod/golden/negative_timestamp @@ -0,0 +1,3 @@ +path: -100.json +name: -100.json +timestamp: -100 diff --git a/internal/report/testdata/TestGetForPeriod/golden/not_inclusive_period b/internal/report/testdata/TestGetForPeriod/golden/not_inclusive_period new file mode 100644 index 0000000..56c8ba5 --- /dev/null +++ b/internal/report/testdata/TestGetForPeriod/golden/not_inclusive_period @@ -0,0 +1,3 @@ +path: 1.json +name: 1.json +timestamp: 1 diff --git a/internal/report/testdata/TestGetForPeriod/golden/specific_time_single_valid_report b/internal/report/testdata/TestGetForPeriod/golden/specific_time_single_valid_report new file mode 100644 index 0000000..7c4d2c9 --- /dev/null +++ b/internal/report/testdata/TestGetForPeriod/golden/specific_time_single_valid_report @@ -0,0 +1,3 @@ +path: 2.json +name: 2.json +timestamp: 2 diff --git a/internal/reportutils/testdata/TestGetReports/golden/empty_directory b/internal/report/testdata/TestGetPerPeriod/golden/empty_directory similarity index 100% rename from internal/reportutils/testdata/TestGetReports/golden/empty_directory rename to internal/report/testdata/TestGetPerPeriod/golden/empty_directory diff --git a/internal/reportutils/testdata/TestGetReports/golden/files_in_subdir b/internal/report/testdata/TestGetPerPeriod/golden/files_in_subdir similarity index 100% rename from internal/reportutils/testdata/TestGetReports/golden/files_in_subdir rename to internal/report/testdata/TestGetPerPeriod/golden/files_in_subdir diff --git a/internal/report/testdata/TestGetPerPeriod/golden/get_all_reports b/internal/report/testdata/TestGetPerPeriod/golden/get_all_reports new file mode 100644 index 0000000..e3e067a --- /dev/null +++ b/internal/report/testdata/TestGetPerPeriod/golden/get_all_reports @@ -0,0 +1,28 @@ +1: + path: 1.json + name: 1.json + timestamp: 1 +2: + path: 2.json + name: 2.json + timestamp: 2 +3: + path: 3.json + name: 3.json + timestamp: 3 +101: + path: 101.json + name: 101.json + timestamp: 101 +107: + path: 107.json + name: 107.json + timestamp: 107 +251: + path: 251.json + name: 251.json + timestamp: 251 +257: + path: 257.json + name: 257.json + timestamp: 257 diff --git a/internal/report/testdata/TestGetPerPeriod/golden/get_newest_of_period b/internal/report/testdata/TestGetPerPeriod/golden/get_newest_of_period new file mode 100644 index 0000000..d6fdc70 --- /dev/null +++ b/internal/report/testdata/TestGetPerPeriod/golden/get_newest_of_period @@ -0,0 +1,4 @@ +0: + path: 7.json + name: 7.json + timestamp: 7 diff --git a/internal/reportutils/testdata/TestGetReports/golden/invalid_file_extension b/internal/report/testdata/TestGetPerPeriod/golden/invalid_file_extension similarity index 100% rename from internal/reportutils/testdata/TestGetReports/golden/invalid_file_extension rename to internal/report/testdata/TestGetPerPeriod/golden/invalid_file_extension diff --git a/internal/reportutils/testdata/TestGetReports/golden/invalid_file_names b/internal/report/testdata/TestGetPerPeriod/golden/invalid_file_names similarity index 100% rename from internal/reportutils/testdata/TestGetReports/golden/invalid_file_names rename to internal/report/testdata/TestGetPerPeriod/golden/invalid_file_names diff --git a/internal/report/testdata/TestGetPerPeriod/golden/mix_of_valid_and_invalid b/internal/report/testdata/TestGetPerPeriod/golden/mix_of_valid_and_invalid new file mode 100644 index 0000000..5f45d53 --- /dev/null +++ b/internal/report/testdata/TestGetPerPeriod/golden/mix_of_valid_and_invalid @@ -0,0 +1,4 @@ +0: + path: 2.json + name: 2.json + timestamp: 2 diff --git a/internal/report/testdata/TestGetPerPeriod/golden/multiple_consecutive_windows b/internal/report/testdata/TestGetPerPeriod/golden/multiple_consecutive_windows new file mode 100644 index 0000000..d777da2 --- /dev/null +++ b/internal/report/testdata/TestGetPerPeriod/golden/multiple_consecutive_windows @@ -0,0 +1,12 @@ +0: + path: 7.json + name: 7.json + timestamp: 7 +100: + path: 107.json + name: 107.json + timestamp: 107 +200: + path: 207.json + name: 207.json + timestamp: 207 diff --git a/internal/report/testdata/TestGetPerPeriod/golden/multiple_non-consecutive_windows b/internal/report/testdata/TestGetPerPeriod/golden/multiple_non-consecutive_windows new file mode 100644 index 0000000..9fb936e --- /dev/null +++ b/internal/report/testdata/TestGetPerPeriod/golden/multiple_non-consecutive_windows @@ -0,0 +1,12 @@ +0: + path: 7.json + name: 7.json + timestamp: 7 +100: + path: 107.json + name: 107.json + timestamp: 107 +250: + path: 257.json + name: 257.json + timestamp: 257 diff --git a/internal/report/testdata/TestNew/golden/valid_report b/internal/report/testdata/TestNew/golden/valid_report new file mode 100644 index 0000000..7ce2121 --- /dev/null +++ b/internal/report/testdata/TestNew/golden/valid_report @@ -0,0 +1,3 @@ +path: 1627847285.json +name: 1627847285.json +timestamp: 1627847285 diff --git a/internal/report/testdata/TestNew/golden/valid_report_with_path b/internal/report/testdata/TestNew/golden/valid_report_with_path new file mode 100644 index 0000000..252b842 --- /dev/null +++ b/internal/report/testdata/TestNew/golden/valid_report_with_path @@ -0,0 +1,3 @@ +path: /some/dir/1627847285.json +name: 1627847285.json +timestamp: 1627847285 diff --git a/internal/reportutils/reportutils.go b/internal/reportutils/reportutils.go deleted file mode 100644 index 932d99c..0000000 --- a/internal/reportutils/reportutils.go +++ /dev/null @@ -1,177 +0,0 @@ -// Package reportutils provides utility functions for handling reports. -package reportutils - -// package report - -import ( - "errors" - "log/slog" - "os" - "path/filepath" - "strconv" - "strings" - "time" - - "github.com/ubuntu/ubuntu-insights/internal/constants" -) - -// ErrInvalidPeriod is returned when a function requiring a period, received an invalid, period that isn't a non-negative integer. -var ErrInvalidPeriod = errors.New("invalid period, period should be a positive integer") - -// type Report struct{} - -// GetPeriodStart returns the start of the period window for a given period in seconds. -func GetPeriodStart(period int) (int64, error) { - if period <= 0 { - return 0, ErrInvalidPeriod - } - utcTime := time.Now().UTC().Unix() - return utcTime - (utcTime % int64(period)), nil -} - -// GetReportTime returns a int64 representation of the report time from the report path. -func GetReportTime(path string) (int64, error) { - fileName := filepath.Base(path) - return strconv.ParseInt(strings.TrimSuffix(fileName, filepath.Ext(fileName)), 10, 64) -} - -// Path is a method on Report - -// GetReportPath returns the path for the most recent report within a period window, returning an empty string if no report is found. -// Not inclusive of the period end (periodStart + period). -// -// For example, given reports 1 and 7, with time 2 and period 7, the function will return the path for report 1. -func GetReportPath(dir string, time int64, period int) (string, error) { - if period <= 0 { - return "", ErrInvalidPeriod - } - - periodStart := time - (time % int64(period)) - periodEnd := periodStart + int64(period) - - // Reports names are utc timestamps. Get the most recent report within the period window. - var mostRecentReportPath string - err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { - if err != nil { - slog.Error("Failed to access path", "path", path, "error", err) - return err - } - - // Skip subdirectories. - if d.IsDir() && path != dir { - return filepath.SkipDir - } - - if filepath.Ext(d.Name()) != constants.ReportExt { - slog.Info("Skipping non-report file, invalid extension", "file", d.Name()) - return nil - } - - reportTime, err := GetReportTime(d.Name()) - if err != nil { - slog.Info("Skipping non-report file, invalid file name", "file", d.Name()) - return nil - } - - if reportTime < periodStart { - return nil - } - if reportTime >= periodEnd { - return filepath.SkipDir - } - - mostRecentReportPath = path - return nil - }) - - if err != nil { - return "", err - } - - return mostRecentReportPath, nil -} - -// -> map[int64]Report - -// GetReports returns the paths for the latest report within each period window for a given directory. -// The key of the map is the start of the period window, and the report timestamp. -// -// If period is 1, then all reports in the dir are returned. -func GetReports(dir string, period int) (map[int64]int64, error) { - if period <= 0 { - return nil, ErrInvalidPeriod - } - - reports := make(map[int64]int64) - err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { - if err != nil { - slog.Error("Failed to access path", "path", path, "error", err) - return err - } - - if d.IsDir() && path != dir { - return filepath.SkipDir - } - - if filepath.Ext(d.Name()) != constants.ReportExt { - slog.Info("Skipping non-report file, invalid extension", "file", d.Name()) - return nil - } - - reportTime, err := GetReportTime(d.Name()) - if err != nil { - slog.Info("Skipping non-report file, invalid file name", "file", d.Name()) - return nil - } - - periodStart := reportTime - (reportTime % int64(period)) - if existingReport, ok := reports[periodStart]; !ok || existingReport < reportTime { - reports[periodStart] = reportTime - } - - return nil - }) - - if err != nil { - return nil, err - } - - return reports, nil -} - -// reports.GetAll(dir string) ([]Report, error) - -// GetAllReports returns the filename for all reports within a given directory, which match the expected pattern. -// Does not traverse subdirectories. -func GetAllReports(dir string) ([]string, error) { - reports := make([]string, 0) - err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { - if err != nil { - slog.Error("Failed to access path", "path", path, "error", err) - return err - } - - if d.IsDir() && path != dir { - return filepath.SkipDir - } - - if filepath.Ext(d.Name()) != constants.ReportExt { - slog.Info("Skipping non-report file, invalid extension", "file", d.Name()) - return nil - } - - if _, err := GetReportTime(d.Name()); err != nil { - slog.Info("Skipping non-report file, invalid file name", "file", d.Name()) - return nil - } - - reports = append(reports, d.Name()) - return nil - }) - - if err != nil { - return nil, err - } - - return reports, nil -} diff --git a/internal/reportutils/testdata/TestGetAllReports/golden/files_in_subdir b/internal/reportutils/testdata/TestGetAllReports/golden/files_in_subdir deleted file mode 100644 index 6548887..0000000 --- a/internal/reportutils/testdata/TestGetAllReports/golden/files_in_subdir +++ /dev/null @@ -1,2 +0,0 @@ -- 1.json -- 2.json diff --git a/internal/reportutils/testdata/TestGetAllReports/golden/mix_of_valid_and_invalid b/internal/reportutils/testdata/TestGetAllReports/golden/mix_of_valid_and_invalid deleted file mode 100644 index a8eb048..0000000 --- a/internal/reportutils/testdata/TestGetAllReports/golden/mix_of_valid_and_invalid +++ /dev/null @@ -1,3 +0,0 @@ -- 1.json -- 2.json -- 500.json diff --git a/internal/reportutils/testdata/TestGetReportPath/golden/empty_directory b/internal/reportutils/testdata/TestGetReportPath/golden/empty_directory deleted file mode 100644 index e69de29..0000000 diff --git a/internal/reportutils/testdata/TestGetReportPath/golden/empty_subdir b/internal/reportutils/testdata/TestGetReportPath/golden/empty_subdir deleted file mode 100644 index e69de29..0000000 diff --git a/internal/reportutils/testdata/TestGetReportPath/golden/files_in_subdir b/internal/reportutils/testdata/TestGetReportPath/golden/files_in_subdir deleted file mode 100644 index e69de29..0000000 diff --git a/internal/reportutils/testdata/TestGetReportPath/golden/invalid_file_extension b/internal/reportutils/testdata/TestGetReportPath/golden/invalid_file_extension deleted file mode 100644 index e69de29..0000000 diff --git a/internal/reportutils/testdata/TestGetReportPath/golden/invalid_file_names b/internal/reportutils/testdata/TestGetReportPath/golden/invalid_file_names deleted file mode 100644 index e69de29..0000000 diff --git a/internal/reportutils/testdata/TestGetReportPath/golden/negative_timestamp b/internal/reportutils/testdata/TestGetReportPath/golden/negative_timestamp deleted file mode 100644 index 411e61e..0000000 --- a/internal/reportutils/testdata/TestGetReportPath/golden/negative_timestamp +++ /dev/null @@ -1 +0,0 @@ --100.json \ No newline at end of file diff --git a/internal/reportutils/testdata/TestGetReportPath/golden/not_inclusive_period b/internal/reportutils/testdata/TestGetReportPath/golden/not_inclusive_period deleted file mode 100644 index a3d3d4c..0000000 --- a/internal/reportutils/testdata/TestGetReportPath/golden/not_inclusive_period +++ /dev/null @@ -1 +0,0 @@ -1.json \ No newline at end of file diff --git a/internal/reportutils/testdata/TestGetReportPath/golden/specific_time_single_valid_report b/internal/reportutils/testdata/TestGetReportPath/golden/specific_time_single_valid_report deleted file mode 100644 index e1f33a0..0000000 --- a/internal/reportutils/testdata/TestGetReportPath/golden/specific_time_single_valid_report +++ /dev/null @@ -1 +0,0 @@ -2.json \ No newline at end of file diff --git a/internal/reportutils/testdata/TestGetReportTime/golden/alt_extension b/internal/reportutils/testdata/TestGetReportTime/golden/alt_extension deleted file mode 100644 index d29665a..0000000 --- a/internal/reportutils/testdata/TestGetReportTime/golden/alt_extension +++ /dev/null @@ -1 +0,0 @@ -1627847285 diff --git a/internal/reportutils/testdata/TestGetReportTime/golden/valid_report_time b/internal/reportutils/testdata/TestGetReportTime/golden/valid_report_time deleted file mode 100644 index d29665a..0000000 --- a/internal/reportutils/testdata/TestGetReportTime/golden/valid_report_time +++ /dev/null @@ -1 +0,0 @@ -1627847285 diff --git a/internal/reportutils/testdata/TestGetReportTime/golden/valid_report_time_with_path b/internal/reportutils/testdata/TestGetReportTime/golden/valid_report_time_with_path deleted file mode 100644 index d29665a..0000000 --- a/internal/reportutils/testdata/TestGetReportTime/golden/valid_report_time_with_path +++ /dev/null @@ -1 +0,0 @@ -1627847285 diff --git a/internal/reportutils/testdata/TestGetReports/golden/get_all_reports b/internal/reportutils/testdata/TestGetReports/golden/get_all_reports deleted file mode 100644 index e295670..0000000 --- a/internal/reportutils/testdata/TestGetReports/golden/get_all_reports +++ /dev/null @@ -1,7 +0,0 @@ -1: 1 -2: 2 -3: 3 -101: 101 -107: 107 -251: 251 -257: 257 diff --git a/internal/reportutils/testdata/TestGetReports/golden/get_newest_of_period b/internal/reportutils/testdata/TestGetReports/golden/get_newest_of_period deleted file mode 100644 index 017ba7a..0000000 --- a/internal/reportutils/testdata/TestGetReports/golden/get_newest_of_period +++ /dev/null @@ -1 +0,0 @@ -0: 7 diff --git a/internal/reportutils/testdata/TestGetReports/golden/mix_of_valid_and_invalid b/internal/reportutils/testdata/TestGetReports/golden/mix_of_valid_and_invalid deleted file mode 100644 index f196b73..0000000 --- a/internal/reportutils/testdata/TestGetReports/golden/mix_of_valid_and_invalid +++ /dev/null @@ -1 +0,0 @@ -0: 2 diff --git a/internal/reportutils/testdata/TestGetReports/golden/multiple_consecutive_windows b/internal/reportutils/testdata/TestGetReports/golden/multiple_consecutive_windows deleted file mode 100644 index 8e9a957..0000000 --- a/internal/reportutils/testdata/TestGetReports/golden/multiple_consecutive_windows +++ /dev/null @@ -1,3 +0,0 @@ -0: 7 -100: 107 -200: 207 diff --git a/internal/reportutils/testdata/TestGetReports/golden/multiple_consequtive_windows b/internal/reportutils/testdata/TestGetReports/golden/multiple_consequtive_windows deleted file mode 100644 index 8e9a957..0000000 --- a/internal/reportutils/testdata/TestGetReports/golden/multiple_consequtive_windows +++ /dev/null @@ -1,3 +0,0 @@ -0: 7 -100: 107 -200: 207 diff --git a/internal/reportutils/testdata/TestGetReports/golden/multiple_non-consecutive_windows b/internal/reportutils/testdata/TestGetReports/golden/multiple_non-consecutive_windows deleted file mode 100644 index f2cb19d..0000000 --- a/internal/reportutils/testdata/TestGetReports/golden/multiple_non-consecutive_windows +++ /dev/null @@ -1,3 +0,0 @@ -0: 7 -100: 107 -250: 257 diff --git a/internal/reportutils/testdata/TestGetReports/golden/multiple_non-consequtive_windows b/internal/reportutils/testdata/TestGetReports/golden/multiple_non-consequtive_windows deleted file mode 100644 index f2cb19d..0000000 --- a/internal/reportutils/testdata/TestGetReports/golden/multiple_non-consequtive_windows +++ /dev/null @@ -1,3 +0,0 @@ -0: 7 -100: 107 -250: 257 diff --git a/internal/uploader/internal_test.go b/internal/uploader/internal_test.go index f9882d3..27be531 100644 --- a/internal/uploader/internal_test.go +++ b/internal/uploader/internal_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" "github.com/ubuntu/ubuntu-insights/internal/constants" "github.com/ubuntu/ubuntu-insights/internal/fileutils" + "github.com/ubuntu/ubuntu-insights/internal/report" ) func TestUploadBadFile(t *testing.T) { @@ -25,21 +26,22 @@ func TestUploadBadFile(t *testing.T) { url string consent bool + rNewErr bool wantErr bool }{ "Ok": {fName: "0.json", fileContents: basicContent, wantErr: false}, "Missing File": {fName: "0.json", fileContents: basicContent, missingFile: true, wantErr: true}, "File Is Dir": {fName: "0.json", fileIsDir: true, wantErr: true}, - "Non-numeric": {fName: "not-numeric.json", fileContents: basicContent, wantErr: true}, - "Bad File Ext": {fName: "0.txt", fileContents: basicContent}, + "Non-numeric": {fName: "not-numeric.json", fileContents: basicContent, rNewErr: true}, + "Bad File Ext": {fName: "0.txt", fileContents: basicContent, rNewErr: true}, "Bad Contents": {fName: "0.json", fileContents: badContent, wantErr: true}, "Bad URL": {fName: "0.json", fileContents: basicContent, url: "http://bad host:1234", wantErr: true}, "Ok Consent": {fName: "0.json", fileContents: basicContent, consent: true, wantErr: false}, "Missing File Consent": {fName: "0.json", fileContents: basicContent, missingFile: true, consent: true, wantErr: true}, "File Is Dir Consent": {fName: "0.json", fileIsDir: true, consent: true, wantErr: true}, - "Non-numeric Consent": {fName: "not-numeric.json", fileContents: basicContent, consent: true, wantErr: true}, - "Bad File Ext Consent": {fName: "0.txt", fileContents: basicContent, consent: true}, + "Non-numeric Consent": {fName: "not-numeric.json", fileContents: basicContent, consent: true, rNewErr: true}, + "Bad File Ext Consent": {fName: "0.txt", fileContents: basicContent, consent: true, rNewErr: true}, "Bad Contents Consent": {fName: "0.json", fileContents: badContent, consent: true, wantErr: true}, "Bad URL Consent": {fName: "0.json", fileContents: basicContent, url: "http://bad host:1234", consent: true, wantErr: true}, } @@ -77,8 +79,13 @@ func TestUploadBadFile(t *testing.T) { if tc.url == "" { tc.url = ts.URL } - - err := um.upload(tc.fName, tc.url, tc.consent, false) + r, err := report.New(fPath) + if tc.rNewErr { + require.Error(t, err, "Setup: failed to create report object") + return + } + require.NoError(t, err, "Setup: failed to create report object") + err = um.upload(r, tc.url, tc.consent, false) if tc.wantErr { require.Error(t, err) return diff --git a/internal/uploader/upload.go b/internal/uploader/upload.go index 2f181e3..cb04d46 100644 --- a/internal/uploader/upload.go +++ b/internal/uploader/upload.go @@ -16,7 +16,7 @@ import ( "github.com/ubuntu/ubuntu-insights/internal/constants" "github.com/ubuntu/ubuntu-insights/internal/fileutils" - "github.com/ubuntu/ubuntu-insights/internal/reportutils" + "github.com/ubuntu/ubuntu-insights/internal/report" ) var ( @@ -34,7 +34,7 @@ func (um Uploader) Upload(force bool) error { return fmt.Errorf("upload failed to get consent state: %v", err) } - reports, err := reportutils.GetAllReports(um.collectedDir) + reports, err := report.GetAll(um.collectedDir) if err != nil { return fmt.Errorf("failed to get reports: %v", err) } @@ -45,17 +45,17 @@ func (um Uploader) Upload(force bool) error { } var wg sync.WaitGroup - for _, name := range reports { + for _, r := range reports { wg.Add(1) - go func(name string) { + go func(r report.Report) { defer wg.Done() - err := um.upload(name, url, consent, force) + err := um.upload(r, url, consent, force) if errors.Is(err, ErrReportNotMature) { - slog.Debug("Skipped report upload, not mature enough", "file", name, "source", um.source) + slog.Debug("Skipped report upload, not mature enough", "file", r.Name, "source", um.source) } else if err != nil { - slog.Warn("Failed to upload report", "file", name, "source", um.source, "error", err) + slog.Warn("Failed to upload report", "file", r.Name, "source", um.source, "error", err) } - }(name) + }(r) } wg.Wait() @@ -64,21 +64,15 @@ func (um Uploader) Upload(force bool) error { // upload uploads an individual report to the server. It returns an error if the report is not mature enough to be uploaded, or if the upload fails. // It also moves the report to the uploaded directory after a successful upload. -func (um Uploader) upload(name, url string, consent, force bool) error { - slog.Debug("Uploading report", "file", name, "consent", consent, "force", force) +func (um Uploader) upload(r report.Report, url string, consent, force bool) error { + slog.Debug("Uploading report", "file", r.Name, "consent", consent, "force", force) - // TODO… pass the Report object directly. - ts, err := reportutils.GetReportTime(name) - if err != nil { - return fmt.Errorf("failed to parse report time from filename: %v", err) - } - - if um.timeProvider.Now().Add(time.Duration(-um.minAge)*time.Second).Before(time.Unix(ts, 0)) && !force { + if um.timeProvider.Now().Add(time.Duration(-um.minAge)*time.Second).Before(time.Unix(r.TimeStamp, 0)) && !force { return ErrReportNotMature } // Check for duplicate reports. - _, err = os.Stat(filepath.Join(um.uploadedDir, name)) + _, err := os.Stat(filepath.Join(um.uploadedDir, r.Name)) if err != nil && !os.IsNotExist(err) { return fmt.Errorf("failed to check if report has already been uploaded: %v", err) } @@ -88,9 +82,9 @@ func (um Uploader) upload(name, url string, consent, force bool) error { return fmt.Errorf("report has already been uploaded") } - origData, err := um.readJSON(name) + origData, err := r.ReadJSON() if err != nil { - return fmt.Errorf("failed to get payload: %v", err) + return fmt.Errorf("failed to read report: %v", err) } data := origData if !consent { @@ -108,11 +102,11 @@ func (um Uploader) upload(name, url string, consent, force bool) error { // Move report first to avoid the situation where the report is sent, but not marked as sent. // TODO: maybe a method on Reports ? - if err := um.moveReport(filepath.Join(um.uploadedDir, name), filepath.Join(um.collectedDir, name), data); err != nil { + if err := um.moveReport(filepath.Join(um.uploadedDir, r.Name), filepath.Join(um.collectedDir, r.Name), data); err != nil { return fmt.Errorf("failed to move report after uploading: %v", err) } if err := send(url, data); err != nil { - if moveErr := um.moveReport(filepath.Join(um.collectedDir, name), filepath.Join(um.uploadedDir, name), origData); moveErr != nil { + if moveErr := um.moveReport(filepath.Join(um.collectedDir, r.Name), filepath.Join(um.uploadedDir, r.Name), origData); moveErr != nil { return fmt.Errorf("failed to send data: %v, and failed to restore the original report: %v", err, moveErr) } return fmt.Errorf("failed to send data: %v", err) @@ -130,21 +124,6 @@ func (um Uploader) getURL() (string, error) { return u.String(), nil } -// readJSON reads the JSON data from the report file. -func (um Uploader) readJSON(file string) ([]byte, error) { - // Read the report file - data, err := os.ReadFile(path.Join(um.collectedDir, file)) - if err != nil { - return nil, fmt.Errorf("failed to read report file: %v", err) - } - - if !json.Valid(data) { - return nil, fmt.Errorf("invalid JSON data in report file") - } - - return data, nil -} - func (um Uploader) moveReport(writePath, removePath string, data []byte) error { // (Report).MarkAsProcessed(data) // (Report).UndoProcessed