Development: Remove eager relationship between team and students

[krusche]

Checklist

General

• I tested all changes and their related features with all corresponding user types on a test server.
• Language: I followed the guidelines for inclusive, diversity-sensitive, and appreciative language.
• I chose a title conforming to the naming conventions for pull requests.

Server

• Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
• I followed the coding and design guidelines.

Motivation and Context

In #7719, the migration to Hibernate 6 introduced circular references in some produced SQL queries. To mitigate this effect, we replace explicit EAGER references on models with fetch queries wherever necessary.
This PR affects the relation between Teams and Students

Description

We removed the explicit EAGER, added a student fetch to some queries and replaced certain query usages where fetching students is required.

Steps for Testing

Prerequisites:

• 1 Instructor
• 3 Students
• 1 Team Programming Exercise with Complaints enabled
• 2 further Team exercises

1. Submit to the programming exercise, ensure normal behaviour and update the status and score.
2. Delete the build plans through Artemis (Exercise Reset) to change the InitializationState.
3. Resume the exercise and submit it once more. Ensure normal behaviour.
4. Finalise the exercise and grade them
5. Submit a complaint as a team member, answer the complaint as an instructor and view the complaint and response as a team member.
6. Create third team exercise
7. Import teams from exercise1 to exercise3 using the exercise team import. Make sure your students receive the websocket messages
8. Import teams from exercise2 to exercise3 using the list team import. Make sure your students receive the websocket messages
9. Update a team. Make sure your students receive the websocket messages
10. Delete a team. Make sure your students receive the websocket messages

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked

Review Progress

Performance Review

• I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

• Code Review 1
• Code Review 2

Manual Tests

• Test 1
• Test 2

Test Coverage

No additional assertions, but changed production lines

Class/File	Line Coverage	Confirmation (assert/expect)
AuthorizationCheckService.java	94.6%
ComplaintService.java	91.1%
ParticipationAuthorizationCheckService.java	100%
ParticipationService.java	94.6%
ProgrammingMessagingService.java	100%
TeamImportStrategy.java	90%
ComplaintResponseResource.java	96.9%
ParticipationResource.java	94.1%
TeamResource.java	96.7%
TeamWebsocketService.java	91.3%

Summary by CodeRabbit

• New Features

  • Enhanced team participation data retrieval with eager loading of team students across various services and resources.
  • Improved team management with new repository methods for loading teams and students.
  • Optimized queries for fetching programming submissions with related entities.

• Bug Fixes

  • Fixed issues related to team information not being eagerly loaded in certain scenarios, which could lead to incomplete data presentation.

• Refactor

  • Renamed methods for clarity and consistency in handling team scores and participations.
  • Adjusted constructors and method signatures to accommodate new team-related operations.

• Documentation

  • Updated comments and documentation in service classes to reflect the new team handling logic.

• Tests

  • Modified test expectations to align with the updated query counts and team participation logic.

[github-actions]

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

[coderabbitai]

Walkthrough

The overarching modification in the Artemis application focuses on optimizing data retrieval for team-related entities. This is achieved by shifting from eager to lazy fetching in the Team class and introducing additional eager loading in various repositories. Methods across services and resources have been adapted to accommodate these changes, ensuring that team and student information is efficiently loaded when necessary. The adjustments enhance performance and data handling for team participations, submissions, and scores.

Changes

File Path	Change Summary
.../domain/Team.java	Changed @ManyToMany fetch type from EAGER to LAZY.
.../repository/ParticipationRepository.java .../repository/ProgrammingExerciseStudentParticipationRepository.java .../repository/StudentParticipationRepository.java .../repository/TeamRepository.java .../repository/TeamScoreRepository.java	Enhanced methods with eager fetching for teams and students, added @EntityGraph annotations, and renamed methods for clarity.
.../repository/ProgrammingSubmissionRepository.java	Custom JPQL query added to fetch ProgrammingSubmission with related entities eagerly.
.../service/... (Various Services) .../web/rest/... (Various Resources)	Adjusted to accommodate repository changes with eager loading and modified method calls. Added TeamRepository where relevant.
.../web/websocket/team/TeamWebsocketService.java	Introduced conditional checks for team students initialization and retrieval.
.../test/... (Various Tests)	Updated tests to reflect the new repository method calls and added assertions.

"In the code's green field, we hop and leap,
Fetching data, no more in a single sweep.
🐇 With lazy and eager, we fine-tune the load,
A rabbit's touch on the improved code road."

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?

Share

• X
• Mastodon
• Reddit
• LinkedIn

---

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit-tests for this file.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit tests for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository from git and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit tests.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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 as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

[coderabbitai]

Review Status

Actionable comments generated: 7

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between e504e96 and 4b18fe6.

Files selected for processing (24)

