-
Notifications
You must be signed in to change notification settings - Fork 275
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.
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
Development
: Handle course image persistence directly
#7912
Conversation
WalkthroughThe software update streamlines file management within a Java application. Redundancies are eliminated by removing unused imports and file handling methods. Enhancements focus on file path generation and file saving processes, incorporating new methods for filename generation and ensuring null safety. Changes in file path handling affect several services and resources, indicating a shift towards a more unified and efficient file management system within the application. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
src/main/java/de/tum/in/www1/artemis/service/FileService.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/de/tum/in/www1/artemis/service/FileService.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/de/tum/in/www1/artemis/service/LectureUnitProcessingService.java
Dismissed
Show dismissed
Hide dismissed
src/main/java/de/tum/in/www1/artemis/service/QuizExerciseService.java
Dismissed
Show dismissed
Hide dismissed
658041a
to
92cf859
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (7)
- src/main/java/de/tum/in/www1/artemis/domain/Course.java (3 hunks)
- src/main/java/de/tum/in/www1/artemis/service/EntityFileService.java (1 hunks)
- src/main/java/de/tum/in/www1/artemis/service/FileService.java (5 hunks)
- src/main/java/de/tum/in/www1/artemis/service/LectureUnitProcessingService.java (1 hunks)
- src/main/java/de/tum/in/www1/artemis/service/QuizExerciseService.java (1 hunks)
- src/main/java/de/tum/in/www1/artemis/web/rest/CourseResource.java (5 hunks)
- src/main/java/de/tum/in/www1/artemis/web/rest/admin/AdminCourseResource.java (5 hunks)
Files skipped from review due to trivial changes (1)
- src/main/java/de/tum/in/www1/artemis/domain/Course.java
Additional comments: 21
src/main/java/de/tum/in/www1/artemis/service/EntityFileService.java (1)
- 71-72: The new
generateFilename
method is used to generate the filename, which is then resolved to the target folder. This change affects the file path generation and assignment process within themoveFileBeforeEntityPersistenceWithIdIfIsTemp
method. Ensure that thegenerateFilename
method correctly handles edge cases and potential security issues related to file path manipulation.src/main/java/de/tum/in/www1/artemis/web/rest/admin/AdminCourseResource.java (2)
- 131-134: The
createCourse
method now handles file paths and saving directly. Ensure that thesaveFileFoo
method (which seems to be a placeholder name and should be verified) correctly saves the file and that thepublicPathForActualPathOrThrow
method properly converts the saved path to a public path. Also, verify that the file handling process does not introduce any security vulnerabilities, such as path traversal or improper access controls.- 160-162: The
deleteCourse
method now includes file path handling for course icons. Ensure that theschedulePathForDeletion
method correctly schedules the deletion of the course icon file and that there are no race conditions or security issues with the deletion process.src/main/java/de/tum/in/www1/artemis/service/LectureUnitProcessingService.java (1)
- 214-214: The
saveTempFileForProcessing
method now uses a different approach for generating the file path and filename. Ensure that thegenerateFilename
method is secure and correctly handles the file name generation to prevent any security issues such as path traversal or file name prediction attacks.src/main/java/de/tum/in/www1/artemis/service/QuizExerciseService.java (1)
- 383-383: The
saveDragAndDropImage
method has been modified to use thebasePath.resolve
method instead offileService.generateFilePath
to generate thesavePath
. Ensure that the new logic for constructing thesavePath
is secure and does not introduce any vulnerabilities, such as path traversal or improper file access.src/main/java/de/tum/in/www1/artemis/service/FileService.java (10)
- 166-166: The method
handleSaveFile
has been modified to include a call tovalidateExtension
which checks the file extension against a set of allowed extensions. This is a security improvement to prevent the upload of potentially dangerous file types.- 177-178: The
generateFilename
method is used to create a filename whenkeepFilename
is false. This method uses the current time and a random UUID to generate a unique filename, which is a good practice to avoid collisions and potential security issues with user-provided filenames.- 181-185: The
copyFile
method is called to handle the actual file copying. The methodcopyFile
is private and encapsulates the file copying logic, which is a good example of the Single Responsibility Principle.- 204-209: The
copyFile
method is a new private method that encapsulates the logic for copying a file from anInputStream
to aPath
. This is a good example of refactoring for better readability and maintainability.- 221-222: The
checkAndSanitizeFilename
method has been made public and annotated with null safety annotations. This change increases the method's visibility for reuse and ensures that it handles null values correctly.- 237-242: The
validateExtension
method has been refactored to check the file extension against a set of allowed extensions. This is a security improvement to ensure that only files with allowed extensions can be uploaded.- 253-254: The
generateFilename
method has been refactored to generate a filename based on the current time and a random UUID. This is a good practice to avoid filename collisions and potential security issues.- 268-268: The
copyExistingFileToTarget
method has been modified to use thegenerateFilename
method for generating the target filename. This ensures consistency in filename generation across the service.- 178-178: The bot has flagged potential security issues with uncontrolled data used in path expressions. The use of
generateFilename
mitigates this by generating a safe filename, but it's important to ensure that the base path (path
) is also safe and not user-controlled.- 199-199: The bot has flagged potential security issues with uncontrolled data used in path expressions. The use of
generateFilename
mitigates this by generating a safe filename, but it's important to ensure that the base path (basePath
) is also safe and not user-controlled.src/main/java/de/tum/in/www1/artemis/web/rest/CourseResource.java (6)
- 8-9: The addition of
java.net.URI
andjava.net.URISyntaxException
imports indicates new functionality related to URI handling has been introduced.- 116-116: A new field
FilePathService filePathService
has been added to the class. This is in line with the PR's objective to handle file paths more directly.- 124-124: The constructor now accepts
FilePathService
as a parameter, which is correctly assigned to the class field. This change is necessary to support the new file path handling logic.- 158-158: The
updateCourse
method now declares that it throwsURISyntaxException
. This change is consistent with the new file handling logic that can potentially throw this exception.- 231-237: The logic for handling file paths and icons has been updated. The
FilePathService
is used to determine the base path for course icons and to generate a public path for the saved file. This aligns with the PR's objective to streamline file handling.- 239-241: The code correctly handles the case where the course icon is removed by scheduling the old icon for deletion. This is a good use of the
FilePathService
to manage file system paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- src/main/java/de/tum/in/www1/artemis/service/FileService.java (5 hunks)
- src/main/java/de/tum/in/www1/artemis/web/rest/CourseResource.java (5 hunks)
- src/main/java/de/tum/in/www1/artemis/web/rest/admin/AdminCourseResource.java (5 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/main/java/de/tum/in/www1/artemis/service/FileService.java
- src/main/java/de/tum/in/www1/artemis/web/rest/CourseResource.java
- src/main/java/de/tum/in/www1/artemis/web/rest/admin/AdminCourseResource.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
manually tested on ts1, works as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on Ts2. Works as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS4. Works as expected. Code lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks clean and I don't see any issues 👍🏽
Checklist
General
Server
Motivation and Context
In #5733 I replaced the two-parted upload with a single multipart upload but forgot to remove the delayed persistence of the file path that was now obsolete.
Description
In this PR, I opened the API of the file service to allow services to save a file directly to a path instead of saving it to a temporary path as before. I removed the delayed persistence method from the Course object and let the course resources handle the file saving directly.
Creating a course requires an additional database call as the current implementation requires the ID as part of the course Icon public path. There are ways to prevent this, but it requires a completely different implementation or to keep replacing the ID in the path on load.
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Only changes that are expected to require no test adaptions. All changes in FileService (<90%) have enough coverage.
Summary by CodeRabbit
Refactor
Bug Fixes
New Features
Documentation