Skip to content

Development: Delete old files properly when updating file submissions #6849

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
Merged
merged 26 commits into from
Jul 26, 2023
Merged

Development: Delete old files properly when updating file submissions #6849

merged 26 commits into from
Jul 26, 2023

Conversation

@MarkusPaulsen
Copy link
Contributor

@MarkusPaulsen MarkusPaulsen commented Jul 7, 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.
  • I documented the Java code using JavaDoc style.

Motivation and Context

The parameter fileUploadSubmission is currently getting used for getting the last submission in FileUploadSubmissionService. The data of this parameter is under the control of the user, which may pose certain risks.

Description

Instead of FileUploadSubmission fileUploadSubmission, we now use StudentParticipation participation for getting the last submission in FileUploadSubmissionService, as the data of this parameter is not under the control of the user.

Steps for Testing

We just want to make sure that I didn't accidentally break the implementation.

Prerequisites:

  • 1 Instructor
  • 1 Student

Fileupload Exercise (Instructor, Student):

  1. Create a file upload exercise with a description. Upload an image to the description
  2. Start the exercise, the image should show up properly as well as the rest of the description
  3. Participate in the exercise
  4. Upload another file
  5. View/Download the image
  6. No issues should occur

Review Progress

Performance Review

  • I 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

Manual Tests

  • Test 1
  • Test 2

Test Coverage

@MarkusPaulsen MarkusPaulsen requested a review from a team July 7, 2023 05:24
@MarkusPaulsen MarkusPaulsen self-assigned this Jul 7, 2023
@github-actions github-actions bot added the server Pull requests that update Java code. (Added Automatically!) label Jul 7, 2023
@julian-christl julian-christl self-assigned this Jul 8, 2023
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 16, 2023
View reviewed changes
src/main/java/de/tum/in/www1/artemis/service/FileUploadSubmissionService.java

// We need to ensure that we can access the store file and the stored file is the same as was passed to us in the request
final var storedFileHash = DigestUtils.md5Hex(Files.newInputStream(Path.of(newLocalFilePath)));
final var storedFileHash = DigestUtils.md5Hex(Files.newInputStream(Path.of(savePath)));

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](1). This path depends on a [user-provided value](2). This path depends on a [user-provided value](2).
Strohgelaender
Strohgelaender previously requested changes Jul 21, 2023
View reviewed changes
Copy link
Contributor

@Strohgelaender Strohgelaender left a comment

Choose a reason for hiding this comment

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

The server style action is currently failing, please run spotlessApply to fix the reported issues.

@MarkusPaulsen MarkusPaulsen changed the title Development: Load participation from DB with file-upload submissions Development: Use participation from DB with file-upload submissions Jul 26, 2023
@MarkusPaulsen MarkusPaulsen changed the title Development: Use participation from DB with file-upload submissions Development: Use participation to get file-upload submissions from DB Jul 26, 2023
RY997
RY997 approved these changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@RY997 RY997 left a comment

Choose a reason for hiding this comment

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

Reapprove

@krusche krusche changed the title Development: Use participation to get file-upload submissions from DB Development: Delete old files properly when updating file submissions Jul 26, 2023
krusche
krusche approved these changes Jul 26, 2023
View reviewed changes
Copy link
Member

@krusche krusche left a comment

Choose a reason for hiding this comment

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

Code looks good to me and makes sense 👍

dearjasmina
dearjasmina approved these changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@dearjasmina dearjasmina left a comment

Choose a reason for hiding this comment

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

reapprove

Strohgelaender
Strohgelaender approved these changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@Strohgelaender Strohgelaender left a comment

Choose a reason for hiding this comment

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

code looks good.

@krusche krusche added this to the 6.3.8 milestone Jul 26, 2023
@krusche krusche merged commit d5ccc42 into develop Jul 26, 2023
@krusche krusche deleted the chore/utiliseStudentParticipation branch July 26, 2023 18:40
jakubriegel pushed a commit that referenced this pull request Jul 30, 2023
@MarkusPaulsen @jakubriegel
Development: Delete old files properly when updating file submissions (…
852e6bd 
…#6849)
bensofficial pushed a commit that referenced this pull request Aug 14, 2023
@MarkusPaulsen @bensofficial
Development: Delete old files properly when updating file submissions (…
30366eb 
…#6849)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bassner bassner bassner requested changes

@krusche krusche krusche approved these changes

@dearjasmina dearjasmina dearjasmina approved these changes

@Strohgelaender Strohgelaender Strohgelaender approved these changes

@JohannesStoehr JohannesStoehr Awaiting requested review from JohannesStoehr

@laadvo laadvo Awaiting requested review from laadvo

@tunargs tunargs Awaiting requested review from tunargs

@dmytropolityka dmytropolityka Awaiting requested review from dmytropolityka

@MaximilianAnzinger MaximilianAnzinger Awaiting requested review from MaximilianAnzinger

@SolizerCodes SolizerCodes Awaiting requested review from SolizerCodes

+1 more reviewer

@RY997 RY997 RY997 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@MarkusPaulsen MarkusPaulsen

@julian-christl julian-christl

Labels

ready to merge server Pull requests that update Java code. (Added Automatically!) small

Projects

None yet

Milestone

6.3.8

Development

Successfully merging this pull request may close these issues.