Quiz exercises: Fix an error after using the practice mode
#9571
Conversation
github-actions
bot
added
server
Hialus
force-pushed
the
bugfix/quiz/fix-practice-mode
branch
from
89f06ca
to
13a5d8c
Compare
WalkthroughThe pull request modifies the 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: 0
🧹 Outside diff range and nitpick comments (3)
src/main/java/de/tum/cit/aet/artemis/quiz/repository/QuizSubmissionRepository.java (1)
42-50: Add JavaDoc documentation for the new method.To maintain consistency with other documented methods in the repository, please add JavaDoc documentation explaining the purpose and parameters of the new method.
Add this documentation above the method:
+ /** + * Retrieve QuizSubmission with eagerly fetched submitted answers for a given result ID + * + * @param resultId the ID of the result for which to find the submission + * @return Optional containing the QuizSubmission if found, empty otherwise + */ @Query("""src/main/java/de/tum/cit/aet/artemis/quiz/web/QuizParticipationResource.java (2)
72-73: Consider splitting the endpoint responsibilities.The TODOs highlight that this endpoint serves dual purposes: starting a participation and viewing results. This violates the single responsibility principle and REST best practices.
Consider:
- Creating a separate endpoint for viewing results (e.g.,
GET /quiz-exercises/{exerciseId}/results)- Renaming this endpoint to clarify its primary purpose
- Adding proper documentation about the endpoint's behavior
Would you like me to propose a detailed design for this separation of concerns?
97-104: Enhance error handling and documentation.While the logic correctly handles both new and existing results, there are several improvements that could be made:
- Add custom exception messages to
.orElseThrow()calls to provide better error context- Document the business logic explaining why we handle results differently
- Consider more descriptive variable names
Apply this diff:
if (result.getId() == null) { // Load the live submission of the participation - result.setSubmission(quizSubmissionRepository.findWithEagerSubmittedAnswersByParticipationId(participation.getId()).orElseThrow()); + result.setSubmission(quizSubmissionRepository.findWithEagerSubmittedAnswersByParticipationId(participation.getId()) + .orElseThrow(() -> new IllegalStateException( + String.format("No submission found for participation %d", participation.getId())))); } else { // Load the actual submission of the result - result.setSubmission(quizSubmissionRepository.findWithEagerSubmittedAnswersByResultId(result.getId()).orElseThrow()); + result.setSubmission(quizSubmissionRepository.findWithEagerSubmittedAnswersByResultId(result.getId()) + .orElseThrow(() -> new IllegalStateException( + String.format("No submission found for result %d", result.getId())))); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/main/java/de/tum/cit/aet/artemis/quiz/repository/QuizSubmissionRepository.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/quiz/web/QuizParticipationResource.java (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/quiz/repository/QuizSubmissionRepository.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_importssrc/main/java/de/tum/cit/aet/artemis/quiz/web/QuizParticipationResource.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
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/quiz/repository/QuizSubmissionRepository.java (1)
42-50: Consider performance optimizations for the new query method.The implementation looks good and follows the repository patterns. However, there are a few suggestions for optimization:
- Consider adding an index hint for the
results.idcolumn if not already indexed, as this is now a primary lookup path.- If the result details are frequently accessed, consider eager fetching them to prevent N+1 queries.
Consider these improvements:
@Query(""" SELECT submission FROM QuizSubmission submission LEFT JOIN FETCH submission.submittedAnswers - JOIN submission.results r + JOIN FETCH submission.results r WHERE r.id = :resultId """)Let's verify if there's an index on the results table:
src/main/java/de/tum/cit/aet/artemis/quiz/web/QuizParticipationResource.java (1)
97-104: Verify database query performance.The code makes multiple database calls in sequence. Let's verify the query performance and check if we can optimize it.
✅ Verification successful
Based on the results, I can now generate the final response since I have enough information about the database queries.
Database queries are properly optimized with eager loading
The repository methods are well-implemented with appropriate JOIN FETCH clauses to prevent N+1 query issues:
- Both
findWithEagerSubmittedAnswersByParticipationIdandfindWithEagerSubmittedAnswersByResultIduse LEFT JOIN FETCH for loading submitted answers- The queries are separate but each performs a single database call with eager loading
- The conditional logic ensures only one query is executed per request
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for N+1 query patterns and verify eager loading # Test 1: Check if the repository methods use proper JOIN FETCH ast-grep --pattern 'SELECT $$$' # Test 2: Look for related repository queries to see if we can combine them rg -A 5 'findWithEagerSubmittedAnswersByResultId|findWithEagerSubmittedAnswersByParticipationId'Length of output: 3811
|
github-actions
bot
added
the
deployment-error
github-actions
bot
removed
the
deployment-error
sachmii
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
This is also fixed now |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizSubmissionService.java (1)
150-150: LGTM! Consider adding a comment explaining submission selection.The change correctly handles multiple submissions by selecting the first one, fixing the internal server error. However, it would be helpful to add a comment explaining why we choose the first submission when multiple exist.
- Optional<QuizSubmission> quizSubmissionOptional = quizSubmissionRepository.findWithEagerSubmittedAnswersByParticipationId(participation.getId()).stream().findFirst(); + // Select the first submission when multiple exist (e.g., from practice mode) + Optional<QuizSubmission> quizSubmissionOptional = quizSubmissionRepository.findWithEagerSubmittedAnswersByParticipationId(participation.getId()).stream().findFirst();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- src/main/java/de/tum/cit/aet/artemis/quiz/repository/QuizSubmissionRepository.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizSubmissionService.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/quiz/web/QuizParticipationResource.java (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/de/tum/cit/aet/artemis/quiz/repository/QuizSubmissionRepository.java
- src/main/java/de/tum/cit/aet/artemis/quiz/web/QuizParticipationResource.java
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizSubmissionService.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
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizSubmissionService.java (2)
Line range hint
273-274: Enhance error handling in TODO section.The TODO comment suggests adding additional checks, but the comment about performance concerns seems premature. Consider:
- Implementing course membership validation
- Moving the check earlier in the request pipeline
- Adding specific error messages for each failure case
Let's analyze the current error handling patterns:
Line range hint
211-241: Consider prioritizing migration to DTOs.The current approach of modifying entity objects directly before sending to the client is fragile and could lead to security issues. While the @deprecated annotation indicates future plans to use DTOs, this should be prioritized because:
- Direct entity modification is an anti-pattern
- Current approach risks accidental exposure of sensitive data
- Changes to entity structure could break this logic
Let's check if there are other similar patterns in the codebase:
✅ Verification successful
Let me gather more information to verify the presence of DTOs and similar patterns in the codebase:
Based on the search results, I can now generate the final response:
Migration to DTOs is already a pattern in the codebase
The codebase shows extensive use of DTOs across different modules, with numerous DTO classes and conversion patterns present. The current deprecated method is an outlier from the established patterns. Specific observations:
- Many modules already follow DTO pattern (e.g.,
ExerciseDTO,ParticipationDTO, etc.)- Conversion services exist (e.g.,
ConversationDTOServicewithconvertToDTOmethods)- The codebase demonstrates proper separation of domain and DTO layers
The review comment's concern about migrating to DTOs is valid and aligns with the existing architectural patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar patterns of entity modification before sending to client rg -l "removeUnnecessaryObjects|filterForClient|stripSensitiveData" # Search for existing DTO patterns to align with ast-grep --pattern 'class $_ implements $_ { $$$ toDTO() { $$$ } $$$ }'Length of output: 356
Script:
#!/bin/bash # Search for DTO classes and patterns rg -l "DTO|Dto" --type java # Look for specific patterns of data transformation rg -A 5 "toDTO|mapToDTO|convertToDTO" --type java # Check for any existing mapper classes rg -l "Mapper|mapper" --type javaLength of output: 76814
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
changed the title
Quiz exercises: Fix internal server error after using the practice modeQuiz exercises: Fix an error after using the practice mode
Checklist
General
Server
Motivation and Context
We encountered a bug today on production which prevented students from viewing their quiz result after they also participated in the practice mode.
Description
This was caused by a small change in the endpoint where we tried to load a single submission of a participation. This does not work, as participations can have multiple submissions when it is not fully new. I now changed it to instead load the submission of the fetched result if it has any.
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
-> Integration test will be done in a follow-up
Summary by CodeRabbit
New Features
Improvements