• src/main/java/de/tum/in/www1/artemis/domain/Team.java (1 hunks)
• src/main/java/de/tum/in/www1/artemis/repository/ParticipationRepository.java (1 hunks)
• src/main/java/de/tum/in/www1/artemis/repository/ProgrammingExerciseStudentParticipationRepository.java (1 hunks)
• src/main/java/de/tum/in/www1/artemis/repository/ProgrammingSubmissionRepository.java (1 hunks)
• src/main/java/de/tum/in/www1/artemis/repository/StudentParticipationRepository.java (1 hunks)
• src/main/java/de/tum/in/www1/artemis/repository/TeamRepository.java (6 hunks)
• src/main/java/de/tum/in/www1/artemis/repository/TeamScoreRepository.java (1 hunks)
• src/main/java/de/tum/in/www1/artemis/service/AuthorizationCheckService.java (3 hunks)
• src/main/java/de/tum/in/www1/artemis/service/ComplaintService.java (2 hunks)
• src/main/java/de/tum/in/www1/artemis/service/ParticipantScoreService.java (1 hunks)
• src/main/java/de/tum/in/www1/artemis/service/ParticipationAuthorizationCheckService.java (3 hunks)
• src/main/java/de/tum/in/www1/artemis/service/ParticipationService.java (1 hunks)
• src/main/java/de/tum/in/www1/artemis/service/TeamService.java (1 hunks)
• src/main/java/de/tum/in/www1/artemis/service/programming/ProgrammingMessagingService.java (3 hunks)
• src/main/java/de/tum/in/www1/artemis/service/programming/ProgrammingSubmissionService.java (2 hunks)
• src/main/java/de/tum/in/www1/artemis/service/scheduled/ParticipantScoreScheduleService.java (1 hunks)
• src/main/java/de/tum/in/www1/artemis/service/team/TeamImportStrategy.java (2 hunks)
• src/main/java/de/tum/in/www1/artemis/web/rest/ComplaintResponseResource.java (3 hunks)
• src/main/java/de/tum/in/www1/artemis/web/rest/ParticipationResource.java (2 hunks)
• src/main/java/de/tum/in/www1/artemis/web/rest/TeamResource.java (4 hunks)
• src/main/java/de/tum/in/www1/artemis/web/rest/open/PublicProgrammingSubmissionResource.java (1 hunks)
• src/main/java/de/tum/in/www1/artemis/web/websocket/team/TeamWebsocketService.java (5 hunks)
• src/test/java/de/tum/in/www1/artemis/DatabaseQueryCountTest.java (1 hunks)
• src/test/java/de/tum/in/www1/artemis/exercise/modelingexercise/compass/umlmodel/AbstractUMLDiagramTest.java (1 hunks)

Additional comments: 27

src/main/java/de/tum/in/www1/artemis/service/team/TeamImportStrategy.java (1)

• 5-6: The import of org.hibernate.Hibernate is necessary for the Hibernate.isInitialized method used in the cloneTeamsIntoDestinationExercise method.

src/main/java/de/tum/in/www1/artemis/repository/TeamScoreRepository.java (1)

• 48-52: The new method getAchievedPointsOfTeamsWithStudents correctly uses a LEFT JOIN FETCH to eagerly fetch the students of the team, which should optimize database interactions.

src/main/java/de/tum/in/www1/artemis/web/websocket/team/TeamWebsocketService.java (2)

• 7-7: The import of org.hibernate.Hibernate is necessary for the Hibernate.isInitialized method used in the sendTeamAssignmentUpdate method.

• 29-35: The addition of the TeamRepository field and its inclusion in the constructor are necessary for the lazy loading logic in the sendTeamAssignmentUpdate method.

src/main/java/de/tum/in/www1/artemis/domain/Team.java (1)

• 45-45: The change from EAGER to LAZY fetching for the students collection in the Team class aligns with the PR objectives to optimize database interactions.

src/main/java/de/tum/in/www1/artemis/repository/ProgrammingSubmissionRepository.java (1)

• 37-53: The new JPQL query method findByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents and the default method findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents are designed to optimize the retrieval of ProgrammingSubmission entities with their associated data.

src/main/java/de/tum/in/www1/artemis/repository/ParticipationRepository.java (1)

• 71-79: The methods findWithEagerSubmissionsByIdWithTeamStudents and findWithEagerSubmissionsByIdWithTeamStudentsElseThrow have been updated to include eager fetching of the team and its students, which aligns with the PR objectives to optimize database interactions.

src/main/java/de/tum/in/www1/artemis/service/ParticipationAuthorizationCheckService.java (2)

• 9-12: The import modifications are necessary for the new logic related to team and student fetching in the ParticipationAuthorizationCheckService.

• 27-35: The addition of the TeamRepository field and its inclusion in the constructor are necessary for the lazy loading logic in the canAccessParticipation method.

src/main/java/de/tum/in/www1/artemis/repository/TeamRepository.java (4)

• 3-4: The import of EntityGraphType.LOAD is necessary for the @EntityGraph annotation used in the TeamRepository.

• 29-29: The @EntityGraph annotation on the findAllByExerciseId method is used to eagerly fetch the students of the team, which should optimize database interactions.

• 43-43: The simplification of the getNumberOfTeamsForExercise method query is a performance improvement and makes the query more readable.

• 64-76: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [68-79]

The replacement of findOneWithEagerStudents with findWithStudentsById and the addition of findWithStudentsByIdElseThrow are in line with the PR objectives to optimize database interactions.

src/main/java/de/tum/in/www1/artemis/service/TeamService.java (1)

• 86-86: The logic to remove users from the version control repository if they are no longer part of the team seems correct. It's good to see that the code is handling the synchronization of team members with the VCS permissions.

src/main/java/de/tum/in/www1/artemis/service/scheduled/ParticipantScoreScheduleService.java (1)

• 249-249: The change from findById to findWithStudentsById in the executeTask method is consistent with the PR's objective to switch from EAGER to LAZY loading. This ensures that when a team is fetched, its members are also loaded, which is necessary for the subsequent logic.

src/main/java/de/tum/in/www1/artemis/web/rest/TeamResource.java (4)

• 159-159: The update to use findWithStudentsById in the updateTeam method aligns with the PR's goal to ensure that student information is fetched efficiently when required. This change is necessary for the correct functioning of the method after the switch to LAZY loading.

• 223-223: The use of findWithStudentsByIdElseThrow in the getTeam method is appropriate as it ensures that the team and its students are fetched from the database, which is necessary for the method's logic to function correctly after the switch to LAZY loading.

• 267-267: The change in the deleteTeam method to use findWithStudentsByIdElseThrow is correct and ensures that the team and its students are loaded before deletion. Additionally, the deletion of team scores before the team itself is a good practice to maintain referential integrity in the database.

• 283-284: The actual deletion of the team in the deleteTeam method is correctly placed after the deletion of team scores. This order of operations is important to avoid foreign key constraint violations in the database.

src/main/java/de/tum/in/www1/artemis/service/AuthorizationCheckService.java (3)

• 30-30: The import of TeamRepository is added to support the new functionality related to team handling.

• 57-67: The teamRepository field is properly declared and initialized in the constructor using dependency injection, which is consistent with best practices.

• 563-565: The modification in the isOwnerOfParticipation method now handles the case where the participant is a Team. It fetches the team with students before checking ownership, which aligns with the PR's objective to manage lazy loading of teams and students.

src/main/java/de/tum/in/www1/artemis/service/programming/ProgrammingSubmissionService.java (2)

• 163-164: The method findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents is introduced to fetch a ProgrammingSubmission with feedbacks and team students eagerly. This change aligns with the PR objectives to optimize database interactions by removing explicit EAGER fetching and using fetch queries where necessary. Ensure that the method is correctly implemented in the repository and that it does not introduce any performance regressions.

Verification successful

The previous script contained a syntax error. I will correct the pattern and re-run the ast-grep command to verify the existence and usage of the method findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents in the ProgrammingSubmissionRepository.

---

The ast-grep command did not yield the expected results. I will now use ripgrep (rg) to search for the method findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents in the ProgrammingSubmissionRepository.

---

The search with ripgrep has successfully located the method findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents within the ProgrammingSubmissionRepository. The method is indeed present and is being used in the ProgrammingSubmissionService.java file. Additionally, the method is defined as a default method in the repository interface, delegating to another method findByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents and using stream().findFirst().orElse(null) to return the first result or null if none is found.

This confirms that the method has been implemented in the repository and is being used in the service as intended by the PR objectives.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Verify the new repository method's existence and its usage in the codebase.
ast-grep --lang java --pattern $'ProgrammingSubmissionRepository {
  $$$
  findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents($_, $_);
  $$$
}'

Length of output: 736

---

---

Script:

#!/bin/bash
# Corrected script to verify the new repository method's existence and its usage in the codebase.
ast-grep --lang java --pattern $'class ProgrammingSubmissionRepository {
  $$$
  findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents($_, $_) {
    $$$
  }
  $$$
}'

Length of output: 283

---

---

Script:

#!/bin/bash
# Using ripgrep to search for the method in the ProgrammingSubmissionRepository.
rg "findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents" -A 3 -B 3

Length of output: 3663

* 353-354: The method `findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents` is used again to fetch a `ProgrammingSubmission` with feedbacks and team students eagerly. This is consistent with the changes made in the first hunk and should be verified for correct implementation and performance.

Verification successful

---

