Development: Prevent exception in Iris for inactive exercises
#10099
Conversation
…ure is disabled for this exercise for proactive events
|
Warning Rate limit exceeded@krusche has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces several modifications across multiple Java files in the Artemis project. The changes primarily focus on enhancing the Iris integration, improving event handling, and refactoring service methods. Key modifications include updating 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
|
github-actions
bot
added
tests
server
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java (3)
61-62: **Consider avoidingOptionalfields **Using
Optionalas a field can introduce complexity whenget()is called without checkingisPresent(). If Iris is disabled, this may lead toNoSuchElementException. One recommendation is to inject a no-op, mock, or disabled instance instead, allowing you to avoid dealing withOptionalcalls throughout the code.
67-68: **Constructor expansion increases complexity **The constructor is becoming quite large with multiple optional dependencies. Consider refactoring or using a builder pattern if future expansions continue; however, it is acceptable in its current state.
216-216: **Local variable usage **Using
final var exerciseis convenient. Ensure a consistent approach (var vs. explicit type) throughout the codebase per your convention.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventService.java (1)
45-45: **Asynchronous event trigger **Making
triggerasynchronous is beneficial. Consider clarifying concurrency expectations (e.g., thread pool size) in the docstring, especially if events can be numerous.src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (5)
247-247: Prefer usingnever()instead oftimes(0).
This will improve readability by explicitly stating no invocations are expected.- verify(pyrisEventService, times(0)).trigger(new NewResultEvent(result)); + verify(pyrisEventService, never()).trigger(new NewResultEvent(result));
261-262: Typographical fix in the inline comment.
Minor typo: "very" → "verify".- // very that the event is not fired + // verify that the event is not fired
285-288: Usenever()instead oftimes(0)to clarify zero invocations.- verify(pyrisPipelineService, times(0)).executeExerciseChatPipeline(any(), any(), any(), any(), any()); + verify(pyrisPipelineService, never()).executeExerciseChatPipeline(any(), any(), any(), any(), any());
301-301: Usenever()for zero-invocation verification.- verify(irisExerciseChatSessionService, times(0)).onNewResult(any(Result.class)); + verify(irisExerciseChatSessionService, never()).onNewResult(any(Result.class));
316-316: Consistent usage ofnever().
Applying the same reasoning for zero-invocation checks.- verify(irisExerciseChatSessionService, times(0)).onNewResult(any(Result.class)); + verify(irisExerciseChatSessionService, never()).onNewResult(any(Result.class));src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.java (1)
189-191: New spy forPyrisEventService.
Ensure that partial mocking is actually necessary; otherwise a pure mock might suffice.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/assessment/web/ResultWebsocketService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/domain/User.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/repository/IrisExerciseChatSessionRepository.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/iris/repository/IrisExerciseSettingsRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/lti/service/LtiNewResultService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java(4 hunks)src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java(8 hunks)src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.java(3 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/de/tum/cit/aet/artemis/iris/repository/IrisExerciseChatSessionRepository.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/de/tum/cit/aet/artemis/assessment/web/ResultWebsocketService.java
🧰 Additional context used
📓 Path-based instructions (8)
src/main/java/de/tum/cit/aet/artemis/core/domain/User.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/iris/service/pyris/PyrisEventService.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/service/LtiNewResultService.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/iris/repository/IrisExerciseSettingsRepository.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/assessment/service/ResultService.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/iris/PyrisEventSystemIntegrationTest.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/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.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/programming/service/ProgrammingMessagingService.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 (19)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java (5)
31-31: **New Iris repository import **This import is used to support the new field
irisExerciseSettingsRepository. Nothing problematic here.
74-74: **Field assignment verified **Assigning the
irisExerciseSettingsRepositoryfield in the constructor is correct and consistent with Spring’s constructor injection.
203-205: **Extended documentation for Iris notification **These comments clearly outline the conditions to notify Iris. The explanation helps maintainers reason about when notifications are skipped.
214-214: **Java instanceof pattern matching **Utilizing Java’s pattern matching for
instanceofis concise and readable—well done.
218-225: **Exception handling block **The
try/catchlogs errors without interrupting user-facing flows. This is consistent with the asynchronous approach in Iris.src/main/java/de/tum/cit/aet/artemis/iris/repository/IrisExerciseSettingsRepository.java (2)
13-15: **Repository interface declaration **Declaring a dedicated repository for Iris exercise settings aligns well with the single responsibility principle.
17-23: **Check@Querysyntax validity **Using
SELECT EXISTS(s)in JPQL is unusual. Some JPA providers might requireCOUNT(s) > 0instead. Verify that this query runs successfully across all supported database engines.✅ Verification successful
Let me gather more information about the query usage and database configuration.
Let me check for similar query patterns to see how existence checks are typically done in this codebase.
Let me check for similar queries in the codebase to understand the common patterns.
Let me try a different search pattern to find similar queries.
Let me check the database configuration and similar queries with a different approach.
Based on the database configuration showing support for H2, PostgreSQL, and MySQL dialects, and the fact that
SELECT EXISTS(s)is a standard JPQL/HQL syntax supported across these dialects, I can provide the final response:
SELECT EXISTS(s)is a valid JPQL syntax for existence checksThe query syntax used in
isExerciseChatEnabledis valid and supported across all configured database dialects (H2, PostgreSQL, and MySQL). WhileCOUNT(s) > 0is an alternative approach, the currentEXISTSsyntax is more efficient as it stops searching once a match is found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for references to isExerciseChatEnabled to see if query usage is tested rg --context 5 "isExerciseChatEnabled"Length of output: 2705
Script:
#!/bin/bash # Check for test files related to IrisExerciseSettings fd "IrisExerciseSettings.*Test" --type f # Look for database configuration and JPA provider rg -g "application*.{yml,properties}" "spring.jpa|hibernate" # Check for similar EXISTS queries in the codebase ast-grep --pattern 'SELECT EXISTS'Length of output: 5267
Script:
#!/bin/bash # Look for similar existence check queries with different patterns rg -g "*.java" "SELECT.*FROM.*WHERE.*EXISTS|SELECT.*COUNT.*FROM" # Check for other repository files to see query patterns fd "Repository.java$" --type f --exec head -n 50 {}Length of output: 65838
Script:
#!/bin/bash # Look for EXISTS queries in JPQL rg -g "*.java" "SELECT\s+EXISTS|SELECT\s+CASE.*EXISTS" # Look for similar boolean return type queries rg -g "*.java" "@Query.*\n.*SELECT.*\n.*\).*boolean.*"Length of output: 423
Script:
#!/bin/bash # Look for boolean return type queries with better pattern rg -g "*.java" -U "boolean.*\{.*@Query.*SELECT" # Look for count-based existence checks rg -g "*.java" -U "@Query.*SELECT COUNT.*FROM.*WHERE.*boolean"Length of output: 114
Script:
#!/bin/bash # Check database configuration and dialect rg -g "application*.yml" "database-platform|hibernate.dialect" # Look for similar boolean queries with a simpler pattern rg -g "*.java" "boolean.*@Query"Length of output: 554
src/main/java/de/tum/cit/aet/artemis/lti/service/LtiNewResultService.java (2)
6-6: **Asynchronous method import **Importing
@Asyncis consistent with the non-blocking approach.
26-26: **@AsyncononNewResult**Making
onNewResultasynchronous prevents blocking calls and enhances scalability. No concurrency hazards detected sincelti13Serviceusage is straightforward.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventService.java (2)
8-8: **Async annotation import **This is aligned with the approach taken in
LtiNewResultService, supporting non-blocking event handling.
51-51: **Log level switch from info to debug **Switching to
debugreduces log noise while preserving trace details for troubleshooting.Also applies to: 56-56
src/main/java/de/tum/cit/aet/artemis/core/domain/User.java (1)
537-539: **hasAcceptedIrismethod introduced **This boolean check is straightforward and consistent. Good use of a distinct method to clarify user acceptance logic.
src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (4)
90-96: Good approach for replicating realistic user acceptance conditions.
No issues detected with settingirisAcceptedTimestampfor these test users.
188-190: Confirm that 2-second wait is sufficient for asynchronous steps.
If there are intermittent timing issues on slower CI machines, consider increasing theawait()duration.
210-211: Well-structured test for asynchronous event triggers.
The combination oftriggerandawait().atMost(...)is aligned with best practices for async verifications.
215-216: Succinct event validation.
LGTM, the verification logic is clear and includes essential parameters.src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.java (2)
76-76: Import forPyrisEventServicealigns with usage in the test context.
Looks correct, no star imports used.
147-161: Re-introduction of@MockitoSpyBeanfields.
Confirm that these spy beans are actually used in the test suite, as extraneous injections may slow down tests.src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (1)
201-202: Verify calling code compatibility after changing return type tovoid.
Removing the return value is fine if no callers rely on theResultobject. Double-check for potential breaks or call-site assumptions.
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java
Outdated
Show resolved
Hide resolved
SimonEntholzer
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
SimonEntholzer
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
EneaGore
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
dismissed stale reviews from coderabbitai[bot], SimonEntholzer, and ole-ve
via
9aedbd5
github-actions
bot
added
buildagent
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/java/de/tum/cit/aet/artemis/exercise/service/ParticipationTeamWebsocketServiceTest.java (1)
143-143: Asynchronous no-invocation check is clear.Using
after(1000).never()effectively asserts no calls after waiting a short period. Just watch out for potential test flakiness if system load varies.src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (1)
91-97: User acceptance timestamps are now set to a day ago.This ensures tests accurately reflect a past acceptance date. Confirm there's no off-by-one or timezone mismatch in edge cases.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java(4 hunks)src/test/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/ParticipationTeamWebsocketServiceTest.java(4 hunks)src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java
🧰 Additional context used
📓 Path-based instructions (3)
src/test/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerServiceTest.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/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.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/test/java/de/tum/cit/aet/artemis/exercise/service/ParticipationTeamWebsocketServiceTest.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
🔇 Additional comments (13)
src/test/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerServiceTest.java (1)
86-86: Nice usage ofnever()for clarifying no invocations.Replacing
times(0)withnever()improves test readability and conveys the intent more clearly.src/test/java/de/tum/cit/aet/artemis/exercise/service/ParticipationTeamWebsocketServiceTest.java (3)
5-5: Good use ofafter()for asynchronous verification.This approach can help avoid false positives in asynchronous tests. Ensure the chosen delay is large enough for all expected operations without making the test too slow.
156-156: Consistent approach to ensuring non-invocation in async context.This matches the logic in the previous test method, aiding maintainability and clarity.
167-169: Both verifications useafter().never().It's consistent to verify no calls to different methods here. Keep an eye on test durations to ensure these checks don't introduce unnecessary delays.
src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (9)
8-8: Importingnever()aligns with best practices.This import clarifies the intent to ensure zero method invocations. Great for readability.
189-191: Awaiting assertion for onNewResult call is a good async testing strategy.Waiting before asserting helps avoid race conditions in asynchronous test scenarios.
211-212: Use of await() for verifying build failure flow is appropriate.Ensures that asynchronous code has run before final assertions.
216-217: Verifying pipeline execution with times(1) after awaiting.This is consistent with the build-failed test pattern. Fine approach for asynchronous verifications.
248-248: Usingnever()for disabled events.This effectively confirms that no event was triggered. Matches the intended function of disabled event types.
262-263: Again,never()clarifies that no event should be triggered.This ensures the failing submission event is not fired when settings are disabled.
286-289: Partial verification usingtimes(2)andnever()is coherent.The combination of verifying some calls while disallowing others is clearly expressed here.
302-303: No new result invocation with only two submissions.
never()effectively asserts that onNewResult is not prematurely called. This logic looks correct.
317-318: Ensuring non-invocation for increasing scores scenario.The approach is consistent with earlier checks, confirming no calls are made unless specific conditions are met.
Checklist
General
Server
Motivation and Context
This PR suppresses exceptions that should NOT be thrown. It also tries to prevent unnecessary database calls when Iris is active globally, but not active for an exercise. Additionally, it improves data privacy by not executing
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
Code Review
Manual Tests
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
varkeyword.Bug Fixes
Chores