Communication: Keep filenames when downloading attachments
#9970
Conversation
…ion/keep-filenames-for-attachment-downloads
github-actions
bot
added
tests
server
…attachment-downloads
PaRangger
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmd (7.8.0)src/main/java/de/tum/cit/aet/artemis/fileupload/domain/FileUpload.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/fileupload/repository/FileUploadRepository.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadScheduleService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. WalkthroughThe pull request introduces a comprehensive file upload management system for the Artemis application. Key changes include modifications to the Changes
Sequence DiagramsequenceDiagram
participant FileResource
participant FileService
participant FileUploadService
participant FileUploadRepository
FileResource->>FileService: handleSaveFileInConversation
FileService-->>FileResource: FilePathInformation
FileResource->>FileUploadService: createFileUpload
FileUploadService->>FileUploadRepository: save
FileUploadService-->>FileResource: File Upload Created
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)src/main/java/de/tum/cit/aet/artemis/fileupload/repository/FileUploadRepository.java (1)Pattern src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadScheduleService.java (1)Pattern 🔇 Additional comments (8)src/main/java/de/tum/cit/aet/artemis/fileupload/repository/FileUploadRepository.java (4)
The method is properly documented with clear return value specification.
The hardcoded value
The method uses proper parameter annotation and has clear documentation.
Replace star import with explicit imports According to the coding guidelines, star imports should be avoided. -import java.util.List;
+import java.util.List;
src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadScheduleService.java (4)
The class follows best practices with constructor injection and well-named constants.
The scheduled task should catch and log exceptions to prevent interruption of the scheduler. Consider staggering scheduled tasks Both cleanup tasks are scheduled to run very close to each other (midnight and 12:05 AM). Consider spreading them out more to avoid potential resource contention.
The method uses a well-defined constant for the deletion threshold and has a clear implementation.
Replace the if statement with a switch statement to handle all FileUploadEntityType values more maintainably. 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadScheduleService.java (1)
34-34: Define cron expressions as constants for better maintainabilityThe cron expressions in the
@Scheduledannotations are hardcoded. Defining them as constants improves readability and makes future updates easier.Extract the cron expression into a constant:
+ private static final String DAILY_CRON_EXPRESSION = "0 0 0 * * *"; @Scheduled(cron = DAILY_CRON_EXPRESSION) public void cleanupOrphanedFileUploads() { // method body } @Scheduled(cron = DAILY_CRON_EXPRESSION) public void cleanupNullEntityFileUploads() { // method body }Also applies to: 47-47
src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (1)
Line range hint
131-153: Refactor constructor to reduce the number of parametersThe
FileResourceconstructor now has 17 parameters, which can make the class hard to maintain and test. Consider refactoring to improve code readability and manageability.Possible approaches:
Use setter injection: Inject some dependencies via setters rather than the constructor.
Group services into composite objects: Create helper classes that group related services.
Apply the Builder Pattern: Use a builder to construct the
FileResourcewith only necessary dependencies.src/main/java/de/tum/cit/aet/artemis/fileupload/repository/FileUploadRepository.java (1)
35-41: Optimize JPQL queries for better performanceBoth queries could benefit from performance optimizations:
- For orphaned conversations query, consider using EXISTS clause instead of LEFT JOIN
- For null entity references query, consider adding an index on createdAt column
- @Query(""" - SELECT f - FROM FileUpload f - LEFT JOIN Conversation c ON f.entityId = c.id - WHERE f.entityType = 0 - AND c.id IS NULL - """) + @Query(""" + SELECT f + FROM FileUpload f + WHERE f.entityType = de.tum.cit.aet.artemis.fileupload.domain.FileUploadEntityType.CONVERSATION + AND NOT EXISTS ( + SELECT 1 + FROM Conversation c + WHERE c.id = f.entityId + ) + """)Also applies to: 51-56
src/test/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadScheduleServiceTest.java (2)
39-40: Improve test documentation for future maintainersThe comment about new entity types should be more descriptive and moved to Javadoc.
- // If this fails you may have added a new entity type that needs to be added as a case to the function + /** + * This test verifies the cleanup of orphaned file uploads. + * Note: If this test fails after adding a new FileUploadEntityType, + * you need to update the cleanupOrphanedFileUploads method to handle the new type. + */ fileUploadScheduleService.cleanupOrphanedFileUploads();
51-57: Improve test reliability for time-based operationsThe current implementation could be flaky due to time-based assertions. Consider using a Clock abstraction for better testability.
+ @Mock + private Clock clock; + + private static final ZonedDateTime FIXED_TIME = ZonedDateTime.of(2024, 1, 1, 12, 0, 0, 0, ZoneId.systemDefault()); + @Test void shouldDeleteNullEntityFileUploadsWhenOlderThanThreeDays() { - ZonedDateTime cutoffDate = ZonedDateTime.now().minusDays(FileUploadScheduleService.DAYS_UNTIL_NULL_ENTITY_FILES_ARE_DELETED); + when(clock.instant()).thenReturn(FIXED_TIME.toInstant()); + when(clock.getZone()).thenReturn(FIXED_TIME.getZone()); + + ZonedDateTime cutoffDate = FIXED_TIME.minusDays(FileUploadScheduleService.DAYS_UNTIL_NULL_ENTITY_FILES_ARE_DELETED);src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadService.java (1)
50-52: Fix incorrect JavadocThe Javadoc comment appears to be copied from createFileUpload method.
- /** - * Creates a new file upload entity. + /** + * Finds a file upload entity by its path.src/test/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadServiceTest.java (2)
45-57: Enhance test coverage for FileUpload creationAdd test cases for:
- Invalid paths (path traversal attempts)
- Invalid filenames
- Verification of createdAt initialization
- Edge cases for entityId and entityType being null
Example test case:
@Test void shouldRejectInvalidFilePaths() { String path = "../test/path"; String serverFilePath = "/server/file/path"; String fileName = "test.txt"; assertThatThrownBy(() -> fileUploadService.createFileUpload(path, serverFilePath, fileName, null, null)) .isInstanceOf(IllegalArgumentException.class); }
92-94: Improve deletion testing and avoid hard-coded values
- The delay value (1) should be configurable and not hard-coded
- Add tests for concurrent deletion scenarios
- Consider testing the order of operations (scheduling before deletion)
- verify(fileService).schedulePathForDeletion(Path.of("/path/to/file1"), 1); - verify(fileService).schedulePathForDeletion(Path.of("/path/to/file2"), 1); + verify(fileService).schedulePathForDeletion(Path.of("/path/to/file1"), DELETION_DELAY); + verify(fileService).schedulePathForDeletion(Path.of("/path/to/file2"), DELETION_DELAY);🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 92-92: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: (Path.of("/path/to/file2"), 1)
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration(missing-secure-java)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20241206114231_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (10)
src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/fileupload/domain/FilePathInformation.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/fileupload/domain/FileUpload.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/fileupload/domain/FileUploadEntityType.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/fileupload/repository/FileUploadRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadScheduleService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadService.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadScheduleServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadServiceTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/de/tum/cit/aet/artemis/fileupload/domain/FilePathInformation.java
🧰 Additional context used
📓 Path-based instructions (9)
src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadScheduleServiceTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/main/java/de/tum/cit/aet/artemis/fileupload/domain/FileUploadEntityType.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadScheduleService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/fileupload/repository/FileUploadRepository.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadServiceTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/fileupload/domain/FileUpload.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
🪛 ast-grep (0.31.1)
src/test/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadServiceTest.java
[warning] 91-91: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (Path.of("/path/to/file1"), 1)
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
(missing-secure-java)
[warning] 92-92: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (Path.of("/path/to/file2"), 1)
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
(missing-secure-java)
[warning] 101-101: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (new InvalidPathException("", ""))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
(missing-secure-java)
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java (1)
198-214: LGTM
The changes in handleSaveFileInConversation correctly handle the sanitized original filename and return comprehensive file path information. The method appropriately validates the file extension and uses the sanitized filename consistently.
src/main/java/de/tum/cit/aet/artemis/fileupload/domain/FileUploadEntityType.java (1)
1-23: LGTM
The FileUploadEntityType enum is well-defined with appropriate methods for database key mapping. This setup facilitates future expansion if new entity types are introduced.
src/test/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadServiceTest.java (1)
110-118: LGTM! Good edge case coverage
The test properly verifies the behavior with an empty list and ensures no unnecessary service interactions.
src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadScheduleService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadScheduleService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/fileupload/repository/FileUploadRepository.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/fileupload/domain/FileUpload.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/fileupload/domain/FileUpload.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadScheduleService.java
Outdated
Show resolved
Hide resolved
src/main/resources/config/liquibase/changelog/20241206114231_changelog.xml
Show resolved
Hide resolved
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
JerroyTan
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
zagemello
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
Longppham
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
|
Tested on TS2, works as expected: |
flbrgit
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
…attachment-downloads
…attachment-downloads
…attachment-downloads
…attachment-downloads
…attachment-downloads
…attachment-downloads
Checklist
General
Server
Motivation and Context
Currently, when downloading an attached file from a conversation, the file is saved using a server-generated filename (e.g.,
Markdown_2024-12-08T21-50-24-691_69ca7683.pdf). However, this approach disregards the original filename, which may have been intentionally chosen by the author. Furthermore, files are never deleted from the server, even when their associated conversations are removed, resulting in unnecessary storage retention.Description
I introduced a new model to store metadata about filenames, which also acts as a reference for cleanup operations. For security reasons, the files stored on the server remain unchanged, retaining their original server-generated names. When a user downloads a file, the system retrieves the original filename and includes it in the
Content-Dispositionheaders.Additionally, a scheduled service runs daily at a designated time to identify and safely delete unused files from the server.
This update establishes a foundation for preserving original filenames and managing file cleanup. However, the current implementation is limited to files uploaded via drag-and-drop into the text box within communication channels.
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Summary by CodeRabbit
New Features
FilePathInformationrecord to encapsulate file path details.Bug Fixes
Tests
FileUploadScheduleServiceandFileUploadServiceto ensure proper functionality and error handling.