Skip to content

Modernize and secure temp file creation #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Modernize and secure temp file creation #7

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion promise/src/main/java/com/iluwatar/promise/Utility.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.io.IOException;
import java.io.InputStreamReader;
import java.net.URL;
import java.nio.file.Files;
import java.util.Collections;
import java.util.Comparator;
import java.util.Map;
Expand Down Expand Up @@ -99,7 +100,7 @@ public static Integer countLines(String fileLocation) {
public static String downloadFile(String urlString) throws IOException {
LOGGER.info("Downloading contents from url: {}", urlString);
var url = new URL(urlString);
var file = File.createTempFile("promise_pattern", null);
var file = Files.createTempFile("promise_pattern", null).toFile();
try (var bufferedReader = new BufferedReader(new InputStreamReader(url.openStream()));
var writer = new FileWriter(file)) {
String line;
Comment on lines 100 to 106
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method downloadFile creates a temporary file but does not ensure that this file is deleted after use. This can lead to resource leakage and potentially fill up the temporary directory over time.

Recommended Solution:
Ensure that the temporary file is deleted after it is no longer needed, either by using file.deleteOnExit() or by explicitly deleting the file in a finally block.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review:

  1. Import Statement:

    • The addition of import java.nio.file.Files; seems appropriate if the code is using methods related to file operations from this package.

  2. File Handling:

    • It would be better to handle potential exceptions related to file creation for Files.createTempFile. Since it can throw an IOException, consider adding a throws IOException clause or handling it within the method.
    • Ensure proper cleanup of resources, potentially close any open streams/resources with try-with-resources or separate try-catch blocks where needed.

  3. Instantiation:

    • Change Files.createTempFile(...).toFile(); to just Files.createTempFile(...);.

  4. Variable Naming:

    • Variable names seem descriptive. However, consider maintaining consistency (url vs. fileLocation).

  5. Logging:

    • Consider logging error messages and handling exceptions appropriately (if not done in other parts of the code).

  6. Code Readability:

    • Add comments or documentation where necessary for better code readability and maintainability.
    • Ensure consistent formatting and code style adherence.

  7. Testing:

    • Implement tests to cover success and failure scenarios, especially around file handling and network operations.

  8. Error Handling:

    • Verify how exceptions like IOException are handled throughout the application to ensure reliability under various conditions.

  9. Resource Management:

    • Perform proper resource management, especially when dealing with I/O operations (e.g., input streams), to avoid resource leaks.

  10. Consolidation:

    • Group related imports together for easier code readability.

Suggested Improvement:

// Better handling of exception for Files.createTempFile
public static String downloadFile(String urlString) throws IOException {
    LOGGER.info("Downloading contents from url: {}", urlString);
    URL url = new URL(urlString);

    // Create a temporary file
    Path tempFilePath;
    try {
        tempFilePath = Files.createTempFile("promise_pattern", null);
    } catch (IOException e) {
        LOGGER.error("Error creating temporary file", e);
        throw e; // Handle or rethrow as necessary
    }

    File file = tempFilePath.toFile();

    // Properly close resources with try-with-resources
    try (BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(url.openStream()));
         FileWriter writer = new FileWriter(file)) {

        // Read lines from URL and write to file
        String line;
        while ((line = bufferedReader.readLine()) != null) {
            writer.write(line);
        }
    }

    return file.getAbsolutePath();
}

Overall, the reviewed code seems functional but requires some improvements and error handling enhancements to strengthen its robustness.

Expand Down