Update ZipHandler and Methods

[JeremyIR]

Description

Updates ZipHandler with blob filepath support, adds MoveZip method for moving a file from 'unzip' into the specified subfolder, adds associated tests, and updates Docker infrastructure to properly navigate filesystem when moving .zips.

Note: this PR contains a temporary fix for azure-function-core-tools, which tags it to a specific version. There is a card in place to remediate this.

Issue

1254

Checklist

• I have added tests to cover my changes
• I have added logging where useful (with appropriate log level)

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Redundant Code The 'MoveFile' method is repeatedly mocked and asserted in multiple test cases without any variation in the setup or expected outcome, which could be refactored for better code reuse and maintainability. Error Handling The error returned from the 'MoveFile' method is not checked or asserted in the test cases, which might lead to overlooking potential issues during error conditions.

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible bug	Check for file write errors immediately to prevent further operations on a failed write Ensure that the error from os.WriteFile is checked immediately after the call to prevent proceeding when the file write fails. src/sftp/handler.go [245-249] +zipFileName := fileInfo.Name() +err = os.WriteFile(zipFileName, fileBytes, 0644) +if err != nil { + slog.Error("Failed to write file", slog.Any(utils.ErrorKey, err), slog.String("name", fileInfo.Name())) + return +} - Suggestion importance[1-10]: 10 Why: This suggestion correctly identifies a crucial error handling step that ensures the program does not proceed with further operations if the file write fails. It is essential for maintaining the integrity and reliability of the file handling process.	10
	Ensure correct parameters are used in file movement operations Add assertions to check the correct parameters are passed to MoveFile to ensure that files are moved to the intended locations. src/zip/zip_test.go [52] -mockBlobHandler.AssertCalled(t, "MoveFile", mock.Anything, unzipSuccessUrl) +mockBlobHandler.AssertCalled(t, "MoveFile", filename, unzipSuccessUrl) Suggestion importance[1-10]: 5 Why: Adding assertions to check the correct parameters are passed to MoveFile is a good practice to avoid bugs related to incorrect file handling, though the impact is moderate.	5
Best practice	Ensure that errors from deferred close operations are handled properly Handle potential errors from defer zipReader.Close() by logging or handling the error explicitly. src/zip/zip.go [69-76] zipReader, err := zipHandler.zipClient.OpenReader(zipFileName) if err != nil { slog.Error("Failed to open zip reader", slog.Any(utils.ErrorKey, err)) zipHandler.MoveZip(blobPath, utils.FailureFolder) return err } -defer zipReader.Close() +defer func() { + if cerr := zipReader.Close(); cerr != nil { + slog.Error("Failed to close zip reader", slog.Any(utils.ErrorKey, cerr)) + } +}() Suggestion importance[1-10]: 8 Why: Handling errors from deferred close operations is a best practice that prevents resource leaks and potential data corruption. This suggestion correctly adds error handling for the zipReader.Close() method, which is crucial for robustness.	8
	Validate parameters in MoveZip to prevent file path errors Validate the blobPath and subfolder parameters in MoveZip to ensure they are not empty or malformed, which could lead to incorrect file paths. src/zip/zip.go [106-117] func (zipHandler ZipHandler) MoveZip(blobPath string, subfolder string) { + if blobPath == "" || subfolder == "" { + slog.Error("Invalid parameters for moving file", slog.String("blobPath", blobPath), slog.String("subfolder", subfolder)) + return + } slog.Info("About to move file", slog.String("blobPath", blobPath), slog.String("destination subfolder", subfolder)) sourceUrl := filepath.Join(utils.ContainerName, blobPath) destinationUrl := strings.Replace(sourceUrl, utils.UnzipFolder, filepath.Join(utils.UnzipFolder, subfolder), 1) err := zipHandler.blobHandler.MoveFile(sourceUrl, destinationUrl) ... } Suggestion importance[1-10]: 7 Why: Validating parameters before performing operations like file moves is a best practice that prevents runtime errors due to invalid arguments. This suggestion correctly adds checks for empty or malformed parameters, enhancing the function's reliability.	7
	Improve error handling for file movement operations Ensure that the MoveFile method handles errors properly by checking the return value and logging or handling the error if it occurs. src/zip/zip_test.go [97] -mockBlobHandler.On("MoveFile", mock.Anything, mock.Anything).Return(nil) +err := mockBlobHandler.MoveFile(mock.Anything, mock.Anything) +if err != nil { + log.Errorf("Failed to move file: %v", err) +} Suggestion importance[1-10]: 6 Why: The suggestion to add error handling for the MoveFile method is valid and improves the robustness of the code by ensuring errors are not silently ignored.	6
	Replace repeated string literals with constants Use constants for repeated string literals like file paths to enhance maintainability and reduce the risk of typos. src/zip/zip_test.go [16-17] -var filename = "cheeseburger.zip" -var unzipSuccessUrl = "sftp/unzip/success/cheeseburger.zip" +const Filename = "cheeseburger.zip" +const UnzipSuccessUrl = "sftp/unzip/success/cheeseburger.zip" Suggestion importance[1-10]: 4 Why: Using constants for repeated string literals like file paths can enhance maintainability and reduce typos, but the impact on the overall functionality is minor.	4
Maintainability	Refactor setup code into a helper function to reduce duplication Refactor the repeated code for setting up mocks and creating ZipHandler into a helper function to improve code maintainability and reduce duplication. src/zip/zip_test.go [96-101] -mockBlobHandler := new(mocks.MockBlobHandler) -mockBlobHandler.On("MoveFile", mock.Anything, mock.Anything).Return(nil) -zipHandler := ZipHandler{ - credentialGetter: mockCredentialGetter, - blobHandler: mockBlobHandler, -} +zipHandler := setupZipHandlerWithMocks() Suggestion importance[1-10]: 7 Why: This suggestion to refactor repeated setup code into a helper function is beneficial for maintainability and reduces code duplication across multiple test cases.	7
Enhancement	Implement robust error handling for file move operations Consider implementing a retry mechanism or alternative handling when zipHandler.blobHandler.MoveFile fails, to ensure robust file handling. src/zip/zip.go [111-116] err := zipHandler.blobHandler.MoveFile(sourceUrl, destinationUrl) if err != nil { slog.Error("Unable to move file to "+destinationUrl, slog.Any(utils.ErrorKey, err)) + // Consider retrying or alternative error handling here } else { slog.Info("Successfully moved file to "+destinationUrl) } Suggestion importance[1-10]: 6 Why: Adding a retry mechanism or alternative error handling for file move operations can significantly improve the robustness of file handling, especially in error-prone operations. This suggestion is a valuable enhancement, although it could be more specific about the retry mechanism details.	6

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud