Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Development: Remove eager relationship between team and students #7829

Merged
merged 29 commits into from
Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
f902654
change to lazy
Dec 27, 2023
6ec6622
fix issues with lazy loading
Dec 27, 2023
f5fe11d
fix exceptions
Dec 27, 2023
df1bdb8
fix some loading of team students
julian-christl Jan 5, 2024
3c18642
fix lazy loading in scheduling
julian-christl Jan 5, 2024
4b18fe6
Merge branch 'develop' into chore/remove-team-eager
julian-christl Jan 6, 2024
00514e6
fix course db call assertions
julian-christl Jan 6, 2024
70c5ccb
Merge branch 'develop' into chore/remove-team-eager
julian-christl Jan 6, 2024
3c06fcc
Merge branch 'develop' into chore/remove-team-eager
julian-christl Jan 8, 2024
d6d54bd
cleaned code, code review
julian-christl Jan 8, 2024
ee19511
revert query replacement
julian-christl Jan 8, 2024
091dc37
revert and fix eager loading
julian-christl Jan 8, 2024
7e392e0
remove redundant import
julian-christl Jan 8, 2024
c330a2c
modify queries
julian-christl Jan 9, 2024
5450aa4
fix team student load for programming complaints
Jan 11, 2024
4f55cd7
Merge branch 'develop' into chore/remove-team-eager
julian-christl Jan 11, 2024
74c0160
coderabbit cr
julian-christl Jan 11, 2024
01deefb
fix naming
julian-christl Jan 11, 2024
b82d3f7
fix team student load for scores
julian-christl Jan 12, 2024
20aeb17
Merge branch 'develop' into chore/remove-team-eager
julian-christl Jan 12, 2024
94f7c63
Merge branch 'develop' into chore/remove-team-eager
krusche Jan 13, 2024
f5ad980
fix team score queries
julian-christl Jan 16, 2024
87cd341
try explicit fetch
julian-christl Jan 16, 2024
88e3ca9
revert and fix changes in team scores
julian-christl Jan 16, 2024
18afef4
Merge branch 'develop' into chore/remove-team-eager
julian-christl Jan 16, 2024
154b127
fix more lazy loading issues, improve naming
julian-christl Jan 17, 2024
ebdb049
simplify and tryfix broadcast
julian-christl Jan 17, 2024
552da8d
fix entity graph
julian-christl Jan 17, 2024
4bf2fe2
Merge branch 'develop' into chore/remove-team-eager
julian-christl Jan 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/de/tum/in/www1/artemis/domain/Team.java
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public class Team extends AbstractAuditingEntity implements Participant {
* The cache concurrency strategy needs to be READ_WRITE (and not NONSTRICT_READ_WRITE) since the non-strict mode will cause an SQLIntegrityConstraintViolationException to
* occur when trying to persist the entity for the first time after changes have been made to the related entities of the ManyToMany relationship (users in this case).
*/
@ManyToMany(fetch = FetchType.EAGER)
@ManyToMany(fetch = FetchType.LAZY)
@Cache(usage = CacheConcurrencyStrategy.NONSTRICT_READ_WRITE)
@JsonInclude(JsonInclude.Include.NON_NULL)
@JoinTable(name = "team_student", joinColumns = @JoinColumn(name = "team_id", referencedColumnName = "id"), inverseJoinColumns = @JoinColumn(name = "student_id", referencedColumnName = "id"))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,15 @@ default Participation findByIdWithLegalSubmissionsElseThrow(long participationId
SELECT p
FROM Participation p
LEFT JOIN FETCH p.submissions s
LEFT JOIN FETCH p.team t
LEFT JOIN FETCH t.students
WHERE p.id = :participationId
""")
Optional<Participation> findWithEagerSubmissionsById(@Param("participationId") Long participationId);
Optional<Participation> findWithEagerSubmissionsByIdWithTeamStudents(@Param("participationId") Long participationId);

@NotNull
default Participation findByIdWithSubmissionsElseThrow(long participationId) {
return findWithEagerSubmissionsById(participationId).orElseThrow(() -> new EntityNotFoundException("Participation", participationId));
default Participation findWithEagerSubmissionsByIdWithTeamStudentsElseThrow(long participationId) {
return findWithEagerSubmissionsByIdWithTeamStudents(participationId).orElseThrow(() -> new EntityNotFoundException("Participation", participationId));
}

@NotNull
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ default ProgrammingExerciseStudentParticipation findWithSubmissionsByExerciseIdA

Optional<ProgrammingExerciseStudentParticipation> findByExerciseIdAndStudentLoginAndTestRun(Long exerciseId, String username, boolean testRun);

@EntityGraph(type = LOAD, attributePaths = { "team" })
Optional<ProgrammingExerciseStudentParticipation> findByExerciseIdAndTeamId(Long exerciseId, Long teamId);

@Query("""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,23 @@ default ProgrammingSubmission findByIdElseThrow(long submissionId) {
return findById(submissionId).orElseThrow(() -> new EntityNotFoundException("ProgrammingSubmission", submissionId));
}

@EntityGraph(type = LOAD, attributePaths = { "results.feedbacks" })
ProgrammingSubmission findFirstByParticipationIdAndCommitHashOrderByIdDesc(Long participationId, String commitHash);
@Query("""
SELECT s
FROM ProgrammingSubmission s
LEFT JOIN FETCH s.results r
LEFT JOIN FETCH r.feedbacks f
LEFT JOIN FETCH s.participation p
LEFT JOIN FETCH p.team t
LEFT JOIN FETCH t.students
WHERE p.id = :participationId
AND s.commitHash = :commitHash
ORDER BY s.id DESC
""")
List<ProgrammingSubmission> findByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents(Long participationId, String commitHash);

default ProgrammingSubmission findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents(Long participationId, String commitHash) {
return findByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents(participationId, commitHash).stream().findFirst().orElse(null);
}

@Query("""
SELECT s FROM ProgrammingSubmission s
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,11 @@ public interface StudentParticipationRepository extends JpaRepository<StudentPar
Set<StudentParticipation> findByExerciseId(@Param("exerciseId") Long exerciseId);

@Query("""
SELECT DISTINCT p FROM StudentParticipation p LEFT JOIN FETCH p.results r
SELECT DISTINCT p
FROM StudentParticipation p
LEFT JOIN FETCH p.results r
LEFT JOIN FETCH p.team t
LEFT JOIN FETCH t.students
WHERE p.exercise.course.id = :#{#courseId}
AND (r.rated IS NULL OR r.rated = true)
""")
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
package de.tum.in.www1.artemis.repository;

import static org.springframework.data.jpa.repository.EntityGraph.EntityGraphType.LOAD;

import java.time.Instant;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

import org.springframework.data.jpa.repository.EntityGraph;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.Query;
import org.springframework.data.repository.query.Param;
Expand All @@ -23,6 +26,7 @@
@Repository
public interface TeamRepository extends JpaRepository<Team, Long> {

@EntityGraph(type = LOAD, attributePaths = "students")
List<Team> findAllByExerciseId(@Param("exerciseId") Long exerciseId);

List<Team> findAllByExerciseCourseIdAndShortName(@Param("courseId") Long courseId, @Param("shortName") String shortName);
Expand All @@ -36,7 +40,7 @@ public interface TeamRepository extends JpaRepository<Team, Long> {
@Query("""
SELECT COUNT(DISTINCT team)
FROM Team team
WHERE team.exercise.id = :#{#exerciseId}
WHERE team.exercise.id = :exerciseId
""")
Integer getNumberOfTeamsForExercise(@Param("exerciseId") Long exerciseId);

Expand All @@ -60,8 +64,13 @@ SELECT COUNT(DISTINCT team)
@Query(value = "select distinct team from Team team left join fetch team.students where team.exercise.id = :#{#exerciseId} and team.owner.id = :#{#teamOwnerId}")
List<Team> findAllByExerciseIdAndTeamOwnerIdWithEagerStudents(@Param("exerciseId") long exerciseId, @Param("teamOwnerId") long teamOwnerId);

@Query("select team from Team team left join fetch team.students where team.id = :#{#teamId}")
Optional<Team> findOneWithEagerStudents(@Param("teamId") Long teamId);
@Query("""
SELECT team
FROM Team team
LEFT JOIN FETCH team.students
WHERE team.id = :teamId
""")
Optional<Team> findWithStudentsById(@Param("teamId") Long teamId);
julian-christl marked this conversation as resolved.
Show resolved Hide resolved

/**
* Returns all teams for an exercise (optionally filtered for a specific tutor who owns the teams)
Expand Down Expand Up @@ -98,7 +107,8 @@ default Team save(Exercise exercise, Team team) {
team.setLastModifiedDate(Instant.now());
}
team.setExercise(exercise);
return save(team);
team = save(team);
return findWithStudentsByIdElseThrow(team.getId());
julian-christl marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +108 to +109
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method save is refetching the team after saving it to ensure the students are loaded. This could potentially be optimized by manually setting the already loaded students to avoid an additional database call.

// Suggested change to avoid refetching from the database
team = save(team);
team.setStudents(new HashSet<>(team.getStudents()));
return team;

}

/**
Expand All @@ -122,4 +132,8 @@ private List<Pair<User, Team>> findStudentTeamConflicts(Exercise exercise, Team
default Team findByIdElseThrow(long teamId) throws EntityNotFoundException {
return findById(teamId).orElseThrow(() -> new EntityNotFoundException("Team", teamId));
}

default Team findWithStudentsByIdElseThrow(long teamId) throws EntityNotFoundException {
return findWithStudentsById(teamId).orElseThrow(() -> new EntityNotFoundException("Team", teamId));
}
julian-christl marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,11 @@ public interface TeamScoreRepository extends JpaRepository<TeamScore, Long> {
SELECT t, SUM(s.lastRatedPoints)
FROM TeamScore s
LEFT JOIN s.team t
LEFT JOIN FETCH t.students
WHERE s.exercise IN :exercises
GROUP BY t.id
""")
List<Object[]> getAchievedPointsOfTeams(@Param("exercises") Set<Exercise> exercises);
List<Object[]> getAchievedPointsOfTeamsWithStudents(@Param("exercises") Set<Exercise> exercises);

@Query("""
SELECT s
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import de.tum.in.www1.artemis.domain.lecture.LectureUnit;
import de.tum.in.www1.artemis.domain.participation.StudentParticipation;
import de.tum.in.www1.artemis.repository.CourseRepository;
import de.tum.in.www1.artemis.repository.TeamRepository;
import de.tum.in.www1.artemis.repository.UserRepository;
import de.tum.in.www1.artemis.security.Role;
import de.tum.in.www1.artemis.security.SecurityUtils;
Expand All @@ -53,14 +54,17 @@ public class AuthorizationCheckService {
@Value("${artemis.user-management.course-enrollment.allowed-username-pattern:#{null}}")
private Pattern allowedCourseEnrollmentUsernamePattern;

public AuthorizationCheckService(UserRepository userRepository, CourseRepository courseRepository, ExamDateService examDateService) {
private final TeamRepository teamRepository;

public AuthorizationCheckService(UserRepository userRepository, CourseRepository courseRepository, ExamDateService examDateService, TeamRepository teamRepository) {
this.userRepository = userRepository;
this.courseRepository = courseRepository;
this.examDateService = examDateService;

if (allowedCourseEnrollmentUsernamePattern == null) {
allowedCourseEnrollmentUsernamePattern = allowedCourseRegistrationUsernamePattern;
}
this.teamRepository = teamRepository;
}

/**
Expand Down Expand Up @@ -556,6 +560,9 @@ public boolean isOwnerOfParticipation(@NotNull StudentParticipation participatio
return false;
}
else {
if (participation.getParticipant() instanceof Team team) {
participation.setParticipant(teamRepository.findWithStudentsByIdElseThrow(team.getId()));
}
return participation.isOwnedBy(user);
}
julian-christl marked this conversation as resolved.
Show resolved Hide resolved
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,16 @@ public class ComplaintService {

private final ExamRepository examRepository;

private final TeamRepository teamRepository;

public ComplaintService(ComplaintRepository complaintRepository, ComplaintResponseRepository complaintResponseRepository, ResultRepository resultRepository,
ExamRepository examRepository, UserRepository userRepository) {
ExamRepository examRepository, UserRepository userRepository, TeamRepository teamRepository) {
this.complaintRepository = complaintRepository;
this.complaintResponseRepository = complaintResponseRepository;
this.resultRepository = resultRepository;
this.examRepository = examRepository;
this.userRepository = userRepository;
this.teamRepository = teamRepository;
}

/**
Expand Down Expand Up @@ -97,6 +100,10 @@ else if (complaint.getComplaintType() == ComplaintType.MORE_FEEDBACK && !course.
validateTimeOfComplaintOrRequestMoreFeedback(originalResult, studentParticipation.getExercise(), studentParticipation, course, complaint.getComplaintType());
}

if (studentParticipation.getParticipant() instanceof Team team) {
studentParticipation.setParticipant(teamRepository.findWithStudentsByIdElseThrow(team.getId()));
}
aplr marked this conversation as resolved.
Show resolved Hide resolved

if (!studentParticipation.isOwnedBy(principal.getName())) {
throw new BadRequestAlertException("You can create a complaint only for a result you submitted", ENTITY_NAME, "differentuser");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ private List<ScoreDTO> calculateScores(Set<Exercise> exercises, Set<User> users,
// team exercises
// [0] -> Team
// [1] -> sum of achieved points in exercises
List<Object[]> teamAndAchievedPoints = teamScoreRepository.getAchievedPointsOfTeams(teamExercises);
List<Object[]> teamAndAchievedPoints = teamScoreRepository.getAchievedPointsOfTeamsWithStudents(teamExercises);
for (Object[] rawData : teamAndAchievedPoints) {
Team team = (Team) rawData[0];
double achievedPoints = rawData[1] != null ? ((Number) rawData[1]).doubleValue() : 0.0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,10 @@
import org.slf4j.LoggerFactory;
import org.springframework.stereotype.Service;

import de.tum.in.www1.artemis.domain.Course;
import de.tum.in.www1.artemis.domain.ProgrammingExercise;
import de.tum.in.www1.artemis.domain.User;
import de.tum.in.www1.artemis.domain.*;
import de.tum.in.www1.artemis.domain.participation.*;
import de.tum.in.www1.artemis.repository.ProgrammingExerciseRepository;
import de.tum.in.www1.artemis.repository.TeamRepository;
import de.tum.in.www1.artemis.repository.UserRepository;
import de.tum.in.www1.artemis.web.rest.errors.AccessForbiddenException;

Expand All @@ -25,12 +24,15 @@ public class ParticipationAuthorizationCheckService {

private final AuthorizationCheckService authCheckService;

private final TeamRepository teamRepository;

public ParticipationAuthorizationCheckService(UserRepository userRepository, ProgrammingExerciseRepository programmingExerciseRepository,
AuthorizationCheckService authCheckService) {
AuthorizationCheckService authCheckService, TeamRepository teamRepository) {
this.userRepository = userRepository;
this.programmingExerciseRepository = programmingExerciseRepository;

this.authCheckService = authCheckService;
this.teamRepository = teamRepository;
}

/**
Expand Down Expand Up @@ -68,6 +70,10 @@ public boolean canAccessParticipation(@NotNull final ParticipationInterface part
* @return True, if the user is allowed to access the participation; false otherwise.
*/
public boolean canAccessParticipation(@NotNull final ParticipationInterface participation, final User user) {
if (participation instanceof StudentParticipation studentParticipation && studentParticipation.getParticipant() instanceof Team team) {
// eager load the team with students so their information can be used for the access check below
studentParticipation.setParticipant(teamRepository.findWithStudentsByIdElseThrow(team.getId()));
}
julian-christl marked this conversation as resolved.
Show resolved Hide resolved
if (participation instanceof ProgrammingExerciseParticipation programmingExerciseParticipation) {
return canAccessProgrammingParticipation(programmingExerciseParticipation, user);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,10 @@ private ProgrammingExerciseStudentParticipation configureRepository(ProgrammingE
if (!participation.getInitializationState().hasCompletedState(InitializationState.REPO_CONFIGURED)) {
// do not allow the student to access the repository if this is an exam exercise that has not started yet
boolean allowAccess = !exercise.isExamExercise() || ZonedDateTime.now().isAfter(exercise.getParticipationStartDate());
if (participation.getParticipant() instanceof Team team) {
// eager load the team with students so their information can be used for the repository configuration
participation.setParticipant(teamRepository.findWithStudentsByIdElseThrow(team.getId()));
}
versionControlService.orElseThrow().configureRepository(exercise, participation, allowAccess);
participation.setInitializationState(InitializationState.REPO_CONFIGURED);
return programmingExerciseStudentParticipationRepository.saveAndFlush(participation);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ public void updateRepositoryMembersIfNeeded(Long exerciseId, Team existingTeam,
var optionalParticipation = programmingExerciseStudentParticipationRepository.findByExerciseIdAndTeamId(exerciseId, existingTeam.getId());

optionalParticipation.ifPresent(participation -> {

// Users in the existing team that are no longer in the updated team need to be removed
Set<User> usersToRemove = new HashSet<>(existingTeam.getStudents());
usersToRemove.removeAll(updatedTeam.getStudents());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import de.tum.in.www1.artemis.domain.*;
import de.tum.in.www1.artemis.domain.participation.*;
import de.tum.in.www1.artemis.repository.TeamRepository;
import de.tum.in.www1.artemis.service.WebsocketMessagingService;
import de.tum.in.www1.artemis.service.connectors.lti.LtiNewResultService;
import de.tum.in.www1.artemis.service.notifications.GroupNotificationService;
Expand All @@ -30,12 +31,15 @@ public class ProgrammingMessagingService {

private final Optional<LtiNewResultService> ltiNewResultService;

private final TeamRepository teamRepository;

public ProgrammingMessagingService(GroupNotificationService groupNotificationService, WebsocketMessagingService websocketMessagingService,
ResultWebsocketService resultWebsocketService, Optional<LtiNewResultService> ltiNewResultService) {
ResultWebsocketService resultWebsocketService, Optional<LtiNewResultService> ltiNewResultService, TeamRepository teamRepository) {
this.groupNotificationService = groupNotificationService;
this.websocketMessagingService = websocketMessagingService;
this.resultWebsocketService = resultWebsocketService;
this.ltiNewResultService = ltiNewResultService;
this.teamRepository = teamRepository;
}

public void notifyInstructorAboutStartedExerciseBuildRun(ProgrammingExercise programmingExercise) {
Expand Down Expand Up @@ -81,6 +85,10 @@ public void notifyUserAboutSubmissionError(ProgrammingSubmission submission, Bui
*/
public void notifyUserAboutSubmissionError(Participation participation, BuildTriggerWebsocketError error) {
if (participation instanceof StudentParticipation studentParticipation) {
if (studentParticipation.getParticipant() instanceof Team team) {
// eager load the team with students so their information can be used for the messages below
studentParticipation.setParticipant(teamRepository.findWithStudentsByIdElseThrow(team.getId()));
}
studentParticipation.getStudents().forEach(user -> websocketMessagingService.sendMessageToUser(user.getLogin(), NEW_SUBMISSION_TOPIC, error));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,8 @@ public ProgrammingSubmission processNewProgrammingSubmission(ProgrammingExercise
}

// There can't be two submissions for the same participation and commitHash!
ProgrammingSubmission programmingSubmission = programmingSubmissionRepository.findFirstByParticipationIdAndCommitHashOrderByIdDesc(participation.getId(),
commit.commitHash());
ProgrammingSubmission programmingSubmission = programmingSubmissionRepository
.findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents(participation.getId(), commit.commitHash());
if (programmingSubmission != null) {
throw new IllegalStateException("Submission for participation id " + participation.getId() + " and commitHash " + commit.commitHash() + " already exists!");
}
julian-christl marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -350,7 +350,8 @@ public ProgrammingSubmission getOrCreateSubmissionWithLastCommitHashForParticipa
throws IllegalStateException {
String lastCommitHash = getLastCommitHashForParticipation(participation);
// we first try to get an existing programming submission with the last commit hash
var programmingSubmission = programmingSubmissionRepository.findFirstByParticipationIdAndCommitHashOrderByIdDesc(participation.getId(), lastCommitHash);
var programmingSubmission = programmingSubmissionRepository.findFirstByParticipationIdAndCommitHashOrderByIdDescWithFeedbacksAndTeamStudents(participation.getId(),
lastCommitHash);
// in case no programming submission is available, we create one
return Objects.requireNonNullElseGet(programmingSubmission, () -> createSubmissionWithCommitHashAndSubmissionType(participation, lastCommitHash, submissionType));
}
Expand Down
Loading
Loading