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: Improve slow query performance during manual assessment #9727

Merged
merged 4 commits into from
Nov 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,6 @@ public interface ParticipantScoreRepository extends ArtemisJpaRepository<Partici
""")
List<ParticipantScore> findAllOutdated();

@Override
@EntityGraph(type = LOAD, attributePaths = { "exercise", "lastResult", "lastRatedResult" })
List<ParticipantScore> findAll();

@EntityGraph(type = LOAD, attributePaths = { "exercise", "lastResult", "lastRatedResult" })
List<ParticipantScore> findAllByExercise(Exercise exercise);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ public void importStandardizedCompetencyCatalog(StandardizedCompetencyCatalogDTO
*/
public String exportStandardizedCompetencyCatalog() {
List<KnowledgeArea> knowledgeAreas = getAllForTreeView();
// TODO: we should avoid using findAll() here, as it might return a huge amount of data
krusche marked this conversation as resolved.
Show resolved Hide resolved
krusche marked this conversation as resolved.
Show resolved Hide resolved
List<Source> sources = sourceRepository.findAll();
var catalog = StandardizedCompetencyCatalogDTO.of(knowledgeAreas, sources);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package de.tum.cit.aet.artemis.core.dto;

import com.fasterxml.jackson.annotation.JsonInclude;

@JsonInclude(JsonInclude.Include.NON_EMPTY)
public record CourseGroupsDTO(String instructorGroupName, String editorGroupName, String teachingAssistantGroupName, String studentGroupName) {
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import de.tum.cit.aet.artemis.core.domain.Organization;
import de.tum.cit.aet.artemis.core.domain.User;
import de.tum.cit.aet.artemis.core.dto.CourseForArchiveDTO;
import de.tum.cit.aet.artemis.core.dto.CourseGroupsDTO;
import de.tum.cit.aet.artemis.core.dto.StatisticsEntry;
import de.tum.cit.aet.artemis.core.exception.EntityNotFoundException;
import de.tum.cit.aet.artemis.core.repository.base.ArtemisJpaRepository;
Expand Down Expand Up @@ -323,14 +324,6 @@ GROUP BY SUBSTRING(CAST(s.submissionDate AS string), 1, 10), p.student.login
""")
List<Course> findAllNotEndedCoursesByManagementGroupNames(@Param("now") ZonedDateTime now, @Param("userGroups") List<String> userGroups);

@Query("""
SELECT COUNT(DISTINCT ug.userId)
FROM Course c
JOIN UserGroup ug ON c.studentGroupName = ug.group
WHERE c.id = :courseId
""")
int countCourseStudents(@Param("courseId") long courseId);

