Development: Introduce fileupload module API
#10258
Conversation
github-actions
bot
added
tests
server
|
Warning Rate limit exceeded@ole-ve has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes refactor file upload exercise handling by replacing direct repository/service calls with Optional-based API injections. In multiple service and web classes, dependencies have been updated to use new API interfaces (e.g., FileUploadImportApi, FileSubmissionExportApi, FileUploadApi) and a custom ApiNotPresentException is thrown if the API is absent. Additionally, new abstract classes and API implementations have been introduced to modularize file upload, import, and export functionality, with corresponding adjustments in control flow and error handling. A new architecture test for the file upload module has also been added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ImportService
participant FileUploadImportApi
Client->>ImportService: importOrLoadExercise(fileUploadExercise)
ImportService->>FileUploadImportApi: Retrieve API (or throw exception)
alt API Available
FileUploadImportApi-->>ImportService: Return imported exercise
ImportService-->>Client: Return result
else API Missing
ImportService-->>Client: Throw ApiNotPresentException
end
sequenceDiagram
participant Client
participant ExportService
participant FileSubmissionExportApi
Client->>ExportService: Export file upload exercise
ExportService->>FileSubmissionExportApi: Check API and export submissions
alt API Available
FileSubmissionExportApi-->>ExportService: Return export result (Path)
ExportService-->>Client: Return export Path
else API Missing
ExportService-->>Client: Handle missing export API scenario
end
Possibly related PRs
Suggested labels
Suggested reviewers
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: 0
🧹 Nitpick comments (6)
src/main/java/de/tum/cit/aet/artemis/fileupload/api/FileSubmissionExportApi.java (2)
1-18: Add class-level JavaDoc documentation.As per PR objectives, add JavaDoc documentation to describe the purpose and responsibilities of this API class.
@Profile(PROFILE_CORE) @Controller +/** + * API facade for file upload exercise export operations. + * Part of the server modularization effort to provide a clean interface for file upload module functionality. + */ public class FileSubmissionExportApi extends AbstractFileModuleApi {
26-30: Add method-level JavaDoc documentation.Document the method parameters and return value to maintain consistency with JavaDoc standards.
+ /** + * Exports a file upload exercise with its submissions. + * + * @param exercise The file upload exercise to export + * @param optionsDTO Options controlling the export process + * @param exportDir Directory where the export will be saved + * @param exportErrors List to collect any export errors + * @param reportEntries List to collect archival report entries + * @return Path to the exported exercise + */ public Path exportFileUploadExerciseWithSubmissions(FileUploadExercise exercise, SubmissionExportOptionsDTO optionsDTO, Path exportDir, List<String> exportErrors, List<ArchivalReportEntry> reportEntries) {src/main/java/de/tum/cit/aet/artemis/core/exception/ApiNotPresentException.java (1)
10-19: Consider making the class final.The implementation is clean and follows best practices. Since this exception is not designed for inheritance, consider making it final for better encapsulation.
-public class ApiNotPresentException extends RuntimeException { +public final class ApiNotPresentException extends RuntimeException {src/main/java/de/tum/cit/aet/artemis/fileupload/api/FileUploadApi.java (1)
16-40: Add JavaDoc for public methods.The implementation follows best practices with proper dependency injection and clean delegation. However, public methods should be documented with JavaDoc to maintain consistency with the project's documentation standards.
Add JavaDoc for public methods:
+ /** + * Finds a file upload submission with associated team students, participation, and exercise. + * + * @param submissionId the ID of the submission + * @param exerciseId the ID of the exercise + * @return the found submission + * @throws javax.persistence.EntityNotFoundException if no submission is found + */ public FileUploadSubmission findWithTeamStudentsAndParticipationAndExerciseByIdAndExerciseIdElseThrow(long submissionId, long exerciseId) { + /** + * Creates a new file upload entry. + * + * @param path the path to the file + * @param serverFilePath the server-side path to the file + * @param fileName the name of the file + * @param entityId the ID of the associated entity + * @param entityType the type of the associated entity + */ public void createFileUpload(String path, String serverFilePath, String fileName, Long entityId, FileUploadEntityType entityType) { + /** + * Finds a file upload by its path. + * + * @param path the path to search for + * @return an Optional containing the file upload if found + */ public Optional<FileUpload> findByPath(String path) {src/main/java/de/tum/cit/aet/artemis/fileupload/api/FileUploadImportApi.java (1)
16-44: Add JavaDoc and consider method naming.The implementation follows best practices with proper dependency injection and clean delegation. However, there are a few improvements to consider:
- Public methods should be documented with JavaDoc
- The overloaded
importFileUploadExercisemethods could be renamed to better indicate their differencesConsider these improvements:
+ /** + * Finds a unique file upload exercise by title and course ID. + * + * @param title the title of the exercise + * @param courseId the ID of the course + * @return an Optional containing the exercise if found + * @throws NoUniqueQueryException if multiple exercises are found + */ public Optional<FileUploadExercise> findUniqueWithCompetenciesByTitleAndCourseId(String title, long courseId) throws NoUniqueQueryException { + /** + * Finds a file upload exercise with grading criteria by ID. + * + * @param exerciseId the ID of the exercise + * @return the found exercise + * @throws javax.persistence.EntityNotFoundException if no exercise is found + */ public FileUploadExercise findWithGradingCriteriaByIdElseThrow(Long exerciseId) { - public FileUploadExercise importFileUploadExercise(final FileUploadExercise templateExercise, FileUploadExercise importedExercise) { + /** + * Imports a file upload exercise using a template exercise. + * + * @param templateExercise the exercise to use as a template + * @param importedExercise the exercise to be imported + * @return the imported exercise + */ + public FileUploadExercise importFileUploadExerciseFromTemplate(final FileUploadExercise templateExercise, FileUploadExercise importedExercise) { - public Optional<FileUploadExercise> importFileUploadExercise(final long exerciseToCopyId, final FileUploadExercise importedExercise) { + /** + * Imports a file upload exercise using an exercise ID as template. + * + * @param exerciseToCopyId the ID of the exercise to use as a template + * @param importedExercise the exercise to be imported + * @return an Optional containing the imported exercise if successful + */ + public Optional<FileUploadExercise> importFileUploadExerciseFromId(final long exerciseToCopyId, final FileUploadExercise importedExercise) {src/main/java/de/tum/cit/aet/artemis/exam/service/ExamImportService.java (1)
286-326: Consider extracting exercise type handling to separate methods.The switch expression is clean, but the method is quite long. Consider extracting each exercise type handling into a separate private method for better maintainability.
Example for FILE_UPLOAD case:
+ private Optional<Exercise> handleFileUploadExercise(Exercise exerciseToCopy) { + if (fileUploadImportApi.isEmpty()) { + return Optional.empty(); + } + return fileUploadImportApi.get().importFileUploadExercise( + exerciseToCopy.getId(), + (FileUploadExercise) exerciseToCopy + ); + } Optional<? extends Exercise> exerciseCopied = switch (exerciseToCopy.getExerciseType()) { // ... other cases ... - case FILE_UPLOAD -> { - if (fileUploadImportApi.isEmpty()) { - yield Optional.empty(); - } - yield fileUploadImportApi.get().importFileUploadExercise(exerciseToCopy.getId(), (FileUploadExercise) exerciseToCopy); - } + case FILE_UPLOAD -> handleFileUploadExercise(exerciseToCopy); // ... other cases ... };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/core/exception/ApiNotPresentException.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/export/CourseExamExportService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java(8 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamImportService.java(6 hunks)src/main/java/de/tum/cit/aet/artemis/fileupload/api/AbstractFileModuleApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/fileupload/api/FileSubmissionExportApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/fileupload/api/FileUploadApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/fileupload/api/FileUploadImportApi.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/fileupload/architecture/FileUploadApiArchitectureTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/de/tum/cit/aet/artemis/fileupload/api/AbstractFileModuleApi.java
🧰 Additional context used
📓 Path-based instructions (9)
src/test/java/de/tum/cit/aet/artemis/fileupload/architecture/FileUploadApiArchitectureTest.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/exception/ApiNotPresentException.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/atlas/service/LearningObjectImportService.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/api/FileUploadApi.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/api/FileSubmissionExportApi.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/api/FileUploadImportApi.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/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/exam/service/ExamImportService.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/core/service/export/CourseExamExportService.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
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (17)
src/main/java/de/tum/cit/aet/artemis/core/service/export/CourseExamExportService.java (3)
43-43: LGTM!Clean import of the new API class.
72-72: LGTM!Good use of Optional pattern for the API dependency:
- Final field ensures immutability
- Constructor injection follows best practices
- Optional wrapper allows for graceful handling when the API is not present
Also applies to: 84-91
423-424: LGTM!Excellent use of Optional.ifPresent() to safely handle the API presence:
- Maintains consistent error handling with other exercise types
- Follows the same pattern of adding successful exports to the list
- Properly delegates to the API when available
src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java (5)
40-44: Imports for newly introduced API and exception
No issues identified. The imports correctly introduce theApiNotPresentExceptionandFileUploadImportApi.
82-82: Optional-based field injection
UsingOptional<FileUploadImportApi>facilitates a modular approach. This is a valid strategy for optional dependencies.
115-121: Constructor signature with optional API
This design cleanly includes the optional file upload API. No issues found.
125-125: Trivial field assignment
No further remarks needed.
201-205: Case block for FileUploadExercise
The usage oforElseThrowto handle the absent API scenario is consistent with the other exercise types. Looks good.src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (7)
50-50: Introduced imports for the new optional file upload API and exception
The imports are appropriate and consistent with the usage.Also applies to: 68-68
111-111: Optional FileUploadApi field
Optional injection is a suitable approach for modular features.
131-134: Constructor parameters updated with optional API
Good alignment of parameters. Ensures the API is injected only when present.
149-149: Field assignment
No issues. Straightforward assignment of the optional API field.
193-194: Creating file upload entry
UsingfileUploadApi.orElseThrowto ensure the API is available is valid. Properly handles the conversation file creation.
219-221: Retrieving file by path
Again, theorElseThrowapproach works well in a modular context.
330-332: Fetching submission with Optional-based API
This integration ensures synergy with the exercise’s existing security model. Implementation looks correct.src/test/java/de/tum/cit/aet/artemis/fileupload/architecture/FileUploadApiArchitectureTest.java (1)
1-11: New architecture test class
The class cleanly extendsAbstractModuleAccessArchitectureTestand sets the correct module package. This is a good practice for ensuring modular boundaries.src/main/java/de/tum/cit/aet/artemis/exam/service/ExamImportService.java (1)
73-73: LGTM! Clean dependency injection of optional API.The change to use
Optional<FileUploadImportApi>aligns well with the modularization effort and follows constructor injection best practices.Also applies to: 85-85, 99-99
helios-aet
bot
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
Checklist
General
Server
Motivation and Context
As part of the server modularization, we "facade" the service/repository declaration and method calls to module-external classes via a module API provided by the respective module.
Description
These changes add the module API for fileupload exercises.
Out of scope: Adding a
PROFILE_FILE(or similar) to disable the module on server start. This requires a lot of frontend changes and testing.Steps for Testing
Can only be test on TS3
Basically try to test all the functionality of fileupload exercises regarding submissions, courses, and import/export.
Exam Mode Testing
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
Performance Review
Code Review
Manual Tests
Exam Mode Test
Summary by CodeRabbit
New Features
Refactor
Tests