Skip to content
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

Jump to bottom

Development: Fix unclosed input streams #7325

Merged
merged 1 commit into from
Oct 7, 2023
Merged

Development: Fix unclosed input streams #7325

merged 1 commit into from
Oct 7, 2023

Conversation

Strohgelaender
Copy link
Contributor

@Strohgelaender Strohgelaender commented Oct 5, 2023 •
edited
Loading

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I followed the coding and design guidelines.

Motivation and Context

While reviewing #7322 I noticed that the FileUtils class has two methods copyToFile() and copyInputStreamToFile(). They differ in the fact that only the sencond method closes the given input stream, and copyToFile() leaves the straem open.

Description

This PR replaces copyToFile() with copyInputStreamToFile() where applicabel, and replaces explicit close() calls with try-with-ressources blocks.

Steps for Testing

code review

Review Progress

Performance Review

  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

@github-actions github-actions bot added the server Pull requests that update Java code. (Added Automatically!) label Oct 5, 2023
@Strohgelaender Strohgelaender marked this pull request as ready for review October 5, 2023 07:39
@Strohgelaender Strohgelaender requested a review from a team as a code owner October 5, 2023 07:39
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 5, 2023
View reviewed changes
src/main/java/de/tum/in/www1/artemis/service/FileService.java
@@ -181,7 +181,7 @@
filePath = generateFilePath(filenamePrefix, fileExtension, path);
}
try {
FileUtils.copyToFile(file.getInputStream(), filePath.toFile());
FileUtils.copyInputStreamToFile(file.getInputStream(), filePath.toFile());

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression

This path depends on a [user-provided value](1). This path depends on a [user-provided value](2). This path depends on a [user-provided value](3). This path depends on a [user-provided value](4). This path depends on a [user-provided value](5). This path depends on a [user-provided value](6). This path depends on a [user-provided value](7). This path depends on a [user-provided value](8). This path depends on a [user-provided value](9). This path depends on a [user-provided value](10).
src/main/java/de/tum/in/www1/artemis/service/FileUploadSubmissionService.java
@@ -201,7 +201,7 @@
final Path filePath = dirPath.resolve(filename);
final File savedFile = filePath.toFile();

FileUtils.copyToFile(file.getInputStream(), savedFile);
FileUtils.copyInputStreamToFile(file.getInputStream(), savedFile);

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression

This path depends on a [user-provided value](1). This path depends on a [user-provided value](2).
@Strohgelaender Strohgelaender changed the title Development fix unclosed input streams Development: Fix unclosed input streams Oct 5, 2023
laadvo
laadvo approved these changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@laadvo laadvo left a comment

Choose a reason for hiding this comment

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

LGTM

DominikRemo
DominikRemo approved these changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@DominikRemo DominikRemo left a comment

Choose a reason for hiding this comment

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

Code LGTM

milljoniaer
milljoniaer approved these changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@milljoniaer milljoniaer left a comment

Choose a reason for hiding this comment

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

Code LGTM 👍

@Strohgelaender Strohgelaender added this to the 6.5.5 milestone Oct 5, 2023
laurenzfb
laurenzfb approved these changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@laurenzfb laurenzfb left a comment

Choose a reason for hiding this comment

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

LGTM

@krusche krusche merged commit 285968d into develop Oct 7, 2023
@krusche krusche deleted the chore/close-inputstream branch October 7, 2023 15:47
@krusche krusche modified the milestones: 6.5.5, 6.6.0 Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milljoniaer milljoniaer milljoniaer approved these changes

@laadvo laadvo laadvo approved these changes

@DominikRemo DominikRemo DominikRemo approved these changes

@laurenzfb laurenzfb laurenzfb approved these changes

@julian-christl julian-christl Awaiting requested review from julian-christl

Assignees

@Strohgelaender Strohgelaender

Labels
ready to merge server Pull requests that update Java code. (Added Automatically!) small
Projects
None yet
Milestone
6.6.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Strohgelaender @laurenzfb @DominikRemo @laadvo @milljoniaer @krusche