Athena: Improve AI feedback request validation
#10165
Conversation
github-actions
bot
added
tests
server
github-actions
bot
added
athena
maximiliansoelch
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
EneaGore
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
|
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. 🔧 pmd (7.8.0)src/main/java/de/tum/cit/aet/artemis/text/service/TextExerciseFeedbackService.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. WalkthroughThe pull request introduces comprehensive changes to the feedback request and validation process across multiple exercise types (text, modeling, programming). The modifications centralize rate limit and submission validation logic in the Changes
Sequence DiagramsequenceDiagram
participant Client
participant ParticipationResource
participant AthenaFeedbackSuggestionsService
participant ExerciseFeedbackService
Client->>ParticipationResource: Request Feedback
ParticipationResource->>AthenaFeedbackSuggestionsService: Check Rate Limit
AthenaFeedbackSuggestionsService-->>ParticipationResource: Validate Request
ParticipationResource->>ExerciseFeedbackService: Generate Feedback
ExerciseFeedbackService->>AthenaFeedbackSuggestionsService: Validate Submission
AthenaFeedbackSuggestionsService-->>ExerciseFeedbackService: Confirm Validity
ExerciseFeedbackService-->>Client: Return Feedback
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.html (1)
Line range hint
4-15: Consider enhancing the tooltip message for better user guidance.While the button's disabled state correctly prevents invalid feedback requests, consider making the tooltip more informative about the submission requirements. This would align with the PR's goal of providing clearer error messages.
Consider updating the translation key to include submission requirements:
- [ngbTooltip]="'artemisApp.exerciseActions.requestAutomaticFeedbackTooltip' | artemisTranslate" + [ngbTooltip]="(isSubmitted() ? 'artemisApp.exerciseActions.requestAutomaticFeedbackTooltip' : 'artemisApp.exerciseActions.submissionRequiredTooltip') | artemisTranslate"src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseCodeReviewFeedbackService.java (2)
118-118: Consider improving the error message clarity.While the addition of the boolean parameter is good, the error message "No legal submissions found" could be more specific about what makes a submission "legal" to help users understand and fix the issue.
Consider this alternative:
- throw new BadRequestAlertException("No legal submissions found", "submission", "noSubmissionExists", true); + throw new BadRequestAlertException("No valid submissions found for feedback generation. Please ensure you have submitted your solution.", "submission", "noSubmissionExists", true);
Line range hint
93-118: Good architectural improvements!The changes demonstrate good architectural decisions:
- Centralization of rate limit checks reduces code duplication
- Clear separation of concerns with proper delegation to
AthenaFeedbackSuggestionsService- Improved error handling with more specific feedback
src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingExerciseFeedbackService.java (2)
102-105: Enhance logging for better traceability.The debug log statement now clearly indicates the use of Athena for feedback generation and specifies the exercise ID.
Consider including the submission ID in the log for more precise tracing:
log.debug("Using athena to generate (modeling exercise) feedback request: {}", modelingExercise.getId()); +log.debug("Using submission ID: {}", modelingSubmission.getId());
110-110: Remove redundant log statement or provide meaningful information.The log statement
log.debug("Submission id: {}", modelingSubmission.getId());is isolated and may be redundant given the context.Combine it with the previous log statement or enhance it to provide additional context.
-log.debug("Submission id: {}", modelingSubmission.getId()); +log.debug("Generating feedback for submission ID: {} in exercise ID: {}", modelingSubmission.getId(), modelingExercise.getId());src/test/java/de/tum/cit/aet/artemis/exercise/participation/ParticipationIntegrationTest.java (3)
648-650: Optimize code by removing unused variables and imports.The variable
submissionis assigned but not used in the subsequent code.Remove the unused variable to clean up the code.
-TextSubmission submission = (TextSubmission) resultText1.getSubmission();Alternatively, if the
submissionvariable is intended for future use, consider utilizing it appropriately.
768-770: Enhance code clarity by properly utilizing variables.The variable
submissionis being set but not effectively used in assertions or further logic.Consider using the
submissionvariable to verify the submission content or remove it if unnecessary.
805-807: Utilize thesubmissionvariable meaningfully.Similar to previous instances, the
submissionvariable is assigned values but not used in subsequent assertions or logic.Ensure that the
submissionvariable is either used for assertions to validate test conditions or removed if not needed.src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java (1)
404-406: Consider extracting the submission check to a separate method.The validation logic looks good and aligns with the PR objectives. However, to improve readability and maintainability, consider extracting the submission check into a separate method.
- boolean hasSubmittedOnce = submissionRepository.findAllByParticipationId(participation.getId()).stream().anyMatch(Submission::isSubmitted); - if (!hasSubmittedOnce) { - throw new BadRequestAlertException("You need to submit at least once", "participation", "noSubmissionExists", true); - } + validateSubmissionExists(participation); + } + + private void validateSubmissionExists(StudentParticipation participation) { + boolean hasSubmittedOnce = submissionRepository.findAllByParticipationId(participation.getId()).stream() + .anyMatch(Submission::isSubmitted); + if (!hasSubmittedOnce) { + throw new BadRequestAlertException("You need to submit at least once", "participation", "noSubmissionExists", true); + }src/main/java/de/tum/cit/aet/artemis/text/service/TextExerciseFeedbackService.java (2)
86-88: Ensure proper null or empty check fortextSubmissionThe method
textSubmission.isEmpty()is used to check if the submission is empty. Ensure thattextSubmissionis notnullbefore calling this method to prevent a potentialNullPointerException.Optionally, you can add a null check:
+if (textSubmission == null || textSubmission.isEmpty()) { throw new BadRequestAlertException("Submission cannot be empty for an AI feedback request", "submission", "noAthenaFeedbackOnEmptySubmission", true); }
90-90: Specify an executor forCompletableFuture.runAsyncBy default,
CompletableFuture.runAsyncuses theForkJoinPool.commonPool(). To have better control over thread management and resource utilization, consider specifying a custom executor.Example:
-CompletableFuture.runAsync(() -> this.generateAutomaticNonGradedFeedback(textSubmission, participation, textExercise)); +CompletableFuture.runAsync(() -> this.generateAutomaticNonGradedFeedback(textSubmission, participation, textExercise), Executors.newSingleThreadExecutor());Ensure that the executor service is properly managed and shut down when no longer needed.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/de/tum/cit/aet/artemis/athena/service/AthenaFeedbackSuggestionsService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingExerciseFeedbackService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseCodeReviewFeedbackService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/text/service/TextExerciseFeedbackService.java(2 hunks)src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.html(1 hunks)src/main/webapp/i18n/de/exercise.json(1 hunks)src/main/webapp/i18n/en/exercise.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/participation/ParticipationIntegrationTest.java(5 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.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/webapp/i18n/de/exercise.json (1)
Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseCodeReviewFeedbackService.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/athena/service/AthenaFeedbackSuggestionsService.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/TextExerciseFeedbackService.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/test/java/de/tum/cit/aet/artemis/exercise/participation/ParticipationIntegrationTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingExerciseFeedbackService.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/programming/service/ProgrammingExerciseCodeReviewFeedbackService.java (1)
Learnt from: undernagruzez
PR: ls1intum/Artemis#8498
File: src/main/java/de/tum/in/www1/artemis/service/programming/ProgrammingExerciseCodeReviewFeedbackService.java:236-236
Timestamp: 2024-11-12T12:51:40.391Z
Learning: Rate limit exceptions for AI feedback requests are handled in the client app.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
🔇 Additional comments (21)
src/main/webapp/app/overview/exercise-details/request-feedback-button/request-feedback-button.component.html (2)
1-2: Great use of the new Angular control flow syntax!The template correctly uses the new
@ifsyntax as per the coding guidelines.
3-3: Appropriate extension of exercise type check.The condition now correctly includes both TEXT and MODELING exercise types, which aligns with the PR objective to standardize validation across exercise types.
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseCodeReviewFeedbackService.java (2)
93-93: LGTM! Good refactoring to centralize rate limit checks.The delegation of rate limit checking to
AthenaFeedbackSuggestionsServicereduces code duplication and improves maintainability by centralizing this responsibility.
112-112: LGTM! Improved log message clarity.The debug log message now clearly specifies that this is for a programming exercise, which helps distinguish it from other exercise types during debugging.
src/main/java/de/tum/cit/aet/artemis/modeling/service/ModelingExerciseFeedbackService.java (6)
72-73: Consolidate rate limit checks to improve code reuse.The rate limit check previously implemented in this class has been moved to the
AthenaFeedbackSuggestionsService. By directly callingcheckRateLimitOrThrowfrom the Athena service, we enhance code reuse and maintainability.
74-81: Simplify submission retrieval and validate presence effectively.The code now retrieves the latest submission directly and checks if it is present. This change ensures that a valid submission exists before proceeding with feedback generation.
83-84: Ensure submission does not have an existing Athena result.By invoking
checkLatestSubmissionHasNoAthenaResultOrThrow, the code verifies that the submission hasn't been processed by Athena before, preventing duplicate feedback requests.
85-87: Improve validation for empty submissions.The added check ensures that a submission is not empty before requesting Athena feedback, enhancing error handling and providing clearer feedback to the user.
98-100: Update method signature to include modeling submission.The
generateAutomaticNonGradedFeedbackmethod now accepts aModelingSubmissionas a parameter, which streamlines the feedback generation process by directly using the provided submission.
122-122: Ensure submission and result are correctly associated and saved.The method
saveNewResultis correctly used to save the association between the submission and the new automatic result.src/main/java/de/tum/cit/aet/artemis/athena/service/AthenaFeedbackSuggestionsService.java (3)
65-67: EnsureallowedFeedbackRequestsis configurable and has a sensible default.The introduction of
allowedFeedbackRequestswith a default value of 10 allows for flexibility in configuring the maximum number of feedback requests.
195-211: Implement rate limiting for feedback requests.The new method
checkRateLimitOrThroweffectively checks if the student has exceeded the allowed number of Athena feedback requests, enhancing fairness and resource management.
213-227: Prevent duplicate Athena results for the same submission.The method
checkLatestSubmissionHasNoAthenaResultOrThrowensures that a submission is not reprocessed by Athena if it already has an Athena result, avoiding unnecessary processing and potential confusion.src/test/java/de/tum/cit/aet/artemis/exercise/participation/ParticipationIntegrationTest.java (4)
1711-1719: Validate behavior when no submission exists for feedback request.The test
whenTextFeedbackRequestedAndNoSubmission_thenFailcorrectly checks that aBAD_REQUESTis returned when a feedback request is made without any submission.
1721-1733: Verify feedback request handling for unsubmitted submissions.The test
whenTextFeedbackRequestedAndNoSubmittedSubmission_thenFailensures that feedback requests fail if the submission exists but is not marked as submitted.
1735-1750: Ensure feedback requests fail for empty submissions.The test
whenTextFeedbackRequestedAndSubmissionEmpty_thenFailappropriately validates that feedback cannot be requested for empty text submissions.
1752-1767: Test feedback request failure for empty modeling submissions.The test
whenModelingFeedbackRequestedAndSubmissionEmpty_thenFailcorrectly checks that aBAD_REQUESTis returned when a feedback request is made for an empty modeling submission.src/main/webapp/i18n/en/exercise.json (1)
179-179: LGTM!The added translation key and message are clear and consistent with the PR objectives.
src/main/webapp/i18n/de/exercise.json (1)
178-179: LGTM!The changes look good:
- The updated message for
submissionAlreadyHasAthenaResultis more precise.- The new message for
noAthenaFeedbackOnEmptySubmissionmatches the English version.- Both messages use informal language (dutzen) as required by the coding guidelines.
src/main/java/de/tum/cit/aet/artemis/text/service/TextExerciseFeedbackService.java (2)
76-77: LGTM!The rate limit check using
athenaFeedbackSuggestionsService.get().checkRateLimitOrThrow(participation);is correctly implemented within theisPresent()check.
99-103: Updated method signature enhances clarityChanging the method
generateAutomaticNonGradedFeedbackto acceptTextSubmissionas a parameter improves clarity by explicitly requiring the necessary data. This makes the method more cohesive and aligns with the single responsibility principle.
src/main/java/de/tum/cit/aet/artemis/text/service/TextExerciseFeedbackService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/de/tum/cit/aet/artemis/text/service/TextExerciseFeedbackService.java (1)
82-82:⚠️ Potential issueUnsafe type casting could lead to ClassCastException
The direct casting of submission to TextSubmission without type verification could cause runtime errors.
Apply this diff to add type safety:
-TextSubmission textSubmission = (TextSubmission) submissionOptional.get(); +var submission = submissionOptional.get(); +if (!(submission instanceof TextSubmission)) { + throw new BadRequestAlertException("Invalid submission type", "submission", "invalidSubmissionType", true); +} +TextSubmission textSubmission = (TextSubmission) submission;
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/text/service/TextExerciseFeedbackService.java (1)
99-103: Consider reordering parameters for consistency.While the addition of the TextSubmission parameter improves the method contract, consider reordering parameters to group related entities:
-public void generateAutomaticNonGradedFeedback(TextSubmission textSubmission, StudentParticipation participation, TextExercise textExercise) +public void generateAutomaticNonGradedFeedback(StudentParticipation participation, TextExercise textExercise, TextSubmission textSubmission)This ordering:
- Places the participation first (consistent with other methods)
- Groups exercise-related parameters together
- Ends with the submission being processed
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/text/service/TextExerciseFeedbackService.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/text/service/TextExerciseFeedbackService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
- GitHub Check: Build and Push Docker Image
- GitHub Check: Analyse
- GitHub Check: Build .war artifact
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/text/service/TextExerciseFeedbackService.java (2)
76-81: LGTM! Well-structured validation flow.The implementation properly validates the rate limit and submission existence before proceeding with the feedback request.
86-88: LGTM! Good validation for empty submissions.The check prevents feedback requests for empty submissions with a clear error message, aligning with the PR objectives.
|
Server tests pass locally. I'm not sure why some of them fail on Bamboo at the moment. We need to investigate this, but it's probably unrelated to this PR, so I will merge it. |
Important
Athena is only enabled on TS1, TS2, or TS3
Checklist
General
Server
Client
Motivation and Context
Currently, there are some issues with the AI feedback request feature:
In addition, all AI Feedback request services for the different exercise types have their own implementation for checking the request rate limit, which causes code duplication and is hard to maintain.
Description
This PR fixes several issues and reduces code duplication for the request AI feedback feature:
Steps for Testing
Prerequisites:
1. Testcase: Request Feedback without a successful submission
Do these steps for each exercise type:
2. Testcase: Request Feedback on an empty text submission
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
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests