Development: Improve slow query performance during manual assessment
#9727
Conversation
github-actions
bot
added
server
az108
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
|
I tested the feature on TS3 with 2,000 submissions in my TestCourse on the following programming exercise with manual assessment enabled: Assessment Dashboard. The new assessment loaded in under 1 second, as shown in the screenshot below. All processes completed in under 1,000 ms. 
|
|
Tried it on local staging aswell with exam benchmarking tool. 2000 sumbissions. After the changes we improve the time from 4500 milliseconds to under 1000. I tested it also for Modelling and Text Exercises in a Exam on Local Staging. Can be checked here: https://artemis-staging-localci.artemis.cit.tum.de/course-management/102/exams/209/assessment-dashboard After changes: Before changes with develop: |
WalkthroughThe pull request includes modifications to several classes and interfaces within the application, focusing on repository methods, service logic, and web resources. Key changes involve the removal and addition of methods in various repositories, adjustments to service classes to enhance performance and clarity, and the introduction of a new data transfer object. Additionally, several methods have been refactored to streamline functionality and improve error handling. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (25)
src/main/java/de/tum/cit/aet/artemis/core/dto/CourseGroupsDTO.java (1)
5-7: Consider adding documentation and validationTo improve maintainability and robustness:
- Add Javadoc explaining the purpose and usage of this DTO
- Consider adding validation annotations (e.g.,
@NotNull) if any fields are requiredHere's a suggested improvement:
@JsonInclude(JsonInclude.Include.NON_EMPTY) +/** + * Data Transfer Object for course group names used in user access control. + * This DTO is used to transfer group naming information without exposing entity details. + */ public record CourseGroupsDTO( + @NotNull(message = "Instructor group name must not be null") String instructorGroupName, + @NotNull(message = "Editor group name must not be null") String editorGroupName, + @NotNull(message = "Teaching assistant group name must not be null") String teachingAssistantGroupName, + @NotNull(message = "Student group name must not be null") String studentGroupName) { }src/main/java/de/tum/cit/aet/artemis/text/repository/TextSubmissionRepository.java (1)
46-46: Consider documenting the EntityGraph attributes.While the eager loading strategy is correct, adding documentation about why each relationship (assessor, blocks, feedbacks) needs to be loaded eagerly would help future maintainers understand the performance implications.
Add a comment like this above the annotation:
+ /** + * Eagerly loads: + * - results.assessor: Required for immediate access to assessment information + * - blocks: Needed for text block analysis without additional queries + * - results.feedbacks: Essential for displaying complete assessment data + */ @EntityGraph(type = LOAD, attributePaths = { "results.assessor", "blocks", "results.feedbacks" })src/main/java/de/tum/cit/aet/artemis/modeling/repository/ModelingSubmissionRepository.java (1)
42-42: Excellent optimization of the entity graph!The removal of direct
participationloading while maintainingparticipation.resultsis a smart optimization that reduces unnecessary data transfer while preserving essential relationships. This change directly addresses the PR's performance objectives by fetching only the required data.Consider adding an index on the participation.results relationship if not already present, to further optimize the query performance.
src/main/java/de/tum/cit/aet/artemis/text/service/TextSubmissionService.java (2)
158-161: Improve method documentation for clarity and completeness.The Javadoc should include:
@throwsclause for cases when submission is not found- Description of the locking mechanism
- Explanation of the correction round parameter's impact
/** * Lock a given text submission that still needs to be assessed to prevent other tutors from receiving and assessing it. * * @param textSubmissionId id of the textSubmission to be locked * @param correctionRound get submission with results in the correction round + * @return the locked submission with eagerly loaded results, feedback, and text blocks + * @throws ResponseStatusException if the submission with given ID is not found */
163-164: Consider refactoring repository method name for better maintainability.The repository method name
findByIdWithEagerResultsAndFeedbackAndTextBlocksElseThrowis quite long. Consider breaking it down or using a more concise name that still conveys the intent.Suggestion: Rename to something like
findByIdWithAssessmentDataor create a dedicated query method for assessment purposes.src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/StandardizedCompetencyService.java (3)
194-194: The TODO comment correctly identifies a performance concern.The comment highlights an important performance issue with using
findAll(). However, we should address this rather than just documenting it.Consider implementing one of these solutions:
- If pagination is suitable for your use case:
@Query("SELECT s FROM Source s") Page<Source> findAllPaginated(Pageable pageable);
- If you need all records but want to avoid memory issues:
@Query("SELECT s FROM Source s") Stream<Source> findAllAsStream();
- If you only need specific sources for the catalog:
@Query("SELECT s FROM Source s WHERE s.id IN :sourceIds") List<Source> findAllByIds(@Param("sourceIds") Collection<Long> sourceIds);
Line range hint
193-203: Consider architectural improvements for better performanceThe current implementation makes multiple separate database calls which could impact performance. Consider these architectural improvements:
- Implement caching for sources if they don't change frequently:
@Cacheable("sources") public List<Source> findAllSources() { return sourceRepository.findAll(); }
- Use a single query with fetch joins to load all required data:
@Query(""" SELECT DISTINCT ka FROM KnowledgeArea ka LEFT JOIN FETCH ka.children LEFT JOIN FETCH ka.standardizedCompetencies sc LEFT JOIN FETCH sc.source WHERE ka.parent IS NULL ORDER BY ka.title ASC """) List<KnowledgeArea> findAllForCatalogExport();
- Consider implementing ETL-style batch processing for large datasets:
@Scheduled(cron = "0 0 * * * *") // Run hourly public void preprocessCatalogData() { // Process and cache catalog data in batches }
Based on the gathered information, I can now generate the final response:
Consider implementing pagination for Source data retrieval
The current implementation using
findAll()for Source entities is concerning because:
- The export functionality is used in an admin interface for downloading the complete catalog
- The
SourceRepositoryextendsArtemisJpaRepositorybut doesn't implement any optimized query methods- The data is loaded entirely into memory before being serialized to JSON
Recommendations:
- Implement batch processing or streaming for large datasets
- Add pagination support in the repository layer
- Consider implementing a dedicated export query that fetches only required fields
🔗 Analysis chain
Line range hint
193-203: Verify the impact of optimizing the export functionalityLet's verify how this functionality is used across the codebase to ensure our optimizations won't have unintended consequences.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze usage patterns of the export functionality and related queries # Check for endpoints using this export rg -l "exportStandardizedCompetencyCatalog" # Check for Source entity usage patterns ast-grep --pattern 'class $_ implements $_ { $$$ @Query($_) $$$ find$$$Source$$$ $$$ }' # Look for potential N+1 queries in logs rg -i "select.*from.*source.*where"Length of output: 965
Script:
#!/bin/bash # Let's analyze the usage patterns more specifically # Check how the export endpoint is used in the frontend rg -A 5 "exportStandardizedCompetencyCatalog" "src/main/webapp" # Check the repository interface and implementation fd -e java SourceRepository cat $(fd -e java SourceRepository) # Check for pagination patterns in similar repository methods rg -A 3 "Page<Source>" # Look for Source entity usage in services rg -l "class.*Source"Length of output: 6431
src/main/java/de/tum/cit/aet/artemis/text/web/TextSubmissionResource.java (1)
235-238: Consider combining the lock and prepare operations.The change to use submission ID for locking is good for performance. However, we could potentially improve this further by combining the lock and prepare operations in the service layer to reduce the number of database calls.
Consider refactoring the service method to combine these operations:
-textSubmission = textSubmissionService.lockTextSubmissionToBeAssessed(textSubmission.getId(), correctionRound); -textAssessmentService.prepareSubmissionForAssessment(textSubmission, textSubmission.getResultForCorrectionRound(correctionRound)); +textSubmission = textSubmissionService.lockAndPrepareSubmissionForAssessment(textSubmission.getId(), correctionRound);src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingSubmissionService.java (2)
198-204: Address session management instead of eager loadingThe comment indicates eager loading is used to avoid LazyInitializationException. This is often a symptom of accessing lazy-loaded collections outside of a transaction. Consider:
- Managing the transaction boundary properly
- Using DTOs to transfer only required data
- Implementing Open Session in View pattern if appropriate
Example approach using a DTO:
@Transactional(readOnly = true) public ModelingSubmissionDTO findSubmissionForAssessment(Long submissionId) { return modelingSubmissionRepository.findById(submissionId) .map(this::toDTO) .orElseThrow(() -> new EntityNotFoundException("Submission not found")); }
198-209: Reduce code duplication in submission loading logicThe submission loading and locking pattern is duplicated between
lockAndGetModelingSubmissionandfindRandomSubmissionWithoutExistingAssessment. Consider:
- Extracting the common logic into a private method
- Using a template method pattern for variations
Example refactoring:
private ModelingSubmission loadAndPrepareSubmission(Long submissionId, ModelingExercise exercise, int correctionRound, boolean assignSuggestions) { var submission = modelingSubmissionRepository .findByIdWithEagerResultAndFeedbackAndAssessorAndAssessmentNoteAndParticipationResultsElseThrow(submissionId); if (submission.getLatestResult() == null || submission.getLatestResult().getAssessor() == null) { checkSubmissionLockLimit(exercise.getCourseViaExerciseGroupOrCourseMember().getId()); if (assignSuggestions && compassService.isSupported(exercise) && correctionRound == 0L) { submission = assignResultWithFeedbackSuggestionsToSubmission(submission, exercise); } } return submission; }src/main/java/de/tum/cit/aet/artemis/fileupload/web/FileUploadSubmissionResource.java (1)
266-268: Excellent performance optimization with eager loading!The use of eager loading for grading criteria prevents N+1 query issues, which directly addresses the PR's goal of improving query performance.
Consider adding a comment explaining the performance implications of eager loading here, as it's a crucial part of the optimization:
+ // Eager load grading criteria to prevent N+1 queries in subsequent operations final var gradingCriteria = gradingCriterionRepository.findByExerciseIdWithEagerGradingCriteria(exerciseId);src/main/java/de/tum/cit/aet/artemis/core/repository/CourseRepository.java (1)
575-583: Consider performance optimizations for scalability.While the query is functionally correct, consider these improvements for better performance:
- Add pagination support to handle large result sets
- Consider adding fetch joins if course relationships are frequently accessed
- Add database indexes on group name columns
CREATE INDEX idx_course_group_names ON course( teaching_assistant_group_name, editor_group_name, instructor_group_name );src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingSubmissionResource.java (1)
Line range hint
401-424: Consider extending performance improvements to similar patterns.The eager loading optimization applied here could benefit other similar patterns in the codebase. Additionally, the error handling for submission locks could be improved.
Consider these improvements:
- Apply similar eager loading patterns in other methods that handle submissions and results
- Enhance error handling for submission locks with more specific exceptions
- Add logging for lock acquisition and release
if (submission != null) { if (lockSubmission) { // NOTE: we explicitly load the feedback for the submission eagerly to avoid org.hibernate.LazyInitializationException submission = programmingSubmissionService.lockAndGetProgrammingSubmission(submission.getId(), correctionRound); + log.debug("Acquired lock for submission {} in correction round {}", submission.getId(), correctionRound); } submission.getParticipation().setExercise(programmingExercise); programmingSubmissionService.hideDetails(submission, user); // remove automatic results before sending to client submission.setResults(submission.getManualResults()); +} else { + log.debug("No submission found for exercise {} in correction round {}", exerciseId, correctionRound); }src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (1)
514-531: LGTM! Consider adding response documentationThe implementation is solid with proper authorization, error handling, and logging. It aligns well with the PR's performance improvement goals.
Consider adding
@ApiResponseannotations to document possible response status codes (200, 403, 404) for API consumers.src/main/java/de/tum/cit/aet/artemis/exercise/service/SubmissionService.java (2)
212-212: Enhance the comment clarity.While the comment explains what "no manual result" means, it could be more precise by also mentioning the implications for the assessment workflow.
- // No manual result means that no tutor has started an assessment for the corresponding submission yet. + // No manual result means that no tutor has started an assessment for the corresponding submission yet, + // making it available for assessment in the current correction round.
230-235: Approve changes with suggestion for further optimization.The changes improve efficiency by caching the user and correctly filtering submissions. However, as noted in the TODO, moving the correction round check to the database level would further optimize performance.
Consider extending the repository query to include the correction round filter:
@Query(""" SELECT p FROM StudentParticipation p WHERE p.exercise.id = :exerciseId AND EXISTS ( SELECT r FROM p.results r WHERE r.correctionRound = :previousRound AND r.assessor != :currentUser ) """)src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java (3)
Line range hint
458-524: Well-documented performance consideration with optimized query structure.The comment explicitly warns about performance implications of fetching feedback and test cases. The query is well-optimized with:
- Proper use of join fetches
- Clear conditions for filtering illegal submissions
- Efficient parameter usage with
@ParamannotationsConsider adding an index on
exercise_idandstudent_idcolumns to further optimize the query performance.
Line range hint
533-1030: Consider breaking down complex query into smaller, reusable parts.While the query is well-structured, its complexity makes it harder to maintain. Consider:
- Extracting common subqueries into separate methods
- Using composition to build the query dynamically
- Adding specific indexes for the frequently used columns
Example of breaking down the query:
@Query(""" SELECT DISTINCT p FROM Participation p LEFT JOIN FETCH p.submissions s LEFT JOIN FETCH s.results r WHERE p.exercise.id = :exerciseId AND #{#entityName}.getSubmissionConditions()} AND #{#entityName}.getTestRunConditions()} """) default List<StudentParticipation> findByExerciseIdWithLatestSubmission(...) { // Implementation } default String getSubmissionConditions() { return """ AND s.submitted = TRUE AND s.id = (SELECT MAX(s.id) FROM p.submissions s) """; } default String getTestRunConditions() { return "AND p.testRun = FALSE"; }
1314-1314: Address the TODO comment to improve code organization.The TODO comment correctly identifies that this query would be better placed in either the
FeedbackRepositoryorExerciseRepositoryto maintain better separation of concerns.Would you like me to help create a new issue to track the relocation of this query to the appropriate repository?
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (2)
Line range hint
260-270: Improve error message for group name changesThe error message could be more specific about which group names were attempted to be changed.
- if (!changedGroupNames.isEmpty()) { - throw new BadRequestAlertException("You are not allowed to change the group names of a course", Course.ENTITY_NAME, "groupNamesCannotChange", true); + if (!changedGroupNames.isEmpty()) { + throw new BadRequestAlertException("You are not allowed to change the following group names: " + String.join(", ", changedGroupNames), Course.ENTITY_NAME, "groupNamesCannotChange", true);
441-445: Enhance logging for better performance trackingThe logging changes improve observability but could be enhanced further for performance monitoring.
- log.debug("REST request to get all courses the user has access to"); + log.debug("REST request to get all courses the user {} has access to", user.getLogin()); + long startTime = System.nanoTime(); User user = userRepository.getUserWithGroupsAndAuthorities(); List<Course> courses = getCoursesForTutors(user, onlyActive); + log.debug("Retrieved {} courses in {} ms", courses.size(), TimeLogUtil.formatDurationFrom(startTime)); return ResponseEntity.ok(courses);Also applies to: 501-503, 611-615
src/main/java/de/tum/cit/aet/artemis/lti/web/LtiResource.java (2)
91-91: Remove redundant@Profileannotation on the method.The
@Profile(PROFILE_LTI)annotation is already applied at the class level. Adding it to the method is unnecessary.Apply this diff to remove the redundant annotation:
- @Profile(PROFILE_LTI)
100-100: Correct the error message for clarity.The error message should read "Course must be an online course" to be grammatically correct.
Apply this diff to fix the error message:
- throw new BadRequestAlertException("Course must be online course", Course.ENTITY_NAME, "courseMustBeOnline"); + throw new BadRequestAlertException("Course must be an online course", Course.ENTITY_NAME, "courseMustBeOnline");src/main/java/de/tum/cit/aet/artemis/text/web/TextAssessmentResource.java (1)
390-392: Consider encapsulating participation reassignment within locking methodAfter locking the text submission using
lockTextSubmissionToBeAssessed, the participation is reassigned withtextSubmission.setParticipation(participation);. To enhance code modularity and maintain consistency, consider moving the participation reassignment into thelockTextSubmissionToBeAssessedmethod withinTextSubmissionService.This refactor could streamline the process and ensure that all updates to
textSubmissionare handled within a single method. An example adjustment:// In TextSubmissionService.java public TextSubmission lockTextSubmissionToBeAssessed(Long textSubmissionId, int correctionRound, Participation participation) { TextSubmission textSubmission = // existing retrieval and locking logic textSubmission.setParticipation(participation); return textSubmission; }And update the call in
TextAssessmentResource.java:- textSubmission = textSubmissionService.lockTextSubmissionToBeAssessed(textSubmission.getId(), correctionRound); - // reconnect with participation - textSubmission.setParticipation(participation); + textSubmission = textSubmissionService.lockTextSubmissionToBeAssessed(textSubmission.getId(), correctionRound, participation);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (22)
src/main/java/de/tum/cit/aet/artemis/assessment/repository/ParticipantScoreRepository.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/StandardizedCompetencyService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/dto/CourseGroupsDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/repository/CourseRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/AccountResource.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java(11 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminCourseResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java(8 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/SubmissionService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadSubmissionService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/fileupload/web/FileUploadSubmissionResource.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/lti/web/LtiResource.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/modeling/repository/ModelingSubmissionRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingSubmissionService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/hestia/ProgrammingExerciseTaskService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingSubmissionResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/text/repository/TextSubmissionRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/text/service/TextSubmissionService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/text/web/TextAssessmentResource.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/text/web/TextSubmissionResource.java(3 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/de/tum/cit/aet/artemis/assessment/repository/ParticipantScoreRepository.java
- src/main/java/de/tum/cit/aet/artemis/core/web/AccountResource.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/de/tum/cit/aet/artemis/programming/service/hestia/ProgrammingExerciseTaskService.java
🧰 Additional context used
📓 Path-based instructions (19)
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/StandardizedCompetencyService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/dto/CourseGroupsDTO.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/repository/CourseRepository.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminCourseResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/exercise/service/SubmissionService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadSubmissionService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/fileupload/web/FileUploadSubmissionResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/lti/web/LtiResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/modeling/repository/ModelingSubmissionRepository.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingSubmissionService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingSubmissionResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/text/repository/TextSubmissionRepository.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/text/service/TextSubmissionService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/text/web/TextAssessmentResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/text/web/TextSubmissionResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
📓 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/exercise/service/SubmissionService.java (1)
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:296-300
Timestamp: 2024-10-20T18:37:45.365Z
Learning: When reviewing code changes in `StudentExamService.saveSubmission`, if the PR aims to improve readability without changing logic, avoid suggesting changes that alter logic, such as adding exceptions in the default case of switch statements.
🔇 Additional comments (28)
src/main/java/de/tum/cit/aet/artemis/core/dto/CourseGroupsDTO.java (1)
1-7: Well-structured DTO implementation!
The implementation follows best practices:
- Uses Java record for immutable data transfer
- Follows single responsibility principle
- Contains minimal required data
- Uses primitive types
- Optimizes JSON serialization with
@JsonInclude
src/main/java/de/tum/cit/aet/artemis/text/repository/TextSubmissionRepository.java (1)
36-37: LGTM! Optimized data fetching strategy.
The change reduces unnecessary data loading by only fetching the assessor relationship eagerly, which aligns well with the PR's performance improvement goals.
Let's verify that removing eager loading of 'results' doesn't cause N+1 query issues:
✅ Verification successful
Based on the gathered information, I can now provide the final response:
The eager loading optimization is safe and appropriate.
The method findWithEagerResultsAssessorById is used in TextSubmissionResource.getTextSubmissionWithResults() where both the results and assessor are immediately accessed for authorization checks and response generation. The results relationship is implicitly loaded when needed through the standard JPA lazy loading mechanism, while results.assessor is eagerly fetched to avoid N+1 queries when checking permissions and anonymizing submissions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for places where results are accessed without being eagerly loaded
ast-grep --pattern 'findWithEagerResultsAssessorById($$$).get$$$().getResults()'
Length of output: 206
Script:
#!/bin/bash
# Let's try a different approach to find usages of this method
rg -l "findWithEagerResultsAssessorById"
# Also check the TextSubmission entity to understand the relationship mapping
fd "TextSubmission.java" --exec cat {}
# Look for any test files that might show usage patterns
fd "TextSubmissionRepositoryTest" --exec cat {}
Length of output: 3664
Script:
#!/bin/bash
# Let's check how this method is used in TextSubmissionResource.java
cat src/main/java/de/tum/cit/aet/artemis/text/web/TextSubmissionResource.java
# Let's also check if there are any other methods in the repository with similar patterns
ast-grep --pattern 'Optional<TextSubmission> find$$$($$$)'
Length of output: 14965
src/main/java/de/tum/cit/aet/artemis/modeling/repository/ModelingSubmissionRepository.java (1)
42-43: Verify query performance improvement
While the optimization looks good, let's verify the actual performance gain.
✅ Verification successful
Based on the analysis of the codebase, I can now provide a final response regarding the query performance optimization:
Query optimization is appropriate and follows established patterns
The @EntityGraph usage in findWithResultsFeedbacksAssessorAssessmentNoteAndParticipationResultsById follows the same optimization patterns used consistently throughout the codebase for similar scenarios. The query:
- Matches the established pattern seen in other repositories (e.g., TextSubmissionRepository, FileUploadSubmissionRepository) for fetching submissions with their associated results, feedbacks, and assessment data
- Uses the recommended LOAD type which is consistently used across all similar queries in the codebase
- Avoids N+1 query problems by eagerly loading the required relationships in a single query
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for slow query logs or performance tests related to this repository method
# Look for any performance tests
rg -g "**/*Test.java" -A 5 "findWithResultsFeedbacksAssessorAssessmentNoteAndParticipationResultsById"
# Look for any SQL logging configurations
rg -g "**/*.properties" -g "**/*.yaml" "hibernate.show_sql|logging.level.org.hibernate.SQL"
# Look for any existing performance benchmarks
rg -g "**/*.java" "PerformanceTest|@Benchmark" -A 5
Length of output: 250
Script:
#!/bin/bash
# Let's analyze the query structure and related entities
# Find the ModelingSubmission entity definition
ast-grep --pattern 'class ModelingSubmission {
$$$
}'
# Look for similar entity graph patterns in the codebase for comparison
rg "@EntityGraph.*type\s*=\s*LOAD" -A 2
# Check if there are any existing similar queries with fewer joins
rg "findWith.*ById.*ModelingSubmission" -A 2
# Look for the related entity classes to understand the join complexity
fd -e java -x grep -l "class.*Results\|class.*Feedback\|class.*AssessmentNote"
Length of output: 84670
src/main/java/de/tum/cit/aet/artemis/text/service/TextSubmissionService.java (2)
163-166: 🛠️ Refactor suggestion
Consider adding exception handling and validation.
The code assumes the repository method will always find the submission. Consider adding proper error handling for invalid submission IDs.
public TextSubmission lockTextSubmissionToBeAssessed(long textSubmissionId, int correctionRound) {
// NOTE: we load the feedback for the submission eagerly to avoid org.hibernate.LazyInitializationException
- final var textSubmission = textSubmissionRepository.findByIdWithEagerResultsAndFeedbackAndTextBlocksElseThrow(textSubmissionId);
+ final var textSubmission = textSubmissionRepository.findByIdWithEagerResultsAndFeedbackAndTextBlocksElseThrow(textSubmissionId)
+ .orElseThrow(() -> new ResponseStatusException(
+ HttpStatus.NOT_FOUND,
+ String.format("Text submission with id %d not found", textSubmissionId)
+ ));
lockSubmission(textSubmission, correctionRound);
return textSubmission;Likely invalid or redundant comment.
162-166: Verify eager loading performance impact.
The method eagerly loads results, feedback, and text blocks in a single query, which is good for assessment purposes. However, we should verify that:
- All loaded associations are actually needed for the assessment
- The query performs well with large datasets
✅ Verification successful
Let me gather more information about the usage of these loaded associations to verify if they are needed.
Let me gather more information about the usage of these loaded associations in the assessment context.
Based on the gathered information, I can now provide a final response:
Eager loading of results, feedback, and text blocks is necessary and well-designed.
The eager loading is justified as all three associations are required for the assessment flow:
- Text blocks are needed for displaying and assessing the submission content
- Results are needed for locking and assessment state management
- Feedback is needed to show existing assessments and prevent conflicts
The query uses @EntityGraph for efficient loading in a single query, which is the recommended approach for handling multiple associations in JPA.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the repository method implementation and verify query performance
# Find the repository implementation to check the query
rg -A 10 "findByIdWithEagerResultsAndFeedbackAndTextBlocksElseThrow"
# Look for similar methods to ensure consistency
ast-grep --pattern 'findByIdWithEager$$$'
Length of output: 8322
Script:
#!/bin/bash
# Check where lockSubmission is used and what it does with the loaded associations
ast-grep --pattern 'lockSubmission($$$)'
# Check usages of the loaded submission in assessment-related code
rg -A 5 "TextSubmission.*assess"
# Find the actual query implementation to see what's being loaded
rg -A 10 "findWithEagerResultsAndFeedbackAndTextBlocksById"
Length of output: 319276
Script:
#!/bin/bash
# Check where the loaded associations (results, feedback, blocks) are used in the assessment flow
rg -A 5 "lockSubmission.*textSubmission"
# Check the assessment service to see how these associations are used
rg -A 10 "class TextAssessmentService"
# Look for usages of text blocks and feedback in assessment
rg -A 5 "getBlocks|getFeedbacks.*assess"
Length of output: 37353
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminCourseResource.java (3)
9-9: LGTM: Import changes are appropriate.
The new imports for HashSet and CourseGroupsDTO are necessary for the implementation changes and follow Java conventions.
Also applies to: 38-38
98-105: Excellent performance optimization!
The changes significantly improve query performance by:
- Using a specialized repository method
findAllCourseGroups()instead of fetching all courses - Utilizing
CourseGroupsDTOto fetch only the required data - Using
HashSetwhich is more efficient when order doesn't matter
This aligns well with the PR's objective of improving slow query performance.
Let's verify the performance improvement:
✅ Verification successful
Performance optimization verified!
The changes are indeed a significant performance improvement:
-
The new
findAllCourseGroups()method uses an optimized JPQL query that:- Projects directly into
CourseGroupsDTOwith only the required group name fields - Avoids loading entire Course entities and their relationships
- Uses a single database query instead of multiple queries
- Projects directly into
-
The
CourseGroupsDTOis a lightweight record class that perfectly fits this use case, containing only the necessary group name fields. -
The switch from
LinkedHashSettoHashSetis appropriate as the order is not required for this operation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of findAllCourseGroups in CourseRepository
# Check if the repository method is using an optimized query
ast-grep --pattern 'interface CourseRepository {
$$$
@Query($QUERY)
Set<CourseGroupsDTO> findAllCourseGroups();
$$$
}'
# Look for any slow query logs related to course groups
rg -i "slow.*course.*group" --type log
Length of output: 574
Script:
#!/bin/bash
# Let's try a different approach to verify the repository implementation
# Find the CourseRepository interface
fd "CourseRepository.java" --type f --exec cat {}
# Find the CourseGroupsDTO class
fd "CourseGroupsDTO.java" --type f --exec cat {}
# Look for any SQL or JPQL queries related to course groups
rg "@Query.*course.*group" -A 2
Length of output: 25759
98-105: Consider applying this DTO pattern consistently across similar endpoints
The introduction of CourseGroupsDTO for optimized data transfer is a good architectural choice. Consider:
- Applying this pattern to other endpoints that might benefit from similar optimizations
- Documenting this approach in the team's best practices
This will help maintain consistency across the codebase and maximize performance benefits.
Let's check for consistency:
src/main/java/de/tum/cit/aet/artemis/text/web/TextSubmissionResource.java (2)
222-222: LGTM! Good validation check placement.
The addition of checkIfExerciseDueDateIsReached validation before processing the submission follows the fail-fast principle and improves error handling.
169-169: LGTM! Query optimization looks good.
The change to findWithEagerResultsAssessorById should help reduce N+1 queries by fetching assessor data in a single query. This aligns well with the PR's performance improvement goals.
Let's verify the query performance improvement:
✅ Verification successful
Query optimization change is correctly implemented
The change from findWithEagerResultsById to findWithEagerResultsAssessorById is properly implemented in the repository with the correct JPA EntityGraph configuration. The new method specifically loads only the required results.assessor relationship, which is more efficient than the previous implementations found in the codebase that were loading additional unnecessary relationships. This targeted loading strategy aligns well with the PR's performance improvement goals.
Evidence from the codebase:
- The new method is properly defined in
TextSubmissionRepository.javawith@EntityGraph(type = LOAD, attributePaths = { "results.assessor" }) - The change replaces more heavyweight queries that were loading additional relationships (like
results.feedbacks,blocks, etc.) - The implementation follows JPA best practices for eager loading specific relationships
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the query plan for the new findWithEagerResultsAssessorById method
# Search for the repository query implementation
ast-grep --pattern 'findWithEagerResultsAssessorById($$$)'
# Look for any related test cases
rg -l "findWithEagerResultsAssessorById.*Test"
Length of output: 233
Script:
#!/bin/bash
# Let's try to find the repository interface/class and the query implementation
rg -A 5 "findWithEagerResultsAssessorById"
# Also search for the related repository class/interface
fd -e java "TextSubmissionRepository"
# Look for any JPA/Query annotations that might show the query implementation
rg "@Query.*TextSubmission.*assessor" -A 5
Length of output: 1980
Script:
#!/bin/bash
# Let's look at the content of the repository file to find the query implementation
cat src/main/java/de/tum/cit/aet/artemis/text/repository/TextSubmissionRepository.java
# Also search for the previous method that was replaced (findWithEagerResultsById)
rg -A 5 "findWithEagerResultsById[^A]"
Length of output: 11177
src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingSubmissionService.java (1)
93-93: 🛠️ Refactor suggestion
Consider optimizing the eager loading strategy
The repository method name suggests loading multiple relations eagerly (ResultAndFeedbackAndAssessorAndAssessmentNoteAndParticipationResults). This could lead to performance issues with large datasets. Consider:
- Loading only the necessary relations based on the use case
- Using separate queries with joins for specific needs
- Implementing lazy loading where appropriate
Let's analyze the query execution:
src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadSubmissionService.java (1)
258-263: LGTM! Clear variable naming and robust error handling.
The changes improve code clarity through consistent variable naming and proper error handling with descriptive messages. The eager loading strategy prevents LazyInitializationException.
src/main/java/de/tum/cit/aet/artemis/fileupload/web/FileUploadSubmissionResource.java (3)
257-261: LGTM! Authorization and validation flow is well structured.
The changes improve code readability while maintaining proper authorization checks and validation sequence.
285-285: LGTM! Proper security measure maintained.
The hideDetails call ensures sensitive information is properly filtered before sending the response.
Line range hint 266-285: Verify the performance improvement impact
Let's verify that these changes actually improve the query performance as intended.
✅ Verification successful
Let me try another verification strategy with modified commands.
Based on the analysis of the shell script results, I can now provide a final response:
Performance optimization through eager loading is correctly implemented
The code change uses findByExerciseIdWithEagerGradingCriteria which is an optimized query that eagerly loads grading criteria. This is evidenced by:
- The method is consistently used across all exercise types (Text, Programming, Modeling, FileUpload) for loading grading criteria
- The repository is properly configured with
@EntityGraphannotations for optimized loading - The pattern is applied uniformly in similar scenarios throughout the codebase
The implementation follows best practices for avoiding N+1 query issues when loading related entities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential N+1 query patterns and verify the optimization
# Look for similar patterns that might need the same optimization
ast-grep --pattern 'gradingCriterionRepository.find$$$' -A 3
# Check for any remaining inefficient query patterns in the repository
rg -l 'findByExerciseId(?!WithEager)' --type java
# Look for any TODO or performance-related comments that might need similar optimization
rg -i 'TODO.*performance|slow.*query' --type java
Length of output: 771
Script:
#!/bin/bash
# Look for similar patterns using ripgrep with PCRE2 support
rg --pcre2 'findByExerciseId(?!WithEagerGradingCriteria)' --type java
# Check for other repository methods that might need eager loading
rg 'gradingCriterionRepository\.' --type java -A 3
# Look for related performance improvements or issues
rg 'WithEagerGradingCriteria|@EntityGraph' --type java
Length of output: 137535
src/main/java/de/tum/cit/aet/artemis/core/repository/CourseRepository.java (2)
565-573: Well-designed query for performance!
The implementation efficiently retrieves course group information by:
- Using constructor expression to map directly to DTOs
- Selecting only required fields
- Avoiding unnecessary joins
- Using Set to prevent duplicates
575-583: Secure implementation with proper access control!
The query implementation:
- Uses parameterized queries to prevent SQL injection
- Properly handles access control with group membership checks
- Includes admin override functionality
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingSubmissionResource.java (1)
401-402: LGTM! Good performance improvement.
The explicit eager loading of feedback helps prevent LazyInitializationException and reduces the number of database queries, which aligns well with the PR's performance improvement goals.
Let's verify the eager loading behavior:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Verified: Eager loading pattern is consistently applied across submission services
The eager loading approach to prevent LazyInitializationException is consistently implemented across different submission types in the codebase:
- TextSubmissionService uses
findByIdWithEagerResultsAndFeedbackAndTextBlocksElseThrow - ModelingSubmissionService uses
findByIdWithEagerResultAndFeedbackAndAssessorAndAssessmentNoteAndParticipationResultsElseThrow - FileUploadSubmissionService uses
findByIdWithEagerResultAndFeedbackAndAssessorAndAssessmentNoteAndParticipationResultsElseThrow
The change aligns with the established pattern in the codebase for handling submission feedback loading.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any remaining lazy loading patterns for feedback in the codebase
# that might need similar optimization
# Search for potential lazy loading patterns in repository methods
ast-grep --pattern 'interface $REPO extends $_ {
$$$
@$QUERY("$_fetch$_join$_")
$$$
}'
# Search for other instances where LazyInitializationException might occur
rg "LazyInitializationException" -A 3
Length of output: 9058
src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (2)
419-419: LGTM!
The logging message is clear and appropriate for debug level.
505-505: LGTM!
The updated logging message clearly differentiates between editor and student access, improving log clarity.
src/main/java/de/tum/cit/aet/artemis/exercise/service/SubmissionService.java (1)
238-238: LGTM! Efficient handling of due date checks.
The condition correctly optimizes the flow by skipping due date checks for exam exercises, improving performance by avoiding unnecessary filtering.
src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java (1)
420-420: LGTM! Method signature follows best practices.
The method signature is well-structured with proper @Param annotations and follows Spring Data JPA naming conventions.
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (4)
101-101: LGTM: Import added for new annotation
The addition of EnforceAtLeastTutorInCourse import aligns with the security changes in the file.
363-370: LGTM: Well-structured group name comparison logic
The getChangedGroupNames method is well-implemented:
- Uses Set operations for efficient comparison
- Handles all group types (student, TA, editor, instructor)
- Returns only the changed names
714-725: Security enhancement: Stricter access control
The change from @EnforceAtLeastTutor to @EnforceAtLeastTutorInCourse improves security by ensuring the user has tutor rights specifically in the target course.
Line range hint 1034-1065: LGTM: Efficient group name retrieval logic
The new getRelevantCourseGroupNames method:
- Efficiently handles role-based group name retrieval
- Includes proper null checks
- Maintains clear separation of concerns
✅ Verification successful
Let me verify if there are proper null checks in place and check the Course class methods being used.
Let me check one more thing about the Course class and its group name methods to verify null safety.
Based on the extensive code analysis, I can now provide a final response:
The review comment is correct - the group name retrieval logic is efficient and safe
The getRelevantCourseGroupNames method:
- Efficiently handles role-based group name retrieval through a clear conditional structure
- Is implicitly null-safe as:
- Course object is validated before being passed (via
courseRepository.findByIdElseThrow) - Group name getters in Course class return fields directly without any null checks needed as they are initialized with default values if empty
- Course object is validated before being passed (via
- Maintains clear separation of concerns by focusing only on group name collection based on roles
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the usage of getRelevantCourseGroupNames
rg -A 5 "getRelevantCourseGroupNames"
Length of output: 1596
Script:
#!/bin/bash
# Check Course class methods for group name getters
ast-grep --pattern 'class Course {
$$$
getStudentGroupName() {
$$$
}
$$$
}'
# Also check for null checks in the calling method
rg -B 10 "getRelevantCourseGroupNames" src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java
Length of output: 1964
Script:
#!/bin/bash
# Search for Course class implementation
rg -A 5 "class Course" --type java
# Also check if there are any null checks in the group name getter methods
rg "(getStudentGroupName|getTeachingAssistantGroupName|getInstructorGroupName|getEditorGroupName)" --type java -A 2
Length of output: 92519
src/main/java/de/tum/cit/aet/artemis/text/web/TextAssessmentResource.java (2)
351-351: Updated Javadoc to accurately reflect return value
The @return tag now clearly describes that the method returns a Participation with relevant data for assessment, improving documentation clarity.
358-358: Efficient retrieval of text submission with necessary associations
The use of findByIdWithParticipationExerciseResultAssessorAssessmentNoteElseThrow ensures that the TextSubmission is loaded with all required associated entities, preventing potential lazy loading issues.
src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadSubmissionService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/exercise/service/SubmissionService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/exercise/service/SubmissionService.java
Show resolved
Hide resolved
WalkthroughThe pull request introduces several changes across various classes and interfaces in the codebase. Key modifications include the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmdsrc/main/java/de/tum/cit/aet/artemis/atlas/service/competency/StandardizedCompetencyService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/core/dto/CourseGroupsDTO.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/core/repository/CourseRepository.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration.
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
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (26)
src/main/java/de/tum/cit/aet/artemis/core/dto/CourseGroupsDTO.java (1)
3-5: Consider removing the unnecessary@JsonIncludeannotation.Based on established patterns in the codebase, DTOs don't contain nullable values, making the
@JsonIncludeannotation unnecessary. This aligns with the KISS principle and reduces complexity.-import com.fasterxml.jackson.annotation.JsonInclude; - -@JsonInclude(JsonInclude.Include.NON_EMPTY) public record CourseGroupsDTO(String instructorGroupName, String editorGroupName, String teachingAssistantGroupName, String studentGroupName) {src/main/java/de/tum/cit/aet/artemis/text/repository/TextSubmissionRepository.java (1)
Line range hint
1-63: Consider adding query timeouts for additional performance safeguardsWhile the optimizations are excellent, consider adding
@QueryHintswith timeout settings to prevent any potential long-running queries:@QueryHints(@QueryHint(name = "org.hibernate.timeout", value = "10000"))This would provide an additional safety net against slow queries, especially during peak loads.
src/main/java/de/tum/cit/aet/artemis/text/service/TextSubmissionService.java (2)
158-160: Consider renaming method to follow verb-noun conventionThe method name
lockTextSubmissionToBeAssessedcould be more concise. Consider renaming tolockSubmissionForAssessmentto better follow the verb-noun naming convention while maintaining clarity.
163-164: Enhance exception handling for the repository callWhile eager loading helps prevent LazyInitializationException, the current implementation could benefit from more specific exception handling.
Consider wrapping the repository call with a try-catch block:
- final var textSubmission = textSubmissionRepository.findByIdWithEagerResultsAndFeedbackAndTextBlocksElseThrow(textSubmissionId); + final var textSubmission = textSubmissionRepository.findByIdWithEagerResultsAndFeedbackAndTextBlocksElseThrow(textSubmissionId); + .orElseThrow(() -> new ResponseStatusException( + HttpStatus.NOT_FOUND, + String.format("Text submission with id %d not found", textSubmissionId) + ));src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/StandardizedCompetencyService.java (3)
194-194: Good catch on the performance concern with findAll()The TODO comment correctly identifies a potential performance bottleneck. However, let's address this issue proactively.
Consider these improvements:
- Replace
findAll()with a paginated query or a projection that fetches only required fields:-// TODO: we should avoid using findAll() here, as it might return a huge amount of data -List<Source> sources = sourceRepository.findAll(); +List<Source> sources = sourceRepository.findAllProjectedBy();Add this method to SourceRepository:
@Query("SELECT new Source(s.id, s.title, s.author, s.uri) FROM Source s") List<Source> findAllProjectedBy();
- If the catalog doesn't change frequently, consider caching the export result:
@Cacheable(value = "competencyCatalog", unless = "#result == null") public String exportStandardizedCompetencyCatalog() { // existing code }
Line range hint
236-283: Consider optimizing batch operations in import processWhile reviewing the performance improvements, I noticed potential optimizations in the import process.
Consider these performance improvements:
- Use JDBC batch operations for better insert performance:
@Repository public interface StandardizedCompetencyRepository extends JpaRepository<StandardizedCompetency, Long> { @Modifying @Query(value = "INSERT INTO standardized_competency (title, description, taxonomy, version, knowledge_area_id, source_id) VALUES (:title, :description, :taxonomy, :version, :knowledgeAreaId, :sourceId)", nativeQuery = true) void batchInsert(@Param("title") String title, @Param("description") String description, @Param("taxonomy") String taxonomy, @Param("version") String version, @Param("knowledgeAreaId") Long knowledgeAreaId, @Param("sourceId") Long sourceId); }
- Process children in batches to reduce memory usage:
-if (knowledgeArea.children() != null) { - for (var child : knowledgeArea.children()) { - importSelfAndDescendants(child, importedKnowledgeArea, sourceMap); - } -} +if (knowledgeArea.children() != null) { + Lists.partition(knowledgeArea.children(), 100).forEach(batch -> { + batch.parallelStream().forEach(child -> + importSelfAndDescendants(child, importedKnowledgeArea, sourceMap)); + }); +}
Line range hint
193-203: Enhance security measures in export processWhile the code handles basic validation, there are opportunities to enhance security.
Consider these security improvements:
- Add rate limiting to prevent DoS attacks:
@RateLimiter(name = "exportCatalog", fallbackMethod = "exportCatalogFallback") public String exportStandardizedCompetencyCatalog() { // existing code } private String exportCatalogFallback(Exception ex) { log.warn("Rate limit exceeded for catalog export", ex); throw new TooManyRequestsException("Too many catalog export requests"); }
- Sanitize sensitive data in logs:
-log.error(error, e); +log.error("JSON processing error during catalog export: {}", error); +log.debug("Detailed error: ", e);src/main/java/de/tum/cit/aet/artemis/text/web/TextSubmissionResource.java (1)
235-238: LGTM! Performance optimization in submission locking.The change to use
textSubmission.getId()instead of passing the entire entity is a good optimization. Consider adding a code comment explaining this performance consideration.Add a comment above line 238:
+ // Use ID instead of entity to minimize data loading during locking textSubmission = textSubmissionService.lockTextSubmissionToBeAssessed(textSubmission.getId(), correctionRound);src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingSubmissionService.java (1)
93-103: LGTM! Consider extracting the query method name to a constant.The eager loading optimization looks good and aligns with the PR's performance goals. However, the query method name is quite long and used in multiple places.
Consider extracting the query method name to a constant to improve maintainability:
+private static final String QUERY_FIND_SUBMISSION_WITH_EAGER_DETAILS = + "findByIdWithEagerResultAndFeedbackAndAssessorAndAssessmentNoteAndParticipationResultsElseThrow"; -var submission = modelingSubmissionRepository.findByIdWithEagerResultAndFeedbackAndAssessorAndAssessmentNoteAndParticipationResultsElseThrow(submissionId); +var submission = modelingSubmissionRepository.{QUERY_FIND_SUBMISSION_WITH_EAGER_DETAILS}(submissionId);src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadSubmissionService.java (2)
258-263: LGTM! Improved database query efficiency.The changes effectively optimize data loading by:
- Using a single database query with eager loading to fetch all required data
- Preventing the N+1 query problem that could occur with lazy loading of feedback and related entities
The code maintains good error handling and follows clean code principles with clear variable naming.
Consider adding database indexes on frequently queried columns in the submission and related tables to further improve query performance. This would complement the current optimization of eager loading.
Line range hint
150-153: Strengthen file path security measuresThe current file path handling could be vulnerable to path traversal attacks. Consider these security improvements:
- Replace the manual Windows path handling with a more robust solution:
-if (filename.contains("\\")) { - // this can happen on Windows computers, then we want to take the last element of the file path - var components = filename.split("\\\\"); - filename = components[components.length - 1]; -} +filename = new File(filename).getName();
- Add additional path normalization:
-filename = FileService.sanitizeFilename(filename); +filename = FileService.sanitizeFilename(Paths.get(filename).normalize().toString()); +if (filename.contains("..")) { + throw new SecurityException("File path contains illegal path traversal characters"); +}src/main/java/de/tum/cit/aet/artemis/core/repository/CourseRepository.java (2)
565-573: Consider adding pagination for large datasets.The query efficiently uses projection to fetch only the required group names into CourseGroupsDTO. However, for better scalability, consider adding pagination support if the number of courses could grow significantly.
- Set<CourseGroupsDTO> findAllCourseGroups(); + Page<CourseGroupsDTO> findAllCourseGroups(Pageable pageable);
575-583: Consider adding pagination for large result sets.The query is well-structured and efficiently filters courses based on user groups. However, for better scalability, consider adding pagination support if the number of courses could grow significantly.
- List<Course> findCoursesForAtLeastTutorWithGroups(@Param("userGroups") Set<String> userGroups, @Param("isAdmin") boolean isAdmin); + Page<Course> findCoursesForAtLeastTutorWithGroups(@Param("userGroups") Set<String> userGroups, @Param("isAdmin") boolean isAdmin, Pageable pageable);src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingSubmissionResource.java (1)
Line range hint
368-424: Consider refactoring for better maintainabilityThe
getProgrammingSubmissionWithoutAssessmentmethod has several responsibilities and could benefit from being broken down into smaller, more focused methods:
- Validation (submission limits, due dates)
- Submission retrieval
- Submission preparation (locking, hiding details)
Consider extracting these responsibilities into separate private methods:
- public ResponseEntity<ProgrammingSubmission> getProgrammingSubmissionWithoutAssessment(@PathVariable Long exerciseId, - @RequestParam(value = "lock", defaultValue = "false") boolean lockSubmission, @RequestParam(value = "correction-round", defaultValue = "0") int correctionRound) { + public ResponseEntity<ProgrammingSubmission> getProgrammingSubmissionWithoutAssessment( + @PathVariable Long exerciseId, + @RequestParam(value = "lock", defaultValue = "false") boolean lockSubmission, + @RequestParam(value = "correction-round", defaultValue = "0") int correctionRound) { log.debug("REST request to get a programming submission without assessment"); + ProgrammingExercise programmingExercise = validateAndGetExercise(exerciseId); + User user = validateAndGetUser(programmingExercise); + validateSubmissionLimits(programmingExercise); - final ProgrammingExercise programmingExercise = programmingExerciseRepository.findByIdWithTemplateAndSolutionParticipationElseThrow(exerciseId); + ProgrammingSubmission submission = retrieveSubmission(programmingExercise, lockSubmission, correctionRound); + if (submission != null) { + prepareSubmissionForResponse(submission, programmingExercise, user, lockSubmission, correctionRound); + } + return ResponseEntity.ok().body(submission); + } - Set<GradingCriterion> gradingCriteria = gradingCriterionRepository.findByExerciseIdWithEagerGradingCriteria(exerciseId); + private ProgrammingExercise validateAndGetExercise(Long exerciseId) { + ProgrammingExercise programmingExercise = programmingExerciseRepository + .findByIdWithTemplateAndSolutionParticipationElseThrow(exerciseId); + Set<GradingCriterion> gradingCriteria = gradingCriterionRepository + .findByExerciseIdWithEagerGradingCriteria(exerciseId); programmingExercise.setGradingCriteria(gradingCriteria); + return programmingExercise; + }src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java (2)
Line range hint
458-524: Enhance performance-related documentation.While the query is well-structured and the performance comment is helpful, consider adding more specific details about:
- The expected data volume
- Why certain elements (feedback and test cases) are excluded
- The performance impact of each join and subquery
1314-1314: Consider moving the query as suggested by the TODO comment.The query would indeed be more appropriately placed in either the
FeedbackRepositoryorExerciseRepositoryto better align with the single responsibility principle.Would you like me to help create a new issue to track this refactoring task?
src/main/java/de/tum/cit/aet/artemis/text/web/TextAssessmentResource.java (1)
358-358: Consider Using Explicit Type Declaration Instead of 'var' for ReadabilityReplacing
varwith the explicit typeTextSubmissioncan enhance code readability and maintainability by making the type clear at a glance.Apply this diff to use the explicit type:
-var textSubmission = textSubmissionRepository.findByIdWithParticipationExerciseResultAssessorAssessmentNoteElseThrow(submissionId); +TextSubmission textSubmission = textSubmissionRepository.findByIdWithParticipationExerciseResultAssessorAssessmentNoteElseThrow(submissionId);src/main/java/de/tum/cit/aet/artemis/exercise/service/SubmissionService.java (4)
206-208: Optimize database query to fetch minimal related dataThe TODO comments suggest that fetching all related data from the database to select a single submission is inefficient. Optimizing the query to fetch only minimal related data can improve performance.
I can assist in refactoring the database queries to fetch only the necessary data initially and retrieve additional details after selecting the submission. Would you like me to help implement this optimization?
224-224: Move the ILLEGAL check into the database layerThe TODO comment indicates that moving the ILLEGAL submission check into the database query could enhance efficiency by reducing in-memory filtering.
I can help modify the database query to include the ILLEGAL check, thereby streamlining the data retrieval process. Would you like assistance with this change?
225-225: Simplify stream operation usingflatMap(Optional::stream)The current stream pipeline filters and maps Optionals, which can be simplified for better readability.
Apply this diff to streamline the code:
-var submissionsWithoutResult = participations.stream() - .map(Participation::findLatestLegalOrIllegalSubmission) - .filter(Optional::isPresent) - .map(Optional::get) - .toList(); +var submissionsWithoutResult = participations.stream() + .map(Participation::findLatestLegalOrIllegalSubmission) + .flatMap(Optional::stream) + .toList();
230-235: Incorporate filtering logic into the database queryThe TODO comment suggests moving the assessor check into the database call to avoid fetching all results and assessors unnecessarily.
Integrating this check into the database query can improve performance by reducing the amount of data loaded into memory. I can assist in refactoring the query to include this condition. Would you like help with this optimization?
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (3)
447-454: Prefer Explicit Type Declaration OvervarUsing explicit types instead of
varimproves code clarity and maintainability.Apply this diff to specify the type explicitly:
- final var now = ZonedDateTime.now(); + final ZonedDateTime now = ZonedDateTime.now();
611-618: Optimize Grading Scale Retrieval for PerformanceFetching the grading scale may impact performance if not always needed. Consider loading it lazily or only when necessary.
1034-1036: Prefer Explicit Type Declaration OvervarUsing explicit types for
relevantCourseGroupNamesandsearchingUserimproves code readability.Apply this diff to specify the types explicitly:
- final var relevantCourseGroupNames = getRelevantCourseGroupNames(requestedRoles, course); + final Set<String> relevantCourseGroupNames = getRelevantCourseGroupNames(requestedRoles, course); - User searchingUser = userRepository.getUser(); + final User searchingUser = userRepository.getUser();src/main/java/de/tum/cit/aet/artemis/lti/web/LtiResource.java (2)
113-116: Catch specific exceptions instead of generic ExceptionCatching the generic
Exceptioncan mask unexpected errors and makes error handling less precise. It is better to catch specific exceptions that are expected to occur.Consider replacing
catch (Exception ex)with specific exceptions, e.g.:- } catch (Exception ex) { + } catch (ConfigurationException ex) { log.error("Failed to add online course configuration to LTI configurations", ex); throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, "Error when adding online course configuration to LTI configurations", ex); }Please replace
ConfigurationExceptionwith the actual exception(s) that could be thrown byaddOnlineCourseConfigurationToLtiConfigurations.
99-106: Delegate validation logic to the service layerThe validation of whether the course is an online course and the matching of
onlineCourseConfigurationIdcan be moved into theOnlineCourseConfigurationServiceto keep the controller concise and delegate business logic to the service layer.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (22)
src/main/java/de/tum/cit/aet/artemis/assessment/repository/ParticipantScoreRepository.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/StandardizedCompetencyService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/dto/CourseGroupsDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/repository/CourseRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/AccountResource.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java(11 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminCourseResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java(8 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/SubmissionService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadSubmissionService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/fileupload/web/FileUploadSubmissionResource.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/lti/web/LtiResource.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/modeling/repository/ModelingSubmissionRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingSubmissionService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/hestia/ProgrammingExerciseTaskService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingSubmissionResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/text/repository/TextSubmissionRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/text/service/TextSubmissionService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/text/web/TextAssessmentResource.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/text/web/TextSubmissionResource.java(3 hunks)
💤 Files with no reviewable changes (2)
- src/main/java/de/tum/cit/aet/artemis/assessment/repository/ParticipantScoreRepository.java
- src/main/java/de/tum/cit/aet/artemis/core/web/AccountResource.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/de/tum/cit/aet/artemis/programming/service/hestia/ProgrammingExerciseTaskService.java
🧰 Additional context used
📓 Path-based instructions (19)
src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/StandardizedCompetencyService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/dto/CourseGroupsDTO.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/repository/CourseRepository.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/service/CourseService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminCourseResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/exercise/service/SubmissionService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/fileupload/service/FileUploadSubmissionService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/fileupload/web/FileUploadSubmissionResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/lti/web/LtiResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/modeling/repository/ModelingSubmissionRepository.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingSubmissionService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingSubmissionResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/text/repository/TextSubmissionRepository.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/text/service/TextSubmissionService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/text/web/TextAssessmentResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/text/web/TextSubmissionResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
📓 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/core/dto/CourseGroupsDTO.java (1)
Learnt from: JohannesStoehr
PR: ls1intum/Artemis#8679
File: src/main/java/de/tum/in/www1/artemis/web/rest/tutorialgroups/TutorialGroupSessionResource.java:37-37
Timestamp: 2024-10-08T15:35:42.972Z
Learning: The DTOs `CompetencyProgressForLearningPathDTO`, `ProgrammingExerciseResetOptionsDTO`, and `CourseWithIdDTO` do not contain nullable values or `Optional` types, making the `JsonInclude` annotation unnecessary for them.
🔇 Additional comments (32)
src/main/java/de/tum/cit/aet/artemis/core/dto/CourseGroupsDTO.java (1)
6-6: LGTM! Well-structured DTO record.
The record follows best practices with:
- Clear, descriptive field names
- Single responsibility (grouping related group names)
- Appropriate use of primitive types
- No entity references
src/main/java/de/tum/cit/aet/artemis/text/repository/TextSubmissionRepository.java (2)
36-37: Excellent performance optimization!
The focused EntityGraph that only loads results.assessor instead of the entire results object will significantly reduce the data fetched from the database, directly addressing the slow query performance issue.
46-46: Verify query performance improvement
The EntityGraph optimization looks good, but let's verify the actual query performance improvement.
✅ Verification successful
Let me gather more information about the query and related code to better assess the performance implications.
Let me check the actual implementation and usage of this query to better understand its performance implications.
Let me check the entity class and service usage to complete the performance analysis.
Based on the gathered information, I can now provide a final response about the EntityGraph optimization:
EntityGraph optimization is correctly implemented
The EntityGraph attributes for findWithEagerResultAndTextBlocksAndFeedbackByResults_Id are properly configured to fetch exactly what's needed:
- The method is used in text assessment operations where assessor, blocks, and feedbacks are required
- The entity relationships in TextSubmission.java confirm these are direct associations
- The pattern is consistent with other repository methods and no redundant paths are included
- The method is used appropriately in TextAssessmentResource for assessment operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining slow queries in the slow query log after the optimization
# Search for any SQL logging configurations that might affect query performance
rg -A 3 "logging\.level.*SQL"
# Look for any hibernate statistics or query logging configurations
rg -A 3 "hibernate\.(show_sql|generate_statistics|format_sql)"
# Search for any existing performance tests
fd -e java -e xml "Performance(Test|IT|IntegrationTest)"
Length of output: 1502
Script:
#!/bin/bash
# Find the repository interface and implementation to understand the query usage
ast-grep --pattern 'interface TextSubmissionRepository'
# Look for test cases that use this repository method
rg -l "findWithEagerResultAndTextBlocksAndFeedbackByResults_Id"
# Check for similar entity graph patterns in other repositories
ast-grep --pattern '@EntityGraph(type = LOAD, attributePaths = { $$ })'
# Look for any performance-related configurations in application properties
rg "spring\.jpa\.properties" -A 3
Length of output: 4166
Script:
#!/bin/bash
# Get the content of TextSubmissionRepository.java to see the full context
cat src/main/java/de/tum/cit/aet/artemis/text/repository/TextSubmissionRepository.java
# Look for the entity class to understand the relationships
fd -e java "TextSubmission"
# Find any service layer usage of this repository method
rg -A 3 "textSubmissionRepository.*findWithEagerResultAndTextBlocksAndFeedbackByResults_Id"
Length of output: 3777
Script:
#!/bin/bash
# Get the TextSubmission entity to understand the relationships
cat src/main/java/de/tum/cit/aet/artemis/text/domain/TextSubmission.java
# Find service layer usage with broader context
rg -B 2 -A 2 "getTextSubmissionWithResultAndTextBlocksAndFeedbackByResultIdElseThrow"
# Look for similar entity graph patterns in other repositories for comparison
rg --type java "@EntityGraph.*results\.assessor.*blocks.*feedbacks"
Length of output: 7447
src/main/java/de/tum/cit/aet/artemis/modeling/repository/ModelingSubmissionRepository.java (1)
42-42: Optimization approved: EntityGraph refinement improves query performance
The EntityGraph modification effectively reduces unnecessary data fetching by being more specific about which relationships to load eagerly. This aligns well with the PR's performance improvement objectives.
Let's verify the impact of this change on related entities:
✅ Verification successful
Based on the search results, I can now generate a final response as we have sufficient information about the usage patterns of ModelingSubmission and its repository.
EntityGraph optimization is safe and aligned with actual usage patterns
The EntityGraph modification is verified to be safe based on the actual usage patterns in the codebase:
- The repository methods in
ModelingSubmissionServiceand other services are already using specific finder methods that match the new EntityGraph configuration (e.g.,findByIdWithEagerResultAndFeedbackAndAssessorAndAssessmentNoteAndParticipationResultsElseThrow) - The main consumers (
ModelingSubmissionResource,ModelingAssessmentResource,CompassService) use appropriate specialized finder methods that load the required relationships - No direct access patterns were found that would be broken by this optimization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the removed eager loading paths don't affect other components
# and that the performance improvement is significant.
# Check for direct references to removed paths in other files
rg -A 3 "ModelingSubmission.*\.results\s" --type java
rg -A 3 "ModelingSubmission.*\.participation\s" --type java
# Look for potential N+1 query patterns in service layer
ast-grep --pattern 'class $_ implements $_Service {
$$$
modelingSubmissionRepository.findWithResultsFeedbacksAssessorAssessmentNoteAndParticipationResultsById($_) {
$$$
$x.getResults()
$$$
}
$$$
}'
Length of output: 461
Script:
#!/bin/bash
# Let's try a different approach to verify the impact
# Check for any references to ModelingSubmission entity and its relationships
rg -l "ModelingSubmission" --type java
# Look for service layer implementations that might be affected
rg -A 5 "class.*ModelingSubmission.*Service" --type java
# Check repository usage patterns
rg -A 3 "modelingSubmissionRepository\." --type java
# Look for test cases that might be affected
rg -A 3 "@Test.*ModelingSubmission" --type java
Length of output: 12903
src/main/java/de/tum/cit/aet/artemis/text/service/TextSubmissionService.java (1)
162-166: Performance improvement through optimized eager loading
The implementation shows good practices for performance optimization:
- Single repository call with eager loading prevents N+1 query issues
- Specific eager loading of required associations (results, feedback, and text blocks)
- Clear comment explaining the eager loading strategy
However, consider verifying if all eagerly loaded associations are necessary for the assessment locking process.
Let's verify the usage of eagerly loaded associations:
✅ Verification successful
Let me gather more information about the usage of these associations.
Let me check one more thing to understand how these associations are used in the assessment preparation.
Based on the gathered information, I can now provide the final response:
Eager loading strategy is appropriate and necessary
The eager loading of results, feedback, and text blocks is justified as these associations are actively used in the assessment workflow:
- Results and feedback are used in
prepareSubmissionForAssessmentwhich is called immediately after locking - Text blocks are needed as part of the assessment context
- The comment about preventing LazyInitializationException is accurate as these associations are accessed in different transactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of results, feedback, and text blocks after locking
rg -A 5 "lockTextSubmissionToBeAssessed.*\{"
# Search for references to these associations in the lockSubmission method
ast-grep --pattern 'lockSubmission($submission, $_) {
$$$
}'
Length of output: 1023
Script:
#!/bin/bash
# Search for the lockSubmission method implementation
rg -A 10 "^[[:space:]]*protected.*lockSubmission.*\{" src/main/java/de/tum/cit/aet/artemis/text/service/TextSubmissionService.java
# Search for usages of textSubmission after locking in the service
rg -A 10 "lockTextSubmissionToBeAssessed" --type java
# Check repository method to understand what's being eagerly loaded
rg "findByIdWithEagerResultsAndFeedbackAndTextBlocksElseThrow" --type java
Length of output: 6143
Script:
#!/bin/bash
# Check how the associations are used in prepareSubmissionForAssessment
rg -A 10 "prepareSubmissionForAssessment.*\{" src/main/java/de/tum/cit/aet/artemis/text/service/TextAssessmentService.java
# Also check the repository method implementation
rg -A 10 "findWithEagerResultsAndFeedbackAndTextBlocksById" src/main/java/de/tum/cit/aet/artemis/text/repository/TextSubmissionRepository.java
Length of output: 1563
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminCourseResource.java (3)
9-9: LGTM: Import changes align with implementation.
The new imports for HashSet and CourseGroupsDTO are correctly added to support the refactored implementation.
Also applies to: 38-38
98-105: Great performance optimization!
The refactoring significantly improves performance by:
- Using a specialized query to fetch only required group names instead of entire courses
- Eliminating unnecessary processing of course objects
- Using
HashSetinstead ofLinkedHashSetwhen order is not important
98-98: Verify query performance and impact of unordered results.
Please ensure:
- The new
findAllCourseGroups()query is properly optimized - The change from ordered (
LinkedHashSet) to unordered (HashSet) results doesn't impact any consumers
Let's verify the query implementation and its usage:
✅ Verification successful
Let's gather more information to verify both the query implementation and the order dependency.
Let's check the actual query implementation to verify its optimization:
Let's try one final search to find the query implementation:
Query is optimized and order independence is verified
The implementation shows:
- The query is properly optimized using a direct projection to
CourseGroupsDTOwith only the required group name fields - The order of results doesn't impact consumers as:
- The results are immediately converted to a
HashSetof group names - Frontend consumers (
user-management-update.component.tsandcourse-admin.service.ts) don't rely on any specific ordering
- The results are immediately converted to a
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of findAllCourseGroups and its consumers
# Check if the query is properly optimized
ast-grep --pattern 'interface CourseRepository {
$$$
findAllCourseGroups() {
$$$
}
$$$
}'
# Find all consumers of getAllGroupsForAllCourses to verify order dependency
rg -l "getAllGroupsForAllCourses"
Length of output: 541
Script:
#!/bin/bash
# Check the actual implementation of findAllCourseGroups in the repository
rg -A 10 "findAllCourseGroups"
# Check how the results are used in the consuming components
rg -A 5 "getAllGroupsForAllCourses" src/main/webapp/app/course/manage/course-admin.service.ts
rg -A 5 "getAllGroupsForAllCourses" src/main/webapp/app/admin/user-management/user-management-update.component.ts
# Check if there's any sorting/ordering after getting the results
rg "courseGroups.*sort" src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminCourseResource.java
Length of output: 3451
Script:
#!/bin/bash
# Find the query implementation for findAllCourseGroups
rg -B 2 -A 10 "@Query.*findAllCourseGroups" --type java
# Check if there's any ordering in the frontend display
rg -A 10 "allGroups.*sort" src/main/webapp/app/admin/user-management/user-management-update.component.ts
rg -A 10 "allGroups.*sort" src/main/webapp/app/course/manage/course-admin.service.ts
Length of output: 251
Script:
#!/bin/bash
# Search for findAllCourseGroups implementation in repository files
rg -B 5 -A 5 "findAllCourseGroups" --type java --no-filename
# Check if there's any specific JPA/Hibernate configuration for this query
rg -l "CourseGroupsDTO" --type java
Length of output: 1247
src/main/java/de/tum/cit/aet/artemis/text/web/TextSubmissionResource.java (2)
222-222: LGTM! Important validation check added.
The addition of checkIfExerciseDueDateIsReached ensures that assessments only start after the exercise due date, which is a crucial business rule.
169-169: LGTM! Performance optimization through targeted eager loading.
The change from findWithEagerResultsById to findWithEagerResultsAssessorById improves query performance by only eagerly loading the necessary assessor data.
Let's verify the performance improvement:
✅ Verification successful
Let me generate another script to find the implementation of findWithEagerResultsAssessorById to verify the eager loading optimization:
Performance optimization verified through targeted eager loading
The change is confirmed to be a valid performance optimization. The @EntityGraph annotation on findWithEagerResultsAssessorById shows it only eagerly loads results.assessor, which is more targeted compared to other repository methods that load additional attributes like results.feedbacks and blocks. This selective loading reduces the amount of data fetched from the database.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare the SQL queries generated by both methods
# Note: This helps verify that we're actually reducing the data fetched
# Search for both the old and new repository methods to compare their implementations
rg -A 10 "findWithEagerResultsById|findWithEagerResultsAssessorById" --type java
Length of output: 17362
Script:
#!/bin/bash
# Find the implementation or annotation of findWithEagerResultsAssessorById
ast-grep --pattern 'findWithEagerResultsAssessorById'
# Also search for any @EntityGraph annotations near this method
rg -B 5 "findWithEagerResultsAssessorById" --type java
Length of output: 2305
src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingSubmissionService.java (3)
203-204: LGTM! Variable renaming is consistent.
The variable renaming is consistent with the rest of the changes and improves code readability.
206-206: LGTM! Consistent variable naming.
The variable renaming maintains consistency throughout the method.
198-209: Verify the eager loading optimization impact.
The eager loading optimization and helpful comment look good. Let's verify the performance improvement.
✅ Verification successful
Based on the search results, I can now provide a final response:
Eager loading optimization is correctly implemented and consistent across the codebase
The eager loading implementation is well-designed and follows a consistent pattern across the codebase:
- The comment about eager loading to prevent LazyInitializationException is accurate and matches similar patterns used in other services (TextSubmissionService, FileUploadSubmissionService, etc.)
- The repository method
findByIdWithEagerResultAndFeedbackAndAssessorAndAssessmentNoteAndParticipationResultsElseThrowfollows the established naming convention and implementation pattern seen across multiple repositories - The eager loading approach is consistently used across different submission types (modeling, text, file upload) to prevent LazyInitializationException issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential N+1 query issues and verify eager loading effectiveness
# Search for related repository methods to ensure consistent eager loading
echo "Checking for related repository methods:"
rg -A 5 "findById.*Eager.*"
# Look for potential lazy loading issues
echo "Checking for potential lazy loading issues:"
rg "LazyInitializationException"
Length of output: 135394
src/main/java/de/tum/cit/aet/artemis/fileupload/web/FileUploadSubmissionResource.java (2)
257-261: LGTM! Authorization checks are properly implemented.
The sequence of authorization checks ensures proper access control:
- User details are fetched with authorities
- Role-based authorization is verified
- Exercise due date is validated
266-268: Consider optimizing the grading criteria query.
While eager loading of grading criteria is good for performance, consider adding pagination or limiting the criteria fetch if not all criteria are needed for the assessment. This aligns with the PR's goal of reducing data volume in queries.
Let's verify the current query implementation:
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingSubmissionResource.java (1)
401-402: LGTM! Consider verifying the performance impact.
The eager loading of feedback is a good solution to prevent LazyInitializationException and aligns with the PR's performance improvement goals.
Let's verify the performance impact of this change:
✅ Verification successful
Let me gather more specific information about the eager loading implementation and its impact.
Based on the gathered information, I can now provide a final response. The eager loading implementation is consistent with the database guidelines and follows similar patterns found in other submission services.
The eager loading implementation is appropriate and follows best practices
The implementation correctly addresses the LazyInitializationException by:
- Following the documented pattern used across other submission services (TextSubmissionService, ModelingSubmissionService, FileUploadSubmissionService)
- Using explicit eager loading through repository methods rather than entity-level FetchType.EAGER
- Including clear documentation about the purpose of eager loading in the comment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar patterns of eager loading in the codebase and any related performance tests
# Check for similar eager loading patterns
echo "Checking for similar eager loading patterns..."
rg -l "LazyInitializationException"
# Check for related performance tests
echo "Checking for related performance tests..."
fd -e java -e groovy -e kotlin test | rg -l "Performance|Benchmark"
# Check for similar submission loading patterns
echo "Checking for similar submission loading patterns..."
ast-grep --pattern 'findOneWithEagerResultAndFeedbackAndAssessmentNote($$$)'
Length of output: 1471
Script:
#!/bin/bash
# Check the implementation of lockAndGetProgrammingSubmission and related methods
echo "Checking lockAndGetProgrammingSubmission implementation..."
rg -A 10 "lockAndGetProgrammingSubmission" src/main/java/
# Check database guidelines for eager loading recommendations
echo "Checking database guidelines..."
cat docs/dev/guidelines/database.rst
# Check for similar eager loading patterns in other submission services
echo "Checking other submission services..."
rg -A 5 "eager" src/main/java/de/tum/cit/aet/artemis/*/service/*SubmissionService.java
Length of output: 32356
src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (3)
419-419: LGTM: Logging message is clear and informative
The log message follows good practices by including the relevant filename parameter.
514-532: LGTM: Well-structured attachment file endpoint
The implementation includes proper authorization, validation, and error handling. The method is well-documented and follows REST best practices.
Let's verify the database query performance:
#!/bin/bash
# Check for N+1 queries or inefficient joins in AttachmentRepository
rg -A 5 "findByIdElseThrow.*Attachment"
# Look for any existing performance metrics or slow query logs
rg -g "*.properties" -A 5 "hibernate.generate_statistics|logging.level.org.hibernate.SQL"Line range hint 495-505: LGTM: Well-implemented attachment retrieval endpoint
The implementation includes proper authorization, validation, and error handling. Given the PR's focus on query performance:
Let's verify the database query performance:
✅ Verification successful
Based on the code analysis, I can now generate the final response:
Verified: The attachment retrieval endpoint is well-implemented with proper validation and caching
The implementation shows:
- Proper repository access using
findByIdElseThrowto avoid N+1 queries - Efficient file path handling through
FilePathServicewith standardized paths - Built-in caching through the file system
- Proper validation of course and attachment unit relationships
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for N+1 queries or inefficient joins in AttachmentUnitRepository
rg -A 5 "findByIdElseThrow.*AttachmentUnit"
Length of output: 133
Script:
#!/bin/bash
# Let's try different approaches to find repository and service layer implementations
# 1. Search for AttachmentUnit related repository/service methods
rg -l "AttachmentUnit"
# 2. Look for specific attachment unit query methods
rg -A 5 "findById.*AttachmentUnit|getAttachmentUnit"
# 3. Check for JPA repository definitions
ast-grep --pattern 'interface $_ extends JpaRepository<AttachmentUnit, $_> {
$$$
}'
Length of output: 319766
Script:
#!/bin/bash
# Let's try to find the AttachmentUnit repository and service implementations
rg -A 5 "class AttachmentUnitRepository|class AttachmentUnitService"
# Also check for findById implementation
rg -A 5 "findByIdElseThrow.*AttachmentUnit|findById.*AttachmentUnit"
# Look for any potential N+1 query patterns in the service layer
rg -A 10 "getAttachmentUnitFile|getAttachment.*AttachmentUnit"
Length of output: 305858
src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java (3)
420-420: LGTM! Appropriate eager loading strategy.
The query efficiently loads participations with their results and submissions in a single fetch, which aligns with the PR's performance improvement goals.
533-533: Consistent with previous performance optimization comment.
Line range hint 1019-1030: LGTM! Well-documented return values.
The JavaDoc clearly describes the return types and purpose of the methods, which aids in code maintainability.
src/main/java/de/tum/cit/aet/artemis/text/web/TextAssessmentResource.java (2)
351-351: Javadoc Comment Update Correctly Reflects Return Type
The updated Javadoc comment accurately describes the method's return value, aligning with the changes made to the method.
390-392: Reassignment of 'textSubmission' Ensures Updated State
Reassigning textSubmission after locking and explicitly setting the participation ensures that the submission reflects the latest state and maintains consistency within the method.
src/main/java/de/tum/cit/aet/artemis/exercise/service/SubmissionService.java (1)
238-238: Logic for filtering submissions before due date looks good
The added condition to select only submissions before the due date in non-exam mode is appropriate and enhances the correctness of submission selection.
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (5)
101-101: Importing EnforceAtLeastTutorInCourse Annotation
The import statement for EnforceAtLeastTutorInCourse is correctly added.
260-260: Refactored Group Name Comparison into Separate Method
Extracting the group name comparison logic into the getChangedGroupNames method improves code readability and reusability.
441-444: Refactored Course Retrieval Logic for Tutors
Extracting the course retrieval logic into getCoursesForTutors enhances code modularity and reduces duplication.
502-503: Reused getCoursesForTutors Method for Consistency
Reusing getCoursesForTutors method enhances consistency and promotes code reuse.
715-722: Enhanced Access Control with @EnforceAtLeastTutorInCourse
Adding @EnforceAtLeastTutorInCourse ensures that only users with at least a tutor role in the specific course can access this endpoint, enhancing security.
src/main/java/de/tum/cit/aet/artemis/lti/web/LtiResource.java (2)
58-58: Constructor injection of OnlineCourseConfigurationService is appropriate
The addition of onlineCourseConfigurationService via constructor injection aligns with best practices for dependency injection.
Also applies to: 74-78
82-122: Overall method implementation aligns with REST principles
The updateOnlineCourseConfiguration method appropriately handles the PUT request, includes necessary authorization checks, and maintains proper error handling.
src/main/java/de/tum/cit/aet/artemis/fileupload/web/FileUploadSubmissionResource.java
Show resolved
Hide resolved
...main/java/de/tum/cit/aet/artemis/atlas/service/competency/StandardizedCompetencyService.java
Show resolved
Hide resolved
...main/java/de/tum/cit/aet/artemis/atlas/service/competency/StandardizedCompetencyService.java
Show resolved
Hide resolved
magaupp
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
changed the title
Development: Improve slow query performanceDevelopment: Improve slow query performance during assessment
krusche
changed the title
Development: Improve slow query performance during assessmentDevelopment: Improve slow query performance during manual assessment
Checklist
General
Server
Motivation and Context
The database slow query log reports slow queries that take a long time to finish, e.g. due to unnecessarily high amount to data being transferred. This PR tries to improve the situation for a few queries
Description
Avoid fetching too much data when getting the next "assessable" submission.
Steps for Testing
Prerequisites:
You can use https://artemis-staging-localci.artemis.cit.tum.de/course-management/102/exams/209/assessment-dashboard for testing
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
These updates aim to enhance user experience and improve overall application performance.