From fc60627954b8dc19e94c7c6e9deaa42989294710 Mon Sep 17 00:00:00 2001 From: Donal Hurley Date: Tue, 18 Nov 2025 10:34:29 +0000 Subject: [PATCH 1/4] Ensure only files are in file overview in config apply request --- internal/file/file_manager_service.go | 45 +++++++++++++++++----- internal/file/file_manager_service_test.go | 8 ++-- 2 files changed, 39 insertions(+), 14 deletions(-) diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index 6c7ed4afb..cd7ac491b 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -176,6 +176,8 @@ func (fms *FileManagerService) ConfigApply(ctx context.Context, fms.fileActions = diffFiles + slog.DebugContext(ctx, "Executing config apply file actions", "actions", diffFiles) + rollbackTempFilesErr := fms.backupFiles(ctx) if rollbackTempFilesErr != nil { return model.Error, rollbackTempFilesErr @@ -373,26 +375,49 @@ func (fms *FileManagerService) DetermineFileActions( for _, modifiedFile := range modifiedFiles { fileName := modifiedFile.File.GetFileMeta().GetName() currentFile, ok := filesMap[fileName] - // default to unchanged action modifiedFile.Action = model.Unchanged - // if file is unmanaged, action is set to unchanged so file is skipped when performing actions + // If file is unmanaged, action is set to unchanged so file is skipped when performing actions. if modifiedFile.File.GetUnmanaged() { slog.DebugContext(ctx, "Skipping unmanaged file updates", "file_name", fileName) continue } - // if file doesn't exist in the current files, file has been added - // set file action - if _, statErr := os.Stat(fileName); errors.Is(statErr, os.ErrNotExist) { - modifiedFile.Action = model.Add + + // If file currently exists on disk, is being tracked in manifest and file hash is different. + // Treat it as a file update. + if ok && modifiedFile.File.GetFileMeta().GetHash() != currentFile.GetFileMeta().GetHash() { + modifiedFile.Action = model.Update fileDiff[fileName] = modifiedFile continue - // if file currently exists and file hash is different, file has been updated - // copy contents, set file action - } else if ok && modifiedFile.File.GetFileMeta().GetHash() != currentFile.GetFileMeta().GetHash() { - modifiedFile.Action = model.Update + } + + // If file doesn't exist on disk. + // Treat it as adding a new file. + if fileStats, statErr := os.Stat(fileName); errors.Is(statErr, os.ErrNotExist) { + modifiedFile.Action = model.Add fileDiff[fileName] = modifiedFile + + continue + // If file already exists on disk but is not being tracked in manifest and the file hash is different. + // Treat it as a file update. + } else if statErr == nil { + if fileStats.IsDir() { + return nil, fmt.Errorf( + "unable to create file %s since a directory with the same name already exists on the data plane", + fileName, + ) + } + + metadataOfFileOnDisk, err := files.FileMeta(fileName) + if err != nil { + return nil, fmt.Errorf("unable to get file metadata for %s: %w", fileName, err) + } + + if metadataOfFileOnDisk.GetHash() != modifiedFile.File.GetFileMeta().GetHash() { + modifiedFile.Action = model.Update + fileDiff[fileName] = modifiedFile + } } } diff --git a/internal/file/file_manager_service_test.go b/internal/file/file_manager_service_test.go index e42128063..fa40ee991 100644 --- a/internal/file/file_manager_service_test.go +++ b/internal/file/file_manager_service_test.go @@ -763,12 +763,12 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) { modifiedFiles: map[string]*model.FileCache{ addTestFile.Name(): { File: &mpi.File{ - FileMeta: protos.FileMeta(addTestFile.Name(), files.GenerateHash(fileContent)), + FileMeta: protos.FileMeta(addTestFile.Name(), files.GenerateHash(addFileContent)), }, }, updateTestFile.Name(): { File: &mpi.File{ - FileMeta: protos.FileMeta(updateTestFile.Name(), files.GenerateHash(fileContent)), + FileMeta: protos.FileMeta(updateTestFile.Name(), files.GenerateHash(updatedFileContent)), }, }, deleteTestFile.Name(): { @@ -782,10 +782,10 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) { FileMeta: protos.FileMeta(deleteTestFile.Name(), files.GenerateHash(fileContent)), }, updateTestFile.Name(): { - FileMeta: protos.FileMeta(updateTestFile.Name(), files.GenerateHash(fileContent)), + FileMeta: protos.FileMeta(updateTestFile.Name(), files.GenerateHash(updatedFileContent)), }, addTestFile.Name(): { - FileMeta: protos.FileMeta(addTestFile.Name(), files.GenerateHash(fileContent)), + FileMeta: protos.FileMeta(addTestFile.Name(), files.GenerateHash(addFileContent)), }, }, expectedCache: make(map[string]*model.FileCache), From 76d9dbd722c3c52882e1c81de3191366e71abfd6 Mon Sep 17 00:00:00 2001 From: Donal Hurley Date: Tue, 18 Nov 2025 16:19:58 +0000 Subject: [PATCH 2/4] Add more unit tests and logging --- internal/file/file_manager_service.go | 3 +++ internal/file/file_manager_service_test.go | 26 +++++++++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index cd7ac491b..930165dfc 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -386,6 +386,7 @@ func (fms *FileManagerService) DetermineFileActions( // If file currently exists on disk, is being tracked in manifest and file hash is different. // Treat it as a file update. if ok && modifiedFile.File.GetFileMeta().GetHash() != currentFile.GetFileMeta().GetHash() { + slog.DebugContext(ctx, "Tracked file requires updating", "file_name", fileName) modifiedFile.Action = model.Update fileDiff[fileName] = modifiedFile @@ -395,6 +396,7 @@ func (fms *FileManagerService) DetermineFileActions( // If file doesn't exist on disk. // Treat it as adding a new file. if fileStats, statErr := os.Stat(fileName); errors.Is(statErr, os.ErrNotExist) { + slog.DebugContext(ctx, "New untracked file to created", "file_name", fileName) modifiedFile.Action = model.Add fileDiff[fileName] = modifiedFile @@ -415,6 +417,7 @@ func (fms *FileManagerService) DetermineFileActions( } if metadataOfFileOnDisk.GetHash() != modifiedFile.File.GetFileMeta().GetHash() { + slog.DebugContext(ctx, "Untracked file requires updating", "file_name", fileName) modifiedFile.Action = model.Update fileDiff[fileName] = modifiedFile } diff --git a/internal/file/file_manager_service_test.go b/internal/file/file_manager_service_test.go index fa40ee991..6fe9d55ce 100644 --- a/internal/file/file_manager_service_test.go +++ b/internal/file/file_manager_service_test.go @@ -805,6 +805,24 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) { expectedContent: make(map[string][]byte), expectedError: nil, }, + { + name: "Test 4: File is actually a directory", + allowedDirs: []string{tempDir}, + modifiedFiles: map[string]*model.FileCache{ + tempDir: { + File: &mpi.File{ + FileMeta: protos.FileMeta(tempDir, files.GenerateHash(fileContent)), + }, + }, + }, + currentFiles: make(map[string]*mpi.File), + expectedCache: map[string]*model.FileCache(nil), + expectedContent: make(map[string][]byte), + expectedError: fmt.Errorf( + "unable to create file %s since a directory with the same name already exists on the data plane", + tempDir, + ), + }, } for _, test := range tests { @@ -828,7 +846,13 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) { test.currentFiles, test.modifiedFiles, ) - require.NoError(tt, fileActionErr) + + if test.expectedError != nil { + require.EqualError(tt, fileActionErr, test.expectedError.Error()) + } else { + require.NoError(tt, fileActionErr) + } + assert.Equal(tt, test.expectedCache, diff) }) } From bec50ce6595d0b819d4d3ed0008d3a7b931e9822 Mon Sep 17 00:00:00 2001 From: Donal Hurley Date: Wed, 19 Nov 2025 13:49:20 +0000 Subject: [PATCH 3/4] Clean up --- internal/file/file_manager_service.go | 46 +++++++++++++--------- internal/file/file_manager_service_test.go | 2 +- 2 files changed, 28 insertions(+), 20 deletions(-) diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index 930165dfc..98446b57e 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -393,34 +393,42 @@ func (fms *FileManagerService) DetermineFileActions( continue } + fileStats, statErr := os.Stat(fileName) + // If file doesn't exist on disk. // Treat it as adding a new file. - if fileStats, statErr := os.Stat(fileName); errors.Is(statErr, os.ErrNotExist) { + if errors.Is(statErr, os.ErrNotExist) { slog.DebugContext(ctx, "New untracked file to created", "file_name", fileName) modifiedFile.Action = model.Add fileDiff[fileName] = modifiedFile continue - // If file already exists on disk but is not being tracked in manifest and the file hash is different. - // Treat it as a file update. - } else if statErr == nil { - if fileStats.IsDir() { - return nil, fmt.Errorf( - "unable to create file %s since a directory with the same name already exists on the data plane", - fileName, - ) - } + } - metadataOfFileOnDisk, err := files.FileMeta(fileName) - if err != nil { - return nil, fmt.Errorf("unable to get file metadata for %s: %w", fileName, err) - } + // If there is an error other than not existing, return that error. + if statErr != nil { + return nil, fmt.Errorf("unable to stat file %s: %w", fileName, statErr) + } - if metadataOfFileOnDisk.GetHash() != modifiedFile.File.GetFileMeta().GetHash() { - slog.DebugContext(ctx, "Untracked file requires updating", "file_name", fileName) - modifiedFile.Action = model.Update - fileDiff[fileName] = modifiedFile - } + // If there is a directory with the same name, return an error. + if fileStats.IsDir() { + return nil, fmt.Errorf( + "unable to create file %s since a directory with the same name already exists", + fileName, + ) + } + + // If file already exists on disk but is not being tracked in manifest and the file hash is different. + // Treat it as a file update. + metadataOfFileOnDisk, err := files.FileMeta(fileName) + if err != nil { + return nil, fmt.Errorf("unable to get file metadata for %s: %w", fileName, err) + } + + if metadataOfFileOnDisk.GetHash() != modifiedFile.File.GetFileMeta().GetHash() { + slog.DebugContext(ctx, "Untracked file requires updating", "file_name", fileName) + modifiedFile.Action = model.Update + fileDiff[fileName] = modifiedFile } } diff --git a/internal/file/file_manager_service_test.go b/internal/file/file_manager_service_test.go index 6fe9d55ce..538295093 100644 --- a/internal/file/file_manager_service_test.go +++ b/internal/file/file_manager_service_test.go @@ -819,7 +819,7 @@ func TestFileManagerService_DetermineFileActions(t *testing.T) { expectedCache: map[string]*model.FileCache(nil), expectedContent: make(map[string][]byte), expectedError: fmt.Errorf( - "unable to create file %s since a directory with the same name already exists on the data plane", + "unable to create file %s since a directory with the same name already exists", tempDir, ), }, From bea3f91b6ab4743b2eaa05732f6c054589e8436d Mon Sep 17 00:00:00 2001 From: Donal Hurley Date: Wed, 19 Nov 2025 14:11:10 +0000 Subject: [PATCH 4/4] Update log message --- internal/file/file_manager_service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/file/file_manager_service.go b/internal/file/file_manager_service.go index 98446b57e..ee591f29f 100644 --- a/internal/file/file_manager_service.go +++ b/internal/file/file_manager_service.go @@ -398,7 +398,7 @@ func (fms *FileManagerService) DetermineFileActions( // If file doesn't exist on disk. // Treat it as adding a new file. if errors.Is(statErr, os.ErrNotExist) { - slog.DebugContext(ctx, "New untracked file to created", "file_name", fileName) + slog.DebugContext(ctx, "New untracked file needs to be created", "file_name", fileName) modifiedFile.Action = model.Add fileDiff[fileName] = modifiedFile