/**
* Counts the number of members of a course, i.e. users that are a member of the course's student, tutor, editor or instructor group.
* Users that are part of multiple groups are NOT counted multiple times.
Expand Down Expand Up @@ -569,4 +562,23 @@ SELECT COUNT(c) > 0
Set<CourseForArchiveDTO> findInactiveCoursesForUserRolesWithNonNullSemester(@Param("isAdmin") boolean isAdmin, @Param("groups") Set<String> groups,
@Param("now") ZonedDateTime now);

@Query("""
SELECT new de.tum.cit.aet.artemis.core.dto.CourseGroupsDTO(
c.instructorGroupName,
c.editorGroupName,
c.teachingAssistantGroupName,
c.studentGroupName
) FROM Course c
""")
Set<CourseGroupsDTO> findAllCourseGroups();

@Query("""
SELECT c
FROM Course c
WHERE c.teachingAssistantGroupName IN :userGroups
OR c.editorGroupName IN :userGroups
OR c.instructorGroupName IN :userGroups
OR :isAdmin = TRUE
""")
List<Course> findCoursesForAtLeastTutorWithGroups(@Param("userGroups") Set<String> userGroups, @Param("isAdmin") boolean isAdmin);
}
Original file line number Diff line number Diff line change
Expand Up @@ -678,6 +678,7 @@ public List<Course> getAllCoursesForManagementOverview(boolean onlyActive) {
var user = userRepository.getUserWithGroupsAndAuthorities();
boolean isAdmin = authCheckService.isAdmin(user);
if (isAdmin && !onlyActive) {
// TODO: we should avoid using findAll() here, as it might return a huge amount of data
krusche marked this conversation as resolved.
Show resolved Hide resolved
krusche marked this conversation as resolved.
Show resolved Hide resolved
return courseRepository.findAll();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import org.apache.sshd.common.config.keys.AuthorizedKeyEntry;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.context.annotation.Profile;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.DeleteMapping;
Expand Down Expand Up @@ -58,9 +57,6 @@ public class AccountResource {

public static final String ENTITY_NAME = "user";

@Value("${jhipster.clientApp.name}")
private String applicationName;

private static final Logger log = LoggerFactory.getLogger(AccountResource.class);

private final UserRepository userRepository;
Expand Down
133 changes: 50 additions & 83 deletions src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import jakarta.validation.constraints.NotNull;

Expand Down Expand Up @@ -99,6 +98,7 @@
import de.tum.cit.aet.artemis.core.security.annotations.EnforceAtLeastStudent;
import de.tum.cit.aet.artemis.core.security.annotations.EnforceAtLeastTutor;
import de.tum.cit.aet.artemis.core.security.annotations.enforceRoleInCourse.EnforceAtLeastEditorInCourse;
import de.tum.cit.aet.artemis.core.security.annotations.enforceRoleInCourse.EnforceAtLeastTutorInCourse;
import de.tum.cit.aet.artemis.core.service.AuthorizationCheckService;
import de.tum.cit.aet.artemis.core.service.CourseService;
import de.tum.cit.aet.artemis.core.service.FilePathService;
Expand All @@ -117,7 +117,6 @@
import de.tum.cit.aet.artemis.exercise.repository.TeamRepository;
import de.tum.cit.aet.artemis.exercise.service.ExerciseService;
import de.tum.cit.aet.artemis.exercise.service.SubmissionService;
import de.tum.cit.aet.artemis.lti.domain.OnlineCourseConfiguration;
import de.tum.cit.aet.artemis.lti.service.OnlineCourseConfigurationService;
import de.tum.cit.aet.artemis.programming.service.ci.CIUserManagementService;
import de.tum.cit.aet.artemis.programming.service.vcs.VcsUserManagementService;
Expand Down Expand Up @@ -255,16 +254,10 @@ public ResponseEntity<Course> updateCourse(@PathVariable Long courseId, @Request
// this is important, otherwise someone could put himself into the instructor group of the updated course
authCheckService.checkHasAtLeastRoleInCourseElseThrow(Role.INSTRUCTOR, existingCourse, user);

Set<String> existingGroupNames = new HashSet<>(List.of(existingCourse.getStudentGroupName(), existingCourse.getTeachingAssistantGroupName(),
existingCourse.getEditorGroupName(), existingCourse.getInstructorGroupName()));
Set<String> newGroupNames = new HashSet<>(List.of(courseUpdate.getStudentGroupName(), courseUpdate.getTeachingAssistantGroupName(), courseUpdate.getEditorGroupName(),
courseUpdate.getInstructorGroupName()));
Set<String> changedGroupNames = new HashSet<>(newGroupNames);
changedGroupNames.removeAll(existingGroupNames);

if (!authCheckService.isAdmin(user)) {
// this means the user must be an instructor, who has NO Admin rights.
// instructors are not allowed to change group names, because this would lead to security problems
final var changedGroupNames = getChangedGroupNames(courseUpdate, existingCourse);
if (!changedGroupNames.isEmpty()) {
throw new BadRequestAlertException("You are not allowed to change the group names of a course", Course.ENTITY_NAME, "groupNamesCannotChange", true);
}
Expand Down Expand Up @@ -367,48 +360,14 @@ else if (courseUpdate.getCourseIcon() == null && existingCourse.getCourseIcon()
return ResponseEntity.ok(result);
}

/**
* PUT courses/:courseId/online-course-configuration : Updates the onlineCourseConfiguration for the given course.
*
* @param courseId the id of the course to update
* @param onlineCourseConfiguration the online course configuration to update
* @return the ResponseEntity with status 200 (OK) and with body the updated online course configuration
*/
// TODO: move into LTIResource
@PutMapping("courses/{courseId}/online-course-configuration")
@EnforceAtLeastInstructor
@Profile(PROFILE_LTI)
public ResponseEntity<OnlineCourseConfiguration> updateOnlineCourseConfiguration(@PathVariable Long courseId,
@RequestBody OnlineCourseConfiguration onlineCourseConfiguration) {
log.debug("REST request to update the online course configuration for Course : {}", courseId);

Course course = courseRepository.findByIdWithEagerOnlineCourseConfigurationElseThrow(courseId);
authCheckService.checkHasAtLeastRoleInCourseElseThrow(Role.INSTRUCTOR, course, null);

if (!course.isOnlineCourse()) {
throw new BadRequestAlertException("Course must be online course", Course.ENTITY_NAME, "courseMustBeOnline");
}

if (!course.getOnlineCourseConfiguration().getId().equals(onlineCourseConfiguration.getId())) {
throw new BadRequestAlertException("The onlineCourseConfigurationId does not match the id of the course's onlineCourseConfiguration",
OnlineCourseConfiguration.ENTITY_NAME, "idMismatch");
}

if (onlineCourseConfigurationService.isPresent()) {
onlineCourseConfigurationService.get().validateOnlineCourseConfiguration(onlineCourseConfiguration);
course.setOnlineCourseConfiguration(onlineCourseConfiguration);
try {
onlineCourseConfigurationService.get().addOnlineCourseConfigurationToLtiConfigurations(onlineCourseConfiguration);
}
catch (Exception ex) {
log.error("Failed to add online course configuration to LTI configurations", ex);
throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR, "Error when adding online course configuration to LTI configurations", ex);
}
}

