Development: Introduce plagiarism module API
#10296
Conversation
github-actions
bot
added
tests
server
WalkthroughThis pull request refactors and extends plagiarism-related functionality across multiple modules. It transitions from direct repository and service dependencies to an API-based approach using Changes
Sequence Diagram(s)sequenceDiagram
participant S as Service
participant O as Optional Plagiarism API
participant R as Repository/Underlying Service
S->>O: Check if API is present
alt API is available
S->>O: Invoke plagiarism method (e.g., findByCourseIdAndStudentId)
O->>R: Retrieve plagiarism data
R-->>O: Return data
O-->>S: Return plagiarism data
else API is absent
S-->>S: Return empty result or throw ApiNotPresentException
end
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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
🔭 Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/assessment/web/GradeStepResource.java (1)
173-173: Consider adding error handling for plagiarism access check.The current implementation silently handles potential failures in plagiarism access checks. Consider adding error handling to ensure security is not compromised.
- plagiarismApi.ifPresent(api -> api.checkAccessAndAnonymizeSubmissionForStudent(textSubmission, userRepository.getUser().getLogin(), textSubmission.getParticipation())); + plagiarismApi.ifPresent(api -> { + try { + api.checkAccessAndAnonymizeSubmissionForStudent(textSubmission, userRepository.getUser().getLogin(), textSubmission.getParticipation()); + } catch (Exception e) { + log.error("Failed to check access and anonymize submission for student: {}", e.getMessage()); + throw new AccessForbiddenException("Failed to verify submission access"); + } + });
♻️ Duplicate comments (1)
src/main/java/de/tum/cit/aet/artemis/text/web/TextSubmissionResource.java (1)
173-173: 🛠️ Refactor suggestionConsider adding error handling for plagiarism access check.
The current implementation silently handles potential failures in plagiarism access checks. Consider adding error handling to ensure security is not compromised.
🧹 Nitpick comments (18)
src/main/java/de/tum/cit/aet/artemis/communication/service/PostingService.java (2)
111-120: Access level changes align with module API requirements.The visibility changes from
protectedtopublicfor these methods support the modularization effort by allowing the new plagiarism module API to access these methods. This change follows the principle of least access while still enabling necessary inter-module communication.Consider documenting these methods as part of the public API to ensure proper usage by other modules.
Also applies to: 130-156, 200-206
152-155: Consider extracting plagiarism-specific logic to a dedicated method.The plagiarism case handling in
broadcastForPostcould be extracted to maintain single responsibility and improve modularity.else if (postDTO.post().getPlagiarismCase() != null) { - String plagiarismCaseTopic = METIS_WEBSOCKET_CHANNEL_PREFIX + "plagiarismCase/" + postDTO.post().getPlagiarismCase().getId(); - websocketMessagingService.sendMessage(plagiarismCaseTopic, postDTO); + broadcastForPlagiarismCase(postDTO); } +private void broadcastForPlagiarismCase(PostDTO postDTO) { + String plagiarismCaseTopic = METIS_WEBSOCKET_CHANNEL_PREFIX + "plagiarismCase/" + postDTO.post().getPlagiarismCase().getId(); + websocketMessagingService.sendMessage(plagiarismCaseTopic, postDTO); +}src/main/java/de/tum/cit/aet/artemis/plagiarism/service/ContinuousPlagiarismControlService.java (1)
84-84: Add error handling for config creation.While the helper method usage improves code reusability, consider handling potential database errors during config creation separately from the plagiarism check errors.
- PlagiarismDetectionConfigHelper.createAndSaveDefaultIfNullAndCourseExercise(exercise, exerciseRepository); + try { + PlagiarismDetectionConfigHelper.createAndSaveDefaultIfNullAndCourseExercise(exercise, exerciseRepository); + } catch (Exception e) { + log.error("Failed to create/update plagiarism detection config: exerciseId={}, error={}", exercise.getId(), e.getMessage(), e); + return; + }src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingExerciseResource.java (4)
109-109: Consider injecting a default implementation in lieu of using Optional.
Storing Optional might be appropriate for toggling the feature, but an alternative is to inject a “no-op” or default instance if the module is absent, avoiding the need to handle Optionals everywhere.
119-119: Optional injection is functionally fine, but ensure consistent usage.
Similarly to plagiarismResultApi, consider a default instance for PlagiarismDetectionApi to simplify feature toggling logic.
127-130: Constructor signature expansion is valid but watch for length creep.
The new optional API arguments conform to the new design; just be mindful that the constructor is becoming lengthy, which can affect readability and maintainability.
446-446: Keep performance considerations in mind when invoking manual checks.
Depending on the size of submissions, this call could be computationally expensive. Evaluate whether asynchronous processing is needed.src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExercisePlagiarismResource.java (2)
106-107: Repeated pattern in retrieving plagiarismDetectionApi.The direct usage of “.orElseThrow()” is clear. However, verify that any repeated retrieval logic couldn’t be refactored into a helper method to reduce duplication.
140-141: Repeated retrieval of detection API.This logic is identical to lines 106-107. You might consider a helper method or utility to unify these calls and reduce repetitive code.
src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryAccessService.java (1)
176-177: Avoid calling get() on Optional to reduce potential confusion.Your condition checks both isEmpty() and get(). This works but can be more readable with something like:
“plagiarismApi.map(api -> api.hasAccessToSubmission(…)).orElse(true) …”
proposed approach to reduce direct get() usage:
-if (plagiarismApi.isEmpty() || plagiarismApi.get().hasAccessToSubmission(...)) { +if (plagiarismApi.map(api -> api.hasAccessToSubmission(...)).orElse(true)) { return; }src/main/java/de/tum/cit/aet/artemis/exam/service/ExamService.java (1)
274-274: Safe usage of Optional.map for retrieving cases.
Retrieving plagiarism cases using map(...) and .orElse(List.of()) is a neat approach to avoid null checks. Just remember List.of() is immutable; if modification is required, consider using new ArrayList<>(...) instead.src/main/java/de/tum/cit/aet/artemis/plagiarism/api/AbstractPlagiarismApi.java (1)
1-6: Consider adding class-level JavaDoc.
Providing a brief JavaDoc explaining the purpose and usage of this abstract base class would help future maintainers.src/main/java/de/tum/cit/aet/artemis/core/exception/ApiNotPresentException.java (1)
6-9: Enhance JavaDoc documentation.While the documentation is present, it could be more detailed to include:
- Example usage scenarios
- Common cases when this exception is thrown
- Impact on the application flow
/** * Exception that an optionally autowired API is not present. * This is caused by Spring profiles not being present. + * + * <p>This exception is thrown when attempting to use an API that requires a specific + * Spring profile, but that profile is not active. For example, when trying to use + * the PlagiarismApi without the 'core' profile enabled.</p> + * + * <p>To resolve this, ensure the required Spring profile is enabled in your + * application configuration.</p> */src/test/java/de/tum/cit/aet/artemis/plagiarism/architecture/PlagiarismApiArchitectureTest.java (1)
10-21: Add class-level documentation to explain test purpose and ignored classes.The test class would benefit from documentation explaining:
- The purpose of the architecture test
- Why specific classes are ignored
- How it fits into the overall testing strategy
+/** + * Architecture test for the plagiarism module that enforces module access rules. + * <p> + * This test ensures that: + * - Classes outside the plagiarism module can only access public APIs + * - Internal implementation details remain encapsulated + * <p> + * The following classes are ignored from these checks: + * - ModelingExerciseResource: Requires direct access for modeling-specific logic + * - TextExerciseResource: Requires direct access for text exercise handling + * - ProgrammingExercisePlagiarismResource: Contains legacy code to be migrated + */ class PlagiarismApiArchitectureTest extends AbstractModuleAccessArchitectureTest {src/main/java/de/tum/cit/aet/artemis/plagiarism/domain/PlagiarismMapping.java (1)
10-10: Consider using ImmutableMap for better immutability guarantees.The record's map field could be modified after construction since
Mapis mutable. Consider using Guava'sImmutableMapto ensure true immutability.-public record PlagiarismMapping(Map<Long, Map<Long, PlagiarismCase>> studentIdToExerciseIdToPlagiarismCaseMap) { +public record PlagiarismMapping(ImmutableMap<Long, ImmutableMap<Long, PlagiarismCase>> studentIdToExerciseIdToPlagiarismCaseMap) {src/main/java/de/tum/cit/aet/artemis/plagiarism/service/PlagiarismDetectionService.java (1)
97-101: Update method documentation to reflect exception handling.The JavaDoc should include
@throws ProgrammingLanguageNotSupportedForPlagiarismDetectionExceptionto document the new exception./** * Check plagiarism in given programing exercise and outputs a Jplag report * * @param exercise exercise to check plagiarism * @return Jplag report of plagiarism checks + * @throws ProgrammingLanguageNotSupportedForPlagiarismDetectionException if the programming language does not support plagiarism detection */src/main/java/de/tum/cit/aet/artemis/communication/service/ReactionService.java (1)
162-175: Extract common API retrieval pattern.The API retrieval and exception handling pattern is duplicated across multiple methods. Consider extracting it to a private method.
+ private PlagiarismPostApi getApi() { + return plagiarismPostApi.orElseThrow(() -> new ApiNotPresentException(PlagiarismPostApi.class, PROFILE_CORE)); + } private Reaction createReactionForAnswer(Reaction reaction, AnswerPost posting, User user, Course course) { - var api = plagiarismPostApi.orElseThrow(() -> new ApiNotPresentException(PlagiarismPostApi.class, PROFILE_CORE)); + var api = getApi(); // ... rest of the method } private Reaction createReactionForPost(Reaction reaction, Post posting, User user, Course course) { - var api = plagiarismPostApi.orElseThrow(() -> new ApiNotPresentException(PlagiarismPostApi.class, PROFILE_CORE)); + var api = getApi(); // ... rest of the method }Also applies to: 189-209
src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseDeletionService.java (1)
182-183: Consider adding error logging for plagiarism result deletion.The current implementation silently handles the case when plagiarism results cannot be deleted. Consider adding error logging to track potential issues.
- plagiarismResultApi.ifPresent(api -> api.deletePlagiarismResultsByExerciseId(exerciseId)); + plagiarismResultApi.ifPresent(api -> { + try { + api.deletePlagiarismResultsByExerciseId(exerciseId); + } catch (Exception e) { + log.error("Failed to delete plagiarism results for exercise {}: {}", exerciseId, e.getMessage()); + } + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
src/main/java/de/tum/cit/aet/artemis/assessment/service/CourseScoreCalculationService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/assessment/web/GradeStepResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/PostingService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/ReactionService.java(7 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/CourseService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/export/DataExportExerciseCreationService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamService.java(6 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseDeletionService.java(6 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseResource.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingExerciseResource.java(7 hunks)src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingSubmissionResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/api/AbstractPlagiarismApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismCaseApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismDetectionApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismPostApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismResultApi.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/domain/PlagiarismDetectionConfigHelper.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/domain/PlagiarismMapping.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/exception/ProgrammingLanguageNotSupportedForPlagiarismDetectionException.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/service/ContinuousPlagiarismControlService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/service/PlagiarismCaseService.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/plagiarism/service/PlagiarismDetectionService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryAccessService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExercisePlagiarismResource.java(7 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.java(8 hunks)src/main/java/de/tum/cit/aet/artemis/text/web/TextSubmissionResource.java(4 hunks)src/test/java/de/tum/cit/aet/artemis/plagiarism/ContinuousPlagiarismControlServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/plagiarism/PlagiarismDetectionConfigHelperTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/plagiarism/PlagiarismDetectionServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/plagiarism/architecture/PlagiarismApiArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java(3 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/de/tum/cit/aet/artemis/plagiarism/service/PlagiarismCaseService.java
✅ Files skipped from review due to trivial changes (4)
- src/test/java/de/tum/cit/aet/artemis/plagiarism/PlagiarismDetectionConfigHelperTest.java
- src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java
- src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java
- src/main/java/de/tum/cit/aet/artemis/plagiarism/domain/PlagiarismDetectionConfigHelper.java
🧰 Additional context used
📓 Path-based instructions (2)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
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/plagiarism/api/AbstractPlagiarismApi.javasrc/main/java/de/tum/cit/aet/artemis/core/exception/ApiNotPresentException.javasrc/main/java/de/tum/cit/aet/artemis/plagiarism/service/ContinuousPlagiarismControlService.javasrc/main/java/de/tum/cit/aet/artemis/plagiarism/exception/ProgrammingLanguageNotSupportedForPlagiarismDetectionException.javasrc/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseDeletionService.javasrc/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismApi.javasrc/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExercisePlagiarismResource.javasrc/main/java/de/tum/cit/aet/artemis/core/service/export/DataExportExerciseCreationService.javasrc/main/java/de/tum/cit/aet/artemis/assessment/web/GradeStepResource.javasrc/main/java/de/tum/cit/aet/artemis/communication/service/ReactionService.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamService.javasrc/main/java/de/tum/cit/aet/artemis/assessment/service/CourseScoreCalculationService.javasrc/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismDetectionApi.javasrc/main/java/de/tum/cit/aet/artemis/text/web/TextSubmissionResource.javasrc/main/java/de/tum/cit/aet/artemis/plagiarism/domain/PlagiarismMapping.javasrc/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismCaseApi.javasrc/main/java/de/tum/cit/aet/artemis/communication/service/PostingService.javasrc/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.javasrc/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismPostApi.javasrc/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismResultApi.javasrc/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseResource.javasrc/main/java/de/tum/cit/aet/artemis/core/service/CourseService.javasrc/main/java/de/tum/cit/aet/artemis/plagiarism/service/PlagiarismDetectionService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryAccessService.javasrc/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingSubmissionResource.javasrc/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingExerciseResource.java
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
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/test/java/de/tum/cit/aet/artemis/plagiarism/architecture/PlagiarismApiArchitectureTest.javasrc/test/java/de/tum/cit/aet/artemis/plagiarism/ContinuousPlagiarismControlServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/plagiarism/PlagiarismDetectionServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java
📓 Learnings (2)
src/main/java/de/tum/cit/aet/artemis/assessment/service/CourseScoreCalculationService.java (2)
Learnt from: julian-christl
PR: ls1intum/Artemis#9322
File: src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java:0-0
Timestamp: 2024-11-12T12:51:51.201Z
Learning: In Artemis, an `ExerciseGroup` always has an associated `Exam`, so `exerciseGroup.exam` is never null.
Learnt from: Strohgelaender
PR: ls1intum/Artemis#8030
File: src/main/java/de/tum/in/www1/artemis/service/CourseScoreCalculationService.java:21-49
Timestamp: 2024-11-12T12:52:03.805Z
Learning: The project consistently avoids using star-imports in Java files.
src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryAccessService.java (2)
Learnt from: julian-christl
PR: ls1intum/Artemis#9322
File: src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java:0-0
Timestamp: 2024-11-12T12:51:51.201Z
Learning: In Artemis, an `ExerciseGroup` always has an associated `Exam`, so `exerciseGroup.exam` is never null.
Learnt from: julian-christl
PR: ls1intum/Artemis#9322
File: src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java:170-172
Timestamp: 2024-11-12T12:51:46.554Z
Learning: In Artemis, `exercise.exerciseGroup` may be null, as not all exercises belong to an `ExerciseGroup`.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: Analyse
🔇 Additional comments (83)
src/main/java/de/tum/cit/aet/artemis/plagiarism/service/ContinuousPlagiarismControlService.java (2)
25-25: LGTM!The import statement follows Java conventions and is properly organized. Moving the helper class to the domain package aligns with the modularization effort.
38-40: LGTM! Well-structured service implementation.The service follows best practices including:
- Constructor-based dependency injection
- Comprehensive error handling
- Consistent logging patterns
- Clear separation of concerns
src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingExerciseResource.java (6)
41-41: Imports for new exception class look appropriate.
The import of ApiNotPresentException aligns with the new architecture for optional plagiarism APIs.
67-70: New Plagiarism-related imports are consistent with the module’s design.
These imports from the plagiarism package clearly indicate a shift toward an API-based approach and comply with the naming conventions.
136-146: Field assignments for the optional APIs are consistent with constructor injection.
These assignments correctly bind the constructor parameters to the fields. No issues observed.
414-415: Runtime exception design is clear for missing plagiarismResultApi.
Throwing ApiNotPresentException helps immediately surface a missing optional API. This is a straightforward and effective solution.
418-419: Potential for ClassCastException with the cast to ModelingPlagiarismResult.
If the API were to return a different implementation, this cast would fail. Ensure the upstream contract guarantees the expected type.
439-440: Similar exception handling for plagiarismDetectionApi.
Consistent approach to fail fast if the optional plagiarism detection API is not present.src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExercisePlagiarismResource.java (6)
8-8: Consistent imports for optional API approach.The newly introduced imports appear consistent with the shift to an API-based plagiarism architecture. Using explicit imports (instead of wildcard) aligns with best practices.
Also applies to: 24-24, 32-35, 38-38
59-61: Adoption of Optional fields.Defining these fields as Optionals is a clean way to handle conditional injection. Ensure that all relevant methods consistently check for their presence to avoid unexpected behavior.
64-64: Constructor injection of Optionals is well-structured.Constructor injection follows recommended dependency injection guidelines, promoting testability and modular design.
Also applies to: 67-68
81-82: Throwing ApiNotPresentException ensures clear error handling.Using an explicit exception to indicate the absence of the required API clarifies the error scenario for consumers. This is an effective mechanism to fail fast.
85-86: Casting to TextPlagiarismResult may risk runtime exceptions.Consider verifying that the returned result is actually a TextPlagiarismResult before casting, especially if the underlying API can yield multiple result types for different exercises.
150-150: Generating JPlag report via API.Ensuring JPlag functionality is now neatly encapsulated in the API. Double-check that any exception handling or resource cleanup is properly handled downstream if the API call fails.
src/main/java/de/tum/cit/aet/artemis/programming/service/RepositoryAccessService.java (2)
5-6: New imports for optional PlagiarismApi integration.Imports align with the new optional API approach, promoting a flexible design that can support or skip plagiarism checks.
Also applies to: 16-16
28-28: Optional field and constructor injection.The optional dependency clearly indicates that plagiarism checks may be unavailable. This is a streamlined way to keep the service decoupled from specific implementations.
Also applies to: 34-35
src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingSubmissionResource.java (2)
57-57: Optional plagiarism API field and constructor support.Switching from direct service dependencies to an Optional API decouples the module from mandatory plagiarism checks. This improves extensibility and future maintainability.
Also applies to: 84-84, 89-89, 96-96
208-209: Conditional anonymization via plagiarismApi usage.Invoking checkAccessAndAnonymizeSubmissionForStudent introduces a clear pattern for partial data hiding. Confirm that returning a 200 OK (instead of a forbidden status) is desired when a user is unauthorized to assess the submission.
src/main/java/de/tum/cit/aet/artemis/assessment/service/CourseScoreCalculationService.java (7)
45-45: Good addition of PlagiarismCaseApi import.
This aligns with the refactoring towards a dedicated API for plagiarism handling.
47-47: Importing PlagiarismMapping is consistent.
This domain object appears necessary for mapping student verdicts.
65-65: Optional field usage is clear.
Declaring the field as Optional promotes explicit handling of the presence or absence of the API. Ensure all call sites handle when it is empty.
70-70: Constructor injection of Optional.
This injection model maintains flexibility when the plagiarism API is omitted.
73-73: Initializing the Optional field.
Assigning the injected value is straightforward; no issues found here.
154-154: Graceful fallback to an empty list.
Returning an empty list if the API is absent avoids errors but conceals missing plagiarism checks. Verify that this silent fallback is intentional.
170-170: Consistent fallback to empty list.
Similar to line 154, ensure that silently skipping plagiarism data is desired. Log or notify if the API is crucial.src/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.java (13)
54-54: New import for ApiNotPresentException.
This facilitates explicit error handling when the plagiarism API is unavailable.
83-85: Plagiarism-related imports introduced.
The new APIs and domain utilities bridge the resource with optional plagiarism functionality.
120-120: Optional field addition.
Makes the resource robust to missing plagiarism result services.
148-148: Optional field addition.
Similarly ensures graceful handling if the detection API is absent.
163-163: Constructor injection for PlagiarismResultApi.
Keeps external API usage decoupled from the resource if absent.
168-168: Extending constructor for PlagiarismDetectionApi.
Maintains consistency with the optional detection approach.
173-173: Assigning the plagiarismResultApi field.
Straightforward assignment; no issues found.
190-190: Assigning the plagiarismDetectionApi field.
Similarly, no concerns—consistent with the constructor parameter.
582-582: Explicit throw if plagiarism result API is absent.
This aligns with a stricter approach than returning an empty fallback, ensuring the calling logic is aware.
586-586: Retrieving the latest plagiarism result.
Casting to TextPlagiarismResult is valid; confirm that this is always the correct type.
587-587: Preparing result for client.
Double-check that the method safely handles null or incomplete data in the result.
607-607: Throwing ApiNotPresentException for detection API.
Consistent with non-silent fallback behavior if the detection API is unavailable.
615-615: Initiating checkTextExercise.
This call encloses detection logic cleanly in the external API.src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java (7)
113-113: PlagiarismCaseApi import.
Indicates the shift away from direct repository usage. No issues noted.
200-200: Defining an Optional field.
Allows safe feature toggling or in-progress integration of plagiarism data.
233-233: Constructor receives Optional.
This design ensures composition of the plagiarism API only when available.
269-269: Assigning the optional API in the constructor.
Matches the plan for a soft dependency on the plagiarism module.
340-340: Early return if API is absent.
Skips the retrieval logic. Confirm that ignoring plagiarism data is the intended fallback.
344-344: Obtaining the present plagiarismCaseApi instance.
Saves repeated optional checking. Good use of get() after isEmpty() guard.
346-346: Finding PlagiarismCase by user & exercise set.
Neatly delegates to the API. Ensure the returned data is adequately validated or sanitized.src/main/java/de/tum/cit/aet/artemis/exam/service/ExamService.java (5)
99-101: Imports align well with modular approach.
These added imports for the new plagiarism API components are consistent with the intended modular design.
171-171: Optional injection for plagiarismCaseApi.
Using Optional is a clean way to conditionally integrate plagiarism handling, preventing NullPointerExceptions if the API is unavailable.
195-201: Constructor update for PlagiarismCaseApi injection.
The updated constructor parameters properly follow constructor injection practices, maintaining clarity and testability. Ensure that call sites either provide a non-empty Optional or intentionally pass Optional.empty() where needed.
222-222: Field assignment consistent with new constructor argument.
Assigning this.plagiarismCaseApi is correctly placed. No further action needed.
353-353: findByExamIdAndStudentId approach mirrors prior logic.
Again, the usage of map(...) and orElse(...) is consistent. This effectively handles absent APIs.src/main/java/de/tum/cit/aet/artemis/plagiarism/exception/ProgrammingLanguageNotSupportedForPlagiarismDetectionException.java (1)
7-7: Constructor visibility change is appropriate.
Making the constructor public allows broader usage of this exception class for unsupported languages. The message is clear and descriptive.src/main/java/de/tum/cit/aet/artemis/core/exception/ApiNotPresentException.java (1)
16-18: LGTM! Clean exception implementation.The implementation is well-structured with:
- Appropriate use of generics with type bounds
- Clear error message formatting
- Good use of constructor injection
src/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismApi.java (1)
14-20: LGTM! Clean API implementation with proper dependency injection.The class follows best practices:
- Uses constructor injection
- Clear separation of concerns
- Proper Spring annotations
src/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismResultApi.java (2)
13-19: LGTM! Clean API implementation with proper dependency injection.The class follows best practices:
- Uses constructor injection
- Clear separation of concerns
- Proper Spring annotations
25-27: Verify null handling in repository method.The method returns null when no results are found. Consider using Optional instead for better null safety.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if the repository method is used with null checks in the codebase rg -A 2 "findFirstWithComparisonsByExerciseIdOrderByLastModifiedDateDescOrNull"Length of output: 4158
Null Handling in PlagiarismResultRepository Confirmed as Intended
The repository method intentionally returns null when no results are found and this behavior is explicitly validated by integration tests (with assertions for both null and non-null outcomes) and consistent usage across API implementations.
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleAccessArchitectureTest.java (3)
3-4: LGTM! Clean import additions for ArchUnit predicates.The new imports enhance test flexibility by enabling class filtering.
28-30: LGTM! Well-structured test method update.The test method now properly excludes ignored classes using the
not(belongToAnyOf())predicate, making the architecture tests more flexible and maintainable.
58-64: LGTM! Clean addition of utility methods.The new methods follow good practices:
getModuleExceptionsSubpackage()provides consistent package naminggetIgnoredClasses()offers a clean extension point for subclassessrc/main/java/de/tum/cit/aet/artemis/plagiarism/service/PlagiarismDetectionService.java (2)
78-89: LGTM! Proper exception handling implementation.The method correctly checks for programming language support before proceeding with plagiarism detection.
126-132: LGTM! Clean exception handling implementation.The method properly validates programming language support and throws a descriptive exception when needed.
src/test/java/de/tum/cit/aet/artemis/plagiarism/PlagiarismDetectionServiceTest.java (1)
22-22: LGTM! Clean import update.The exception import is correctly updated to reflect its new package location.
src/main/java/de/tum/cit/aet/artemis/communication/service/ReactionService.java (2)
44-44: LGTM! Clean dependency injection update.The service now properly uses Optional for flexible dependency injection.
Also applies to: 52-61
138-140: LGTM! Proper API access pattern.The code correctly handles the optional API with appropriate exception handling.
src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseDeletionService.java (4)
35-35: LGTM!The import statement for the new API is correctly placed.
73-73: LGTM!The field declaration and initialization follow Java best practices:
- Using
finalfor immutability- Using
Optionalfor optional dependencies- Consistent naming convention
Also applies to: 100-100
88-89: LGTM!The constructor parameter is correctly updated to use the Optional API.
242-243: Consider adding error logging for plagiarism result deletion.Similar to the previous comment, consider adding error logging here as well.
src/main/java/de/tum/cit/aet/artemis/assessment/web/GradeStepResource.java (3)
36-36: LGTM!The import statement for the new API is correctly placed.
61-61: LGTM!The field declaration and initialization follow Java best practices:
- Using
finalfor immutability- Using
Optionalfor optional dependencies- Consistent naming convention
Also applies to: 81-81, 98-98
66-67: LGTM!The constructor parameter is correctly updated to use the Optional API.
Also applies to: 74-74
src/test/java/de/tum/cit/aet/artemis/plagiarism/ContinuousPlagiarismControlServiceTest.java (1)
37-37: LGTM!The import statement is correctly updated to use the new package structure.
src/main/java/de/tum/cit/aet/artemis/text/web/TextSubmissionResource.java (3)
43-43: LGTM!The import statement for the new API is correctly placed.
81-81: LGTM!The field declaration and initialization follow Java best practices:
- Using
finalfor immutability- Using
Optionalfor optional dependencies- Consistent naming convention
Also applies to: 98-98
87-87: LGTM!The constructor parameter is correctly updated to use the Optional API.
src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseResource.java (4)
57-57: LGTM!The import statement follows Java best practices by being specific and properly ordered.
105-105: LGTM!The field declaration follows best practices:
- Uses
finalfor immutability- Properly uses
Optionalto handle API availability
112-112: LGTM!The constructor parameter follows dependency injection best practices.
128-128: LGTM!The field initialization is consistent with other field initializations in the constructor.
src/main/java/de/tum/cit/aet/artemis/core/service/export/DataExportExerciseCreationService.java (5)
46-46: LGTM!The import statement follows Java best practices by being specific and properly ordered.
81-81: LGTM!The field declaration follows best practices:
- Uses
finalfor immutability- Properly uses
Optionalto handle API availability
96-96: LGTM!The constructor parameter follows dependency injection best practices.
101-101: LGTM!The field initialization is consistent with other field initializations in the constructor.
408-414: LGTM!The code follows best practices for handling Optional:
- Early return if API is not available
- Proper unwrapping of Optional using
get()- Clear and logical flow of operations
src/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismResultApi.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/plagiarism/domain/PlagiarismMapping.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/plagiarism/domain/PlagiarismMapping.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismDetectionApi.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismDetectionApi.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismCaseApi.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismCaseApi.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismPostApi.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/plagiarism/api/PlagiarismPostApi.java
Show resolved
Hide resolved
ole-ve
dismissed stale reviews from HanyangXu0508 and coderabbitai[bot]
via
513d047
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseResource.java (1)
352-352: Consider using method reference for better readability.The lambda expression can be simplified using a method reference.
-plagiarismCaseApi.flatMap(api -> api.getPlagiarismCaseInfoForExerciseAndUser(exercise.getId(), user.getId())).orElse(null); +plagiarismCaseApi.flatMap(api -> api.getPlagiarismCaseInfoForExerciseAndUser(exercise.getId(), user.getId())).orElse(null);src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java (1)
340-347: Consider using method reference for better readability.The code can be simplified by using method reference and extracting the API retrieval to a separate line for better readability.
-if (plagiarismCaseApi.isEmpty()) { - return; -} - -var api = plagiarismCaseApi.get(); -Set<Long> exerciseIds = exercises.stream().map(Exercise::getId).collect(Collectors.toSet()); -List<PlagiarismCase> plagiarismCasesOfUserInCourseExercises = api.findByStudentIdAndExerciseIds(userId, exerciseIds); +Set<Long> exerciseIds = exercises.stream().map(Exercise::getId).collect(Collectors.toSet()); +List<PlagiarismCase> plagiarismCasesOfUserInCourseExercises = plagiarismCaseApi + .map(api -> api.findByStudentIdAndExerciseIds(userId, exerciseIds)) + .orElse(Collections.emptyList());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseDeletionService.java(6 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseResource.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingExerciseResource.java(8 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExercisePlagiarismResource.java(7 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.java(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java
- src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResource.java
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
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/exercise/service/ExerciseDeletionService.javasrc/main/java/de/tum/cit/aet/artemis/core/service/CourseService.javasrc/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.javasrc/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseResource.javasrc/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExercisePlagiarismResource.javasrc/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingExerciseResource.java
⏰ 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 (22)
src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseDeletionService.java (4)
35-35: LGTM!The import follows Java best practices by being specific and avoiding star imports.
73-73: LGTM!The field declaration follows best practices:
- Uses Optional wrapper for optional dependency
- Maintains immutability with final modifier
- Follows least privilege principle with private access
88-89: LGTM!The constructor changes follow dependency injection best practices:
- Uses constructor injection
- Maintains clean parameter ordering
- Properly handles optional dependency
Also applies to: 100-100
183-183: LGTM!The method changes demonstrate proper handling of optional dependencies:
- Uses Optional.ifPresent() for null-safe operations
- Maintains consistent API usage across methods
- Preserves the original functionality while improving modularity
Also applies to: 246-246
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExercisePlagiarismResource.java (3)
8-8: LGTM! Clean import organization.The new imports for Optional, API interfaces, and exceptions are well-organized and follow Java conventions.
Also applies to: 24-24, 32-35
59-61: LGTM! Well-structured dependency injection.The changes demonstrate good practices:
- Constructor injection
- Optional API wrappers for flexible dependency management
- Private field access modifiers
Also applies to: 63-69
81-82: LGTM! Clean API access pattern.The method demonstrates a consistent pattern for API access with proper error handling using Optional.orElseThrow.
Also applies to: 85-86
src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseResource.java (3)
59-59: LGTM!The import statement for the new API is correctly placed.
107-107: LGTM!The field is correctly declared as
Optional<PlagiarismCaseApi>, following Java's best practices for handling optional dependencies.
113-114: LGTM!The constructor parameter and field assignment are correctly implemented, maintaining immutability with the
finalkeyword.Also applies to: 130-130
src/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.java (5)
54-54: LGTM!The import statements for the new APIs and exception are correctly placed and organized.
Also applies to: 83-85
120-120: LGTM!The fields are correctly declared as Optionals, following Java's best practices for handling optional dependencies.
Also applies to: 148-148
163-163: LGTM!The constructor parameters and field assignments are correctly implemented, maintaining immutability with the
finalkeyword.Also applies to: 168-168, 173-173, 190-190
582-583: LGTM!The error handling for missing API is correctly implemented using
ApiNotPresentException.Also applies to: 587-588
607-608: LGTM!The error handling for missing API is consistently implemented across methods.
Also applies to: 615-616
src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java (3)
113-113: LGTM!The import statement for the new API is correctly placed.
200-200: LGTM!The field is correctly declared as
Optional<PlagiarismCaseApi>, following Java's best practices for handling optional dependencies.
233-233: LGTM!The constructor parameter and field assignment are correctly implemented, maintaining immutability with the
finalkeyword.Also applies to: 269-269
src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingExerciseResource.java (4)
41-41: LGTM! Clean import organization.The new imports for plagiarism-related APIs and helpers are well-organized and follow Java conventions.
Also applies to: 67-70
107-107: LGTM! Good use of Optional for dependency injection.The change to Optional wrappers for plagiarism-related dependencies:
- Improves modularity by making these dependencies optional
- Maintains consistency with existing patterns (e.g., competencyProgressApi)
- Follows Java best practices for handling optional dependencies
Also applies to: 117-117
127-127: LGTM! Clean constructor injection implementation.The constructor changes properly handle the Optional wrappers while maintaining clean dependency injection patterns.
Also applies to: 130-130, 136-136, 146-146
418-419: LGTM! Clean API usage implementation.The method implementations properly use the API wrappers and maintain consistent error handling patterns.
Also applies to: 439-440, 446-446
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExercisePlagiarismResource.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingExerciseResource.java
Outdated
Show resolved
Hide resolved
|
|
||
| import de.tum.cit.aet.artemis.core.domain.User; | ||
|
|
||
| public record PlagiarismMapping(Map<Long, Map<Long, PlagiarismCase>> studentIdToExerciseIdToPlagiarismCaseMap) { |
There was a problem hiding this comment.
extracted from src/main/java/de/tum/cit/aet/artemis/plagiarism/service/PlagiarismCaseService.java
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/core/service/export/DataExportExerciseCreationService.java (1)
430-433:⚠️ Potential issueFix potential NullPointerException in post answers check.
The code accesses
plagiarismCase.getPost().getAnswers()without checking ifgetPost()returns null, which could lead to a NullPointerException.Apply this diff to add null check:
- if (!plagiarismCase.getPost().getAnswers().isEmpty()) { + if (plagiarismCase.getPost() != null && !plagiarismCase.getPost().getAnswers().isEmpty()) { headers.add("Plagiarism case replies"); dataStreamBuilder.add(plagiarismCase.getPost().getAnswers().stream().map(AnswerPost::getContent).collect(Collectors.joining("\n"))); }
🧹 Nitpick comments (3)
src/main/java/de/tum/cit/aet/artemis/communication/service/ReactionService.java (1)
138-140: Consider extracting the common error handling pattern.The pattern
plagiarismPostApi.orElseThrow(() -> new ApiNotPresentException(PlagiarismPostApi.class, PROFILE_CORE))is repeated multiple times. Consider extracting it to a private method to improve maintainability and reduce duplication.+ private PlagiarismPostApi getApi() { + return plagiarismPostApi.orElseThrow(() -> new ApiNotPresentException(PlagiarismPostApi.class, PROFILE_CORE)); + }Then use it in the methods:
- PlagiarismPostApi api = plagiarismPostApi.orElseThrow(() -> new ApiNotPresentException(PlagiarismPostApi.class, PROFILE_CORE)); + PlagiarismPostApi api = getApi();Also applies to: 146-148, 162-165, 189-193
src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingExerciseResource.java (2)
413-419: Consider extracting API retrieval logic.The plagiarism result retrieval logic could be more readable by extracting the API retrieval into a helper method.
+ private PlagiarismResultApi getPlagiarismResultApi() { + return plagiarismResultApi.orElseThrow(() -> new ApiNotPresentException(PlagiarismResultApi.class, PROFILE_CORE)); + } + @GetMapping("modeling-exercises/{exerciseId}/plagiarism-result") @EnforceAtLeastEditor public ResponseEntity<PlagiarismResultDTO<ModelingPlagiarismResult>> getPlagiarismResult(@PathVariable long exerciseId) { log.debug("REST request to get the latest plagiarism result for the modeling exercise with id: {}", exerciseId); - PlagiarismResultApi api = plagiarismResultApi.orElseThrow(() -> new ApiNotPresentException(PlagiarismResultApi.class, PROFILE_CORE)); + PlagiarismResultApi api = getPlagiarismResultApi();
438-446: Apply consistent API retrieval pattern.For consistency with the previous suggestion, consider extracting the plagiarism detection API retrieval logic.
+ private PlagiarismDetectionApi getPlagiarismDetectionApi() { + return plagiarismDetectionApi.orElseThrow(() -> new ApiNotPresentException(PlagiarismDetectionApi.class, PROFILE_CORE)); + } + @GetMapping("modeling-exercises/{exerciseId}/check-plagiarism") @FeatureToggle(Feature.PlagiarismChecks) @EnforceAtLeastInstructor public ResponseEntity<PlagiarismResultDTO<ModelingPlagiarismResult>> checkPlagiarism(@PathVariable long exerciseId, @RequestParam int similarityThreshold, @RequestParam int minimumScore, @RequestParam int minimumSize) { - PlagiarismDetectionApi api = plagiarismDetectionApi.orElseThrow(() -> new ApiNotPresentException(PlagiarismDetectionApi.class, PROFILE_CORE)); + PlagiarismDetectionApi api = getPlagiarismDetectionApi();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/de/tum/cit/aet/artemis/communication/service/PostingService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/ReactionService.java(7 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/export/DataExportExerciseCreationService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingExerciseResource.java(8 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExercisePlagiarismResource.java(7 hunks)src/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.java(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/de/tum/cit/aet/artemis/communication/service/PostingService.java
- src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
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/DataExportExerciseCreationService.javasrc/main/java/de/tum/cit/aet/artemis/communication/service/ReactionService.javasrc/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.javasrc/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExercisePlagiarismResource.javasrc/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingExerciseResource.java
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: Analyse
🔇 Additional comments (19)
src/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.java (5)
54-54: LGTM! Clean import additions for API transition.The new imports are correctly organized and specific to the API-based plagiarism handling transition.
Also applies to: 83-85
120-120: LGTM! Well-structured Optional field declarations.The use of Optional for API dependencies follows Java best practices and ensures proper null handling. The private final modifiers align with the least access principle.
Also applies to: 148-148
163-163: LGTM! Clean constructor parameter updates.The constructor properly handles the Optional dependencies and maintains a clear initialization order.
Also applies to: 168-168, 173-173, 190-190
582-583: LGTM! Robust error handling in getPlagiarismResult.The method properly handles API absence and maintains type safety with explicit casting.
Also applies to: 586-587
607-607: LGTM! Clean implementation of checkPlagiarism.The method correctly handles API dependency and maintains the original functionality while transitioning to the API-based approach.
Also applies to: 615-615
src/main/java/de/tum/cit/aet/artemis/core/service/export/DataExportExerciseCreationService.java (3)
46-46: LGTM!The import statement follows the project's conventions by being specific and properly ordered.
81-81: LGTM! Good use of Optional for dependency injection.The changes effectively transition from direct repository dependency to an optional API dependency, aligning with the server modularization effort. The use of
Optionalappropriately handles cases where the API might not be available.Also applies to: 96-96, 101-101
408-413: LGTM! Clean integration of the optional API.The implementation properly handles the case when the API is not available and maintains the same functionality using the new API.
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExercisePlagiarismResource.java (4)
8-8: LGTM! Well-organized imports.The new imports are properly organized and follow Java conventions, avoiding star imports. They align well with the modularization effort by introducing the new API interfaces and related components.
Also applies to: 24-24, 32-34, 37-37
58-60: LGTM! Well-structured dependency injection.The transition to Optional API dependencies is well-implemented:
- Follows constructor injection pattern
- Uses Optional wrappers appropriately for API dependencies
- Maintains clean separation of concerns
Also applies to: 62-68
80-80: Fix inconsistent API class in exception message.The exception message uses
PlagiarismApi.classbut should usePlagiarismDetectionApi.classto match the actual API being accessed.Also applies to: 105-105, 139-139
78-87: LGTM! Clean API integration.The methods have been successfully refactored to use the new API interfaces:
- Proper error handling for missing APIs
- Consistent pattern across all methods
- Maintains existing functionality while improving modularity
Also applies to: 103-123, 137-164
src/main/java/de/tum/cit/aet/artemis/communication/service/ReactionService.java (4)
23-27: LGTM!The new imports are correctly organized and necessary for the API-based approach.
44-44: LGTM!The field declaration follows best practices:
- Uses Optional for optional dependency
- Marked as final for immutability
- Follows single responsibility principle by consolidating plagiarism-related functionality
52-61: LGTM!The constructor follows dependency injection best practices:
- Uses constructor injection
- All dependencies are final
- Parameters are logically ordered
207-208: LGTM!The broadcast logic is correctly delegated to the API, following the single responsibility principle and improving modularity.
src/main/java/de/tum/cit/aet/artemis/modeling/web/ModelingExerciseResource.java (3)
41-41: LGTM! Clean import organization.The new imports for plagiarism APIs and helpers are well-organized and follow Java conventions.
Also applies to: 67-70
106-106: LGTM! Well-structured field declarations.The transition from concrete implementations to
Optional<API>aligns with dependency injection best practices and the modularization goal. The use ofOptionalappropriately handles the potential absence of API implementations.Also applies to: 116-116
126-126: LGTM! Constructor properly updated.The constructor parameter updates and field initializations are consistent with the new API-based approach.
Also applies to: 129-129, 135-135, 145-145
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 the plagiarism module.
Steps for Testing
Perform for modelling-/text-/and programming exercises
Exam Mode Testing
Same steps, but with an exam.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Summary by CodeRabbit
Summary by CodeRabbit