The search results confirm the existence of the method findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents within the ProgrammingSubmissionRepository. The method is used in the ProgrammingSubmissionService class, and there is also a default implementation of this method in the ProgrammingSubmissionRepository itself, which delegates to another method and processes the result to return the first submission or null.

Based on this information, the review comment is consistent with the codebase, as the method is indeed used to fetch a ProgrammingSubmission with feedbacks and team students eagerly.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Verify the new repository method's existence and its usage in the codebase.
ast-grep --lang java --pattern $'ProgrammingSubmissionRepository {
  $$$
  findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents($_, $_);
  $$$
}'

Length of output: 736

---

---

Script:

#!/bin/bash
# Correcting the ast-grep pattern to verify the existence and usage of the method in the ProgrammingSubmissionRepository.
ast-grep --lang java --pattern $'class ProgrammingSubmissionRepository {
  $$$
  findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents($_, $_) {
    $$$
  }
  $$$
}'

Length of output: 283

---

---

Script:

#!/bin/bash
# Using ripgrep to search for the method in the ProgrammingSubmissionRepository.
rg "findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents" -A 3 -B 3

Length of output: 3663

src/main/java/de/tum/in/www1/artemis/service/ParticipationService.java (1)

• 442-445: The change in the configureRepository method ensures that if the participant is a team, it is eagerly loaded with its students. This is necessary for the repository configuration and aligns with the PR's objective to optimize database interactions by replacing explicit EAGER fetching with LAZY fetching and fetch queries where necessary. However, it's important to ensure that this eager loading does not introduce performance regressions elsewhere in the application.

Please verify that the teamRepository.findWithStudentsByIdElseThrow(team.getId()) method is optimized for performance and does not lead to N+1 select issues or other performance bottlenecks.

src/main/java/de/tum/in/www1/artemis/repository/StudentParticipationRepository.java (1)

• 42-46: The modified query in findByCourseIdWithEagerResultsAndEagerTeamAndEagerStudents method now includes LEFT JOIN FETCH for results, team, and students. This change aligns with the PR's objective to optimize database interactions by replacing explicit EAGER fetching with LAZY fetching and fetch queries. Ensure that the rest of the application is compatible with this change, as it may affect any service or controller that relies on the data fetched by this query.

src/main/java/de/tum/in/www1/artemis/web/rest/ParticipationResource.java (1)

• 293-293: The logic here is to handle a case where there is another participation by the same student that is either inactive or finished. This is a good practice to ensure that the state of all participations is consistent. However, it's important to ensure that this logic does not introduce any side effects or race conditions, especially when dealing with the state of participations in a concurrent environment.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 4b18fe6 and 70c5ccb.

Files selected for processing (1)

• src/test/java/de/tum/in/www1/artemis/DatabaseQueryCountTest.java (2 hunks)

Files skipped from review as they are similar to previous changes (1)

• src/test/java/de/tum/in/www1/artemis/DatabaseQueryCountTest.java

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between ebdb049 and 552da8d.

Files selected for processing (1)

• src/main/java/de/tum/in/www1/artemis/repository/ProgrammingExerciseStudentParticipationRepository.java (5 hunks)

Files skipped from review as they are similar to previous changes (1)

• src/main/java/de/tum/in/www1/artemis/repository/ProgrammingExerciseStudentParticipationRepository.java

[basak-akan]

Re-tested on TS1. No further issues found, worked well 👍
Also tested for LTI result service cases, still works

[MaximilianAnzinger]

Code LGTM

[Strohgelaender]

code lgtm

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 552da8d and 4bf2fe2.

Files selected for processing (4)

• src/main/java/de/tum/in/www1/artemis/service/ResultService.java (1 hunks)
• src/main/java/de/tum/in/www1/artemis/service/programming/ProgrammingSubmissionService.java (2 hunks)
• src/main/java/de/tum/in/www1/artemis/service/programming/ProgrammingTriggerService.java (2 hunks)
• src/main/java/de/tum/in/www1/artemis/web/websocket/team/TeamWebsocketService.java (1 hunks)

Files skipped from review as they are similar to previous changes (3)

• src/main/java/de/tum/in/www1/artemis/service/ResultService.java
• src/main/java/de/tum/in/www1/artemis/service/programming/ProgrammingSubmissionService.java
• src/main/java/de/tum/in/www1/artemis/service/programming/ProgrammingTriggerService.java

Additional comments: 1

src/main/java/de/tum/in/www1/artemis/web/websocket/team/TeamWebsocketService.java (1)

• 29-29: The added comment clarifies the use cases for the sendTeamAssignmentUpdate method, which is helpful for understanding when and how to use the method.

[marlon-luca-bu]

Code looks good, successfully tested on ts1

[konrad2002]

manually tested on ts1, works as expected

[kaancayli]

Manually tested on ts6, works as expected.