courseRepository.save(course);

return ResponseEntity.ok(onlineCourseConfiguration);
private static Set<String> getChangedGroupNames(Course courseUpdate, Course existingCourse) {
Set<String> existingGroupNames = new HashSet<>(List.of(existingCourse.getStudentGroupName(), existingCourse.getTeachingAssistantGroupName(),
existingCourse.getEditorGroupName(), existingCourse.getInstructorGroupName()));
Set<String> newGroupNames = new HashSet<>(List.of(courseUpdate.getStudentGroupName(), courseUpdate.getTeachingAssistantGroupName(), courseUpdate.getEditorGroupName(),
courseUpdate.getInstructorGroupName()));
Set<String> changedGroupNames = new HashSet<>(newGroupNames);
changedGroupNames.removeAll(existingGroupNames);
return changedGroupNames;
krusche marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -479,17 +438,20 @@ public ResponseEntity<Set<String>> unenrollFromCourse(@PathVariable Long courseI
@GetMapping("courses")
@EnforceAtLeastTutor
public ResponseEntity<List<Course>> getCourses(@RequestParam(defaultValue = "false") boolean onlyActive) {
log.debug("REST request to get all Courses the user has access to");
log.debug("REST request to get all courses the user has access to");
User user = userRepository.getUserWithGroupsAndAuthorities();
// TODO: we should avoid findAll() and instead try to filter this directly in the database, in case of admins, we should load batches of courses, e.g. per semester
List<Course> courses = courseRepository.findAll();
Stream<Course> userCourses = courses.stream().filter(course -> user.getGroups().contains(course.getTeachingAssistantGroupName())
|| user.getGroups().contains(course.getInstructorGroupName()) || authCheckService.isAdmin(user));
List<Course> courses = getCoursesForTutors(user, onlyActive);
return ResponseEntity.ok(courses);
}

private List<Course> getCoursesForTutors(User user, boolean onlyActive) {
List<Course> userCourses = courseRepository.findCoursesForAtLeastTutorWithGroups(user.getGroups(), authCheckService.isAdmin(user));
if (onlyActive) {
// only include courses that have NOT been finished
userCourses = userCourses.filter(course -> course.getEndDate() == null || course.getEndDate().isAfter(ZonedDateTime.now()));
final var now = ZonedDateTime.now();
userCourses = userCourses.stream().filter(course -> course.getEndDate() == null || course.getEndDate().isAfter(now)).toList();
}
return ResponseEntity.ok(userCourses.toList());
return userCourses;
}

/**
Expand Down Expand Up @@ -536,8 +498,9 @@ public ResponseEntity<List<Course>> getCoursesWithQuizExercises() {
@EnforceAtLeastTutor
public ResponseEntity<List<Course>> getCoursesWithUserStats(@RequestParam(defaultValue = "false") boolean onlyActive) {
log.debug("get courses with user stats, only active: {}", onlyActive);
// TODO: we should avoid using an endpoint in such cases and instead call a service method
List<Course> courses = getCourses(onlyActive).getBody();

User user = userRepository.getUserWithGroupsAndAuthorities();
List<Course> courses = getCoursesForTutors(user, onlyActive);
for (Course course : courses) {
course.setNumberOfInstructors(userRepository.countUserInGroup(course.getInstructorGroupName()));
course.setNumberOfTeachingAssistants(userRepository.countUserInGroup(course.getTeachingAssistantGroupName()));
Expand Down Expand Up @@ -645,11 +608,11 @@ public ResponseEntity<CourseForDashboardDTO> getCourseForDashboard(@PathVariable
}

courseService.fetchParticipationsWithSubmissionsAndResultsForCourses(List.of(course), user, true);
log.debug("courseService.fetchParticipationsWithSubmissionsAndResultsForCourses done");
log.debug("courseService.fetchParticipationsWithSubmissionsAndResultsForCourses done in getCourseForDashboard");
courseService.fetchPlagiarismCasesForCourseExercises(course.getExercises(), user.getId());
log.debug("courseService.fetchPlagiarismCasesForCourseExercises done");
log.debug("courseService.fetchPlagiarismCasesForCourseExercises done in getCourseForDashboard");
GradingScale gradingScale = gradingScaleRepository.findByCourseId(course.getId()).orElse(null);
log.debug("gradingScaleRepository.findByCourseId done");
log.debug("gradingScaleRepository.findByCourseId done in getCourseForDashboard");
CourseForDashboardDTO courseForDashboardDTO = courseScoreCalculationService.getScoresAndParticipationResults(course, gradingScale, user.getId());
logDuration(List.of(course), user, timeNanoStart, "courses/" + courseId + "/for-dashboard (single course)");
return ResponseEntity.ok(courseForDashboardDTO);
Expand Down Expand Up @@ -748,16 +711,15 @@ public ResponseEntity<Set<Course>> getCoursesForNotifications() {
* @return data about a course including all exercises, plus some data for the tutor as tutor status for assessment
*/
@GetMapping("courses/{courseId}/for-assessment-dashboard")
@EnforceAtLeastTutor
@EnforceAtLeastTutorInCourse
public ResponseEntity<Course> getCourseForAssessmentDashboard(@PathVariable long courseId) {
log.debug("REST request /courses/{courseId}/for-assessment-dashboard");
// TODO: use ...ElseThrow below in case the course cannot be found
Course course = courseRepository.findWithEagerExercisesById(courseId);
User user = userRepository.getUserWithGroupsAndAuthorities();
authCheckService.checkHasAtLeastRoleInCourseElseThrow(Role.TEACHING_ASSISTANT, course, user);
Course course = courseRepository.findByIdWithEagerExercisesElseThrow(courseId);

Set<Exercise> interestingExercises = courseRepository.filterInterestingExercisesForAssessmentDashboards(course.getExercises());
course.setExercises(interestingExercises);

User user = userRepository.getUser();
List<TutorParticipation> tutorParticipations = tutorParticipationRepository.findAllByAssessedExercise_Course_IdAndTutor_Id(course.getId(), user.getId());
assessmentDashboardService.generateStatisticsForExercisesForAssessmentDashboard(course.getExercises(), tutorParticipations, false);
return ResponseEntity.ok(course);
Expand Down Expand Up @@ -790,7 +752,7 @@ public ResponseEntity<StatsForDashboardDTO> getStatsForAssessmentDashboard(@Path
@GetMapping("courses/{courseId}")
@EnforceAtLeastStudent
public ResponseEntity<Course> getCourse(@PathVariable Long courseId) {
log.debug("REST request to get Course : {}", courseId);
log.debug("REST request to get course {} for students", courseId);
Course course = courseRepository.findByIdElseThrow(courseId);

User user = userRepository.getUserWithGroupsAndAuthorities();
Expand Down Expand Up @@ -822,7 +784,7 @@ else if (authCheckService.isAtLeastTeachingAssistantInCourse(course, user)) {
@GetMapping("courses/{courseId}/with-exercises")
@EnforceAtLeastTutor
public ResponseEntity<Course> getCourseWithExercises(@PathVariable Long courseId) {
log.debug("REST request to get Course : {}", courseId);
log.debug("REST request to get course {} for tutors", courseId);
Course course = courseRepository.findWithEagerExercisesById(courseId);
authCheckService.checkHasAtLeastRoleInCourseElseThrow(Role.TEACHING_ASSISTANT, course, null);
return ResponseEntity.ok(course);
Expand Down Expand Up @@ -1069,20 +1031,9 @@ public ResponseEntity<List<UserPublicInfoDTO>> searchUsersInCourse(@PathVariable
if (loginOrName.length() < 3 && requestedRoles.contains(Role.STUDENT)) {
throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Query param 'loginOrName' must be three characters or longer if you search for students.");
}
var groups = new HashSet<String>();
if (requestedRoles.contains(Role.STUDENT)) {
groups.add(course.getStudentGroupName());
}
if (requestedRoles.contains(Role.TEACHING_ASSISTANT)) {
groups.add(course.getTeachingAssistantGroupName());
// searching for tutors also searches for editors
groups.add(course.getEditorGroupName());
}
if (requestedRoles.contains(Role.INSTRUCTOR)) {
groups.add(course.getInstructorGroupName());
}
final var relevantCourseGroupNames = getRelevantCourseGroupNames(requestedRoles, course);
User searchingUser = userRepository.getUser();
var originalPage = userRepository.searchAllWithGroupsByLoginOrNameInGroupsNotUserId(PageRequest.of(0, 25), loginOrName, groups, searchingUser.getId());
var originalPage = userRepository.searchAllWithGroupsByLoginOrNameInGroupsNotUserId(PageRequest.of(0, 25), loginOrName, relevantCourseGroupNames, searchingUser.getId());

var resultDTOs = new ArrayList<UserPublicInfoDTO>();
for (var user : originalPage) {
Expand All @@ -1097,6 +1048,22 @@ public ResponseEntity<List<UserPublicInfoDTO>> searchUsersInCourse(@PathVariable
return new ResponseEntity<>(dtoPage.getContent(), headers, HttpStatus.OK);
}

private static HashSet<String> getRelevantCourseGroupNames(Set<Role> requestedRoles, Course course) {
var groups = new HashSet<String>();
if (requestedRoles.contains(Role.STUDENT)) {
groups.add(course.getStudentGroupName());
}
if (requestedRoles.contains(Role.TEACHING_ASSISTANT)) {
groups.add(course.getTeachingAssistantGroupName());
// searching for tutors also searches for editors
groups.add(course.getEditorGroupName());
}
if (requestedRoles.contains(Role.INSTRUCTOR)) {
groups.add(course.getInstructorGroupName());
}
return groups;
}
krusche marked this conversation as resolved.
Show resolved Hide resolved

/**
* GET /courses/:courseId/tutors : Returns all users that belong to the tutor group of the course
*
Expand Down
Loading
Loading