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 all 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 @@ -48,8 +48,8 @@ SELECT max(prr.id)
Optional<ProgrammingExerciseStudentParticipation> findByIdWithLatestResultAndFeedbacksAndRelatedSubmissions(@Param("participationId") Long participationId,
@Param("dateTime") ZonedDateTime dateTime);

@EntityGraph(type = LOAD, attributePaths = { "results", "exercise" })
List<ProgrammingExerciseStudentParticipation> findByBuildPlanId(String buildPlanId);
@EntityGraph(type = LOAD, attributePaths = { "results", "exercise", "team.students" })
List<ProgrammingExerciseStudentParticipation> findWithResultsAndExerciseAndTeamStudentsByBuildPlanId(String buildPlanId);

@Query("""
SELECT DISTINCT p
Expand Down 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 Expand Up @@ -110,6 +111,9 @@ Optional<ProgrammingExerciseStudentParticipation> findWithSubmissionsAndEagerStu
@EntityGraph(type = LOAD, attributePaths = { "submissions" })
List<ProgrammingExerciseStudentParticipation> findWithSubmissionsByExerciseId(Long exerciseId);

@EntityGraph(type = LOAD, attributePaths = { "submissions", "team.students" })
List<ProgrammingExerciseStudentParticipation> findWithSubmissionsAndTeamStudentsByExerciseId(Long exerciseId);

/**
* Will return the participations matching the provided participation ids, but only if they belong to the given exercise.
*
Expand Down Expand Up @@ -151,6 +155,9 @@ Optional<ProgrammingExerciseStudentParticipation> findWithSubmissionsByExerciseI
@EntityGraph(type = LOAD, attributePaths = "student")
Optional<ProgrammingExerciseStudentParticipation> findWithStudentById(Long participationId);

@EntityGraph(type = LOAD, attributePaths = "team.students")
Optional<ProgrammingExerciseStudentParticipation> findWithTeamStudentsById(Long participationId);
MaximilianAnzinger marked this conversation as resolved.
Show resolved Hide resolved

@NotNull
default ProgrammingExerciseStudentParticipation findByIdElseThrow(Long participationId) {
return findById(participationId).orElseThrow(() -> new EntityNotFoundException("Programming Exercise Student Participation", participationId));
Expand All @@ -160,6 +167,10 @@ default Optional<ProgrammingExerciseStudentParticipation> findStudentParticipati
return findByIdWithLatestResultAndFeedbacksAndRelatedSubmissions(participationId, ZonedDateTime.now());
}

default ProgrammingExerciseStudentParticipation findWithTeamStudentsByIdElseThrow(Long participationId) {
return findWithTeamStudentsById(participationId).orElseThrow(() -> new EntityNotFoundException("Programming Exercise Student Participation", participationId));
}

@Transactional // ok because of modifying query
@Modifying
@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 Expand Up @@ -127,11 +131,13 @@ Optional<StudentParticipation> findWithEagerLegalSubmissionsByExerciseIdAndStude
@Query("""
SELECT DISTINCT p FROM StudentParticipation p
LEFT JOIN FETCH p.submissions s
LEFT JOIN FETCH p.team t
LEFT JOIN FETCH t.students
where p.exercise.id = :#{#exerciseId}
AND p.team.id = :#{#teamId}
AND (s.type <> 'ILLEGAL' OR s.type IS NULL)
""")
Optional<StudentParticipation> findWithEagerLegalSubmissionsByExerciseIdAndTeamId(@Param("exerciseId") Long exerciseId, @Param("teamId") Long teamId);
Optional<StudentParticipation> findWithEagerLegalSubmissionsAndTeamStudentsByExerciseIdAndTeamId(@Param("exerciseId") Long exerciseId, @Param("teamId") Long teamId);

@Query("""
SELECT DISTINCT p FROM StudentParticipation p
Expand Down Expand Up @@ -362,16 +368,19 @@ default List<StudentParticipation> findByExerciseIdWithManualResultAndFeedbacksA
List<StudentParticipation> findAllByExerciseIdAndTeamId(@Param("exerciseId") Long exerciseId, @Param("teamId") Long teamId);

@Query("""
select distinct p from StudentParticipation p
left join fetch p.results r
left join fetch r.submission rs
left join fetch p.submissions s
SELECT DISTINCT p
FROM StudentParticipation p
LEFT JOIN FETCH p.results r
LEFT JOIN FETCH r.submission rs
LEFT JOIN FETCH p.submissions s
LEFT JOIN FETCH p.team t
LEFT JOIN FETCH t.students
where p.exercise.id = :#{#exerciseId}
and p.team.id = :#{#teamId}
and (s.type <> 'ILLEGAL' or s.type is null)
and (rs.type <> 'ILLEGAL' or rs.type is null)
""")
List<StudentParticipation> findByExerciseIdAndTeamIdWithEagerResultsAndLegalSubmissions(@Param("exerciseId") Long exerciseId, @Param("teamId") Long teamId);
List<StudentParticipation> findByExerciseIdAndTeamIdWithEagerResultsAndLegalSubmissionsAndTeamStudents(@Param("exerciseId") Long exerciseId, @Param("teamId") Long teamId);

@Query("""
SELECT DISTINCT p
Expand Down Expand Up @@ -460,10 +469,13 @@ List<StudentParticipation> findByExerciseIdWithLatestSubmissionWithoutManualResu
Optional<StudentParticipation> findWithEagerLegalSubmissionsById(@Param("participationId") Long participationId);

@Query("""
select p from Participation p
left join fetch p.results r
left join fetch r.submission
where p.id = :#{#participationId}
SELECT p
FROM StudentParticipation p
LEFT JOIN FETCH p.results r
LEFT JOIN FETCH r.submission
LEFT JOIN FETCH p.team t
LEFT JOIN FETCH t.students
WHERE p.id = :participationId
""")
Optional<StudentParticipation> findWithEagerResultsById(@Param("participationId") Long participationId);

Expand Down Expand Up @@ -698,7 +710,7 @@ SELECT p.id, COUNT(s) FROM StudentParticipation p
LEFT JOIN FETCH s.results r
WHERE p.exercise.id = :#{#exerciseId}
AND p.testRun = FALSE
AND s.id = (SELECT max(id) FROM p.submissions)
AND s.id = (SELECT max(s1.id) FROM p.submissions s1)
AND EXISTS (SELECT s1 FROM p.submissions s1
WHERE s1.participation.id = p.id
AND s1.submitted = 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,8 +26,12 @@
@Repository
public interface TeamRepository extends JpaRepository<Team, Long> {

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

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

List<Team> findAllByExerciseCourseIdAndShortName(@Param("courseId") Long courseId, @Param("shortName") String shortName);

/**
Expand All @@ -36,7 +43,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 +67,8 @@ 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);
@EntityGraph(type = LOAD, attributePaths = "students")
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 +105,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 +130,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 @@ -6,7 +6,6 @@
import java.util.Optional;
import java.util.Set;

import org.springframework.data.domain.Pageable;
import org.springframework.data.jpa.repository.EntityGraph;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.Modifying;
Expand All @@ -27,22 +26,11 @@ public interface TeamScoreRepository extends JpaRepository<TeamScore, Long> {
@Modifying
void deleteAllByTeamId(long teamId);

@EntityGraph(type = LOAD, attributePaths = { "team", "exercise" })
@EntityGraph(type = LOAD, attributePaths = { "team.students", "exercise" })
Optional<TeamScore> findByExercise_IdAndTeam_Id(@Param("exerciseId") Long exerciseId, @Param("teamId") Long teamId);

@EntityGraph(type = LOAD, attributePaths = { "team", "exercise", "lastResult", "lastRatedResult" })
List<TeamScore> findAllByExerciseIn(Set<Exercise> exercises, Pageable pageable);

@Query("""
SELECT DISTINCT s
FROM TeamScore s
WHERE s.exercise = :exercise
AND :user MEMBER OF s.team.students
""")
Optional<TeamScore> findTeamScoreByExerciseAndUserLazy(@Param("exercise") Exercise exercise, @Param("user") User user);

@Query("""
SELECT t, SUM(s.lastRatedPoints)
SELECT t.id, SUM(s.lastRatedPoints)
FROM TeamScore s
LEFT JOIN s.team t
WHERE s.exercise IN :exercises
Expand All @@ -54,16 +42,20 @@ SELECT t, SUM(s.lastRatedPoints)
SELECT s
FROM TeamScore s
LEFT JOIN FETCH s.exercise
LEFT JOIN FETCH s.team t
LEFT JOIN FETCH t.students
WHERE s.exercise IN :exercises
AND :user MEMBER OF s.team.students
AND :user MEMBER OF t.students
""")
List<TeamScore> findAllByExerciseAndUserWithEagerExercise(@Param("exercises") Set<Exercise> exercises, @Param("user") User user);

@Query("""
SELECT s
FROM TeamScore s
LEFT JOIN FETCH s.team t
LEFT JOIN FETCH t.students
WHERE s.exercise IN :exercises
AND :user MEMBER OF s.team.students
AND :user MEMBER OF t.students
""")
List<TeamScore> findAllByExercisesAndUser(@Param("exercises") Set<Exercise> exercises, @Param("user") User user);

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 @@ -555,9 +559,12 @@ public boolean isOwnerOfParticipation(@NotNull StudentParticipation participatio
if (participation.getParticipant() == null) {
return false;
}
else {
return participation.isOwnedBy(user);

if (participation.getParticipant() instanceof Team team && !Hibernate.isInitialized(team.getStudents())) {
participation.setParticipant(teamRepository.findWithStudentsByIdElseThrow(team.getId()));
}

return participation.isOwnedBy(user);
}

/**
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
Loading
Loading