Development: Introduce plagiarism module API
#10296
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 10 minutes and 40 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)
WalkthroughThis pull request refactors the handling of plagiarism across the codebase. It systematically replaces direct repository and service dependencies (e.g., Changes
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: 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
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
Basically try to test all the functionality of the plagiarism module.
Exam Mode Testing
Same procedure, test everything related to plagiarism cases in exams.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Summary by CodeRabbit
New Features
Refactor