removed duplicate rename function

Author: Akshay2191

api/grpc/mpi/v1/command_grpc.pb.go

@@ -85,8 +85,8 @@ type (
 			fileToUpdate *mpi.File,
 		) error
 		SetIsConnected(isConnected bool)
-		RenameFile(ctx context.Context, hash, fileName, tempDir string) error
-		RenameExternalFile(ctx context.Context, fileName, tempDir string) error
+		RenameFile(ctx context.Context, fileName, tempDir string) error
+		ValidateFileHash(ctx context.Context, fileName, expectedHash string) error
 		UpdateClient(ctx context.Context, fileServiceClient mpi.FileServiceClient)
 	}
 
@@ -646,6 +646,7 @@ func (fms *FileManagerService) downloadUpdatedFilesToTempLocation(ctx context.Co
 	return errGroup.Wait()
 }
 
+//nolint:revive // cognitive-complexity of 14 max is 12, loop is needed cant be broken up
 func (fms *FileManagerService) moveOrDeleteFiles(ctx context.Context, actionError error) error {
 actionsLoop:
 	for _, fileAction := range fms.fileActions {
@@ -664,9 +665,14 @@ actionsLoop:
 
 			continue
 		case model.Add, model.Update:
-			err = fms.fileServiceOperator.RenameFile(ctx, fileMeta.GetHash(), tempFilePath, fileMeta.GetName())
+			err = fms.fileServiceOperator.RenameFile(ctx, tempFilePath, fileMeta.GetName())
+			if err != nil {
+				actionError = err
+				break actionsLoop
+			}
+			err = fms.fileServiceOperator.ValidateFileHash(ctx, fileMeta.GetName(), fileMeta.GetHash())
 		case model.ExternalFile:
-			err = fms.fileServiceOperator.RenameExternalFile(ctx, tempFilePath, fileMeta.GetName())
+			err = fms.fileServiceOperator.RenameFile(ctx, tempFilePath, fileMeta.GetName())
 		case model.Unchanged:
 			slog.DebugContext(ctx, "File unchanged")
 		}

api/grpc/mpi/v1/files_grpc.pb.go

@@ -21,7 +21,6 @@ import (
 	"time"
 
 	"github.com/nginx/agent/v3/internal/config"
-	"github.com/nginx/agent/v3/internal/file/filefakes"
 	"github.com/nginx/agent/v3/internal/model"
 
 	"github.com/nginx/agent/v3/pkg/files"
@@ -1402,42 +1401,6 @@ func TestFileManagerService_downloadExternalFiles(t *testing.T) {
 	}
 }
 
-func TestFileManagerService_ExternalFileRenameCalled(t *testing.T) {
-	ctx := context.Background()
-	fms := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{})
-
-	fakeFSO := &filefakes.FakeFileServiceOperatorInterface{}
-
-	fakeFSO.RenameExternalFileStub = func(ctx context.Context, fileName, tempDir string) error {
-		return nil
-	}
-
-	fms.fileServiceOperator = fakeFSO
-
-	fileName := filepath.Join(t.TempDir(), "ext.conf")
-	fms.fileActions = map[string]*model.FileCache{
-		fileName: {
-			File:   &mpi.File{FileMeta: &mpi.FileMeta{Name: fileName}},
-			Action: model.ExternalFile,
-		},
-	}
-
-	tempPath := tempFilePath(fileName)
-	reqDir := filepath.Dir(tempPath)
-	require.NoError(t, os.MkdirAll(reqDir, 0o755))
-	require.NoError(t, os.WriteFile(tempPath, []byte("data"), 0o600))
-
-	err := fms.moveOrDeleteFiles(ctx, nil)
-	require.NoError(t, err)
-
-	assert.Equal(t, 1, fakeFSO.RenameExternalFileCallCount(), "RenameExternalFile should be called once")
-
-	_, srcArg, dstArg := fakeFSO.RenameExternalFileArgsForCall(0)
-
-	assert.Equal(t, tempPath, srcArg, "RenameExternalFile source argument mismatch")
-	assert.Equal(t, fileName, dstArg, "RenameExternalFile destination argument mismatch")
-}
-
 func TestFileManagerService_DownloadFileContent_MaxBytesLimit(t *testing.T) {
 	ctx := context.Background()
 	fms := NewFileManagerService(nil, types.AgentConfig(), &sync.RWMutex{})

internal/file/file_manager_service.go

@@ -110,7 +110,7 @@ func (fso *FileServiceOperator) File(
 		return writeErr
 	}
 
-	return fso.validateFileHash(tempFilePath, expectedHash)
+	return fso.ValidateFileHash(ctx, tempFilePath, expectedHash)
 }
 
 func (fso *FileServiceOperator) UpdateOverview(
@@ -260,7 +260,7 @@ func (fso *FileServiceOperator) ChunkedFile(
 		return writeChunkedFileError
 	}
 
-	return fso.validateFileHash(tempFilePath, expectedHash)
+	return fso.ValidateFileHash(ctx, tempFilePath, expectedHash)
 }
 
 func (fso *FileServiceOperator) UpdateFile(
@@ -282,26 +282,9 @@ func (fso *FileServiceOperator) UpdateFile(
 	return fso.sendUpdateFileStream(ctx, fileToUpdate, fso.agentConfig.Client.Grpc.FileChunkSize)
 }
 
-func (fso *FileServiceOperator) RenameExternalFile(
-	ctx context.Context, source, desination string,
-) error {
-	slog.DebugContext(ctx, fmt.Sprintf("Moving file %s to %s (no hash validation)", source, desination))
-
-	if err := os.MkdirAll(filepath.Dir(desination), dirPerm); err != nil {
-		return fmt.Errorf("failed to create directories for %s: %w", desination, err)
-	}
-
-	moveErr := os.Rename(source, desination)
-	if moveErr != nil {
-		return fmt.Errorf("failed to move file: %w", moveErr)
-	}
-
-	return nil
-}
-
 // renameFile, renames (moves) file from tempDir to new location to update file.
 func (fso *FileServiceOperator) RenameFile(
-	ctx context.Context, hash, source, desination string,
+	ctx context.Context, source, desination string,
 ) error {
 	slog.DebugContext(ctx, fmt.Sprintf("Renaming file %s to %s", source, desination))
 
@@ -315,10 +298,11 @@ func (fso *FileServiceOperator) RenameFile(
 		return fmt.Errorf("failed to rename file: %w", moveErr)
 	}
 
-	return fso.validateFileHash(desination, hash)
+	return nil
 }
 
-func (fso *FileServiceOperator) validateFileHash(filePath, expectedHash string) error {
+func (fso *FileServiceOperator) ValidateFileHash(ctx context.Context, filePath, expectedHash string) error {
+	slog.DebugContext(ctx, "Validating file hash for file ", "file_path", filePath)
 	content, err := os.ReadFile(filePath)
 	if err != nil {
 		return err

internal/file/file_manager_service_test.go

@@ -187,130 +187,3 @@ func TestFileManagerService_UpdateFile_LargeFile(t *testing.T) {
 
 	helpers.RemoveFileWithErrorCheck(t, testFile.Name())
 }
-
-//nolint:revive // complexity is 21.
-func TestFileServiceOperator_RenameExternalFile(t *testing.T) {
-	type testCase struct {
-		name               string
-		wantErrMsg         string
-		setupFailurePath   string
-		destinationPath    string
-		expectedDstContent []byte
-		srcContent         []byte
-		wantErr            bool
-	}
-
-	tests := []testCase{
-		{
-			name:               "Test 1: success",
-			srcContent:         []byte("hello world"),
-			setupFailurePath:   "",
-			destinationPath:    "subdir/dest.txt",
-			wantErr:            false,
-			expectedDstContent: []byte("hello world"),
-		},
-		{
-			name:               "Test 2: mkdirall_fail",
-			srcContent:         []byte("content"),
-			setupFailurePath:   "not_a_dir",
-			destinationPath:    "not_a_dir/dest.txt",
-			wantErr:            true,
-			wantErrMsg:         "failed to create directories for",
-			expectedDstContent: nil,
-		},
-		{
-			name:               "Test 3: rename_fail (src does not exist)",
-			srcContent:         nil,
-			setupFailurePath:   "",
-			destinationPath:    "subdir/dest.txt",
-			wantErr:            true,
-			wantErrMsg:         "failed to move file",
-			expectedDstContent: nil,
-		},
-		{
-			name:               "Test 4: No destination specified (empty dst path)",
-			srcContent:         []byte("source content"),
-			setupFailurePath:   "",
-			destinationPath:    "",
-			wantErr:            true,
-			wantErrMsg:         "failed to move file:",
-			expectedDstContent: nil,
-		},
-		{
-			name:               "Test 5: Restricted directory (simulated permission fail)",
-			srcContent:         []byte("source content"),
-			setupFailurePath:   "",
-			destinationPath:    "restricted_dir/dest.txt",
-			wantErr:            true,
-			wantErrMsg:         "permission denied",
-			expectedDstContent: nil,
-		},
-		{
-			name:               "Test 6: Two files to the same destination",
-			srcContent:         []byte("source content 1"),
-			setupFailurePath:   "",
-			destinationPath:    "collision_dir/file.txt",
-			wantErr:            false,
-			expectedDstContent: []byte("source content 2"),
-		},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			t.Helper()
-			ctx := context.Background()
-			fso := NewFileServiceOperator(types.AgentConfig(), nil, &sync.RWMutex{})
-			tmp := t.TempDir()
-
-			src := filepath.Join(tmp, "src.txt")
-			dst := filepath.Join(tmp, tc.destinationPath)
-
-			if tc.setupFailurePath != "" {
-				parentFile := filepath.Join(tmp, tc.setupFailurePath)
-				require.NoError(t, os.WriteFile(parentFile, []byte("block"), 0o600))
-			}
-
-			if tc.srcContent != nil {
-				require.NoError(t, os.WriteFile(src, tc.srcContent, 0o600))
-			}
-
-			if tc.name == "Test 6: Two files to the same destination" {
-				src1 := src
-				dstCollision := dst
-				require.NoError(t, fso.RenameExternalFile(ctx, src1, dstCollision), "initial rename must succeed")
-
-				src2 := filepath.Join(tmp, "src2.txt")
-				content2 := []byte("source content 2")
-				require.NoError(t, os.WriteFile(src2, content2, 0o600))
-
-				src = src2
-				dst = dstCollision
-			}
-
-			if tc.name == "Test 5: Restricted directory (simulated permission fail)" {
-				parentDir := filepath.Dir(dst)
-				require.NoError(t, os.MkdirAll(parentDir, 0o500))
-			}
-
-			err := fso.RenameExternalFile(ctx, src, dst)
-
-			if tc.wantErr {
-				require.Error(t, err)
-				if tc.wantErrMsg != "" {
-					require.Contains(t, err.Error(), tc.wantErrMsg)
-				}
-
-				return
-			}
-
-			require.NoError(t, err)
-
-			dstContent, readErr := os.ReadFile(dst)
-			require.NoError(t, readErr)
-			require.Equal(t, tc.expectedDstContent, dstContent)
-
-			_, statErr := os.Stat(src)
-			require.True(t, os.IsNotExist(statErr), "Source file should not exist after successful rename")
-		})
-	}
-}