Lectures: Fix edge cases that can lead to invalid lecture unit configurations (ls1intum#11648)

Author: krusche

docs/dev/setup/server.rst

@@ -47,7 +47,6 @@ You only need to modify them if your specific work or production environments re
        user-management:
            use-external: true
            password-reset:
-                credential-provider: <provider>  # Example: TUMonline
                 links:
                     en: '<link>'
                     de: '<link>'

src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyProgressApi.java

@@ -45,6 +45,10 @@ public void updateProgressForUpdatedLearningObjectAsync(LearningObject originalL
         competencyProgressService.updateProgressForUpdatedLearningObjectAsync(originalLearningObject, updatedLearningObject);
     }
 
+    public void updateProgressForUpdatedLearningObjectAsyncWithOriginalCompetencyIds(Set<Long> originalCompetencyIds, LearningObject updatedLearningObject) {
+        competencyProgressService.updateProgressForUpdatedLearningObjectAsyncWithOriginalCompetencyIds(originalCompetencyIds, updatedLearningObject);
+    }
+
     public void updateProgressByLearningObjectSync(LearningObject learningObject, Set<User> users) {
         competencyProgressService.updateProgressByLearningObjectSync(learningObject, users);
     }

src/main/java/de/tum/cit/aet/artemis/atlas/api/CompetencyRepositoryApi.java

@@ -24,4 +24,8 @@ public CompetencyRepositoryApi(CompetencyRepository competencyRepository) {
     public Set<Competency> findAllByCourseId(long courseId) {
         return competencyRepository.findAllByCourseId(courseId);
     }
+
+    public Competency findByIdElseThrow(long competencyId) {
+        return competencyRepository.findByIdElseThrow(competencyId);
+    }
 }

src/main/java/de/tum/cit/aet/artemis/atlas/domain/competency/CompetencyLectureUnitLink.java

@@ -16,6 +16,7 @@
 import org.hibernate.annotations.CacheConcurrencyStrategy;
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 
 import de.tum.cit.aet.artemis.lecture.domain.LectureUnit;
 
@@ -28,6 +29,7 @@ public class CompetencyLectureUnitLink extends CompetencyLearningObjectLink {
     @JsonIgnore
     protected CompetencyLectureUnitId id = new CompetencyLectureUnitId();
 
+    @JsonIgnoreProperties("competencyLinks")
     @ManyToOne(optional = false, cascade = CascadeType.PERSIST)
     @MapsId("lectureUnitId")
     private LectureUnit lectureUnit;

src/main/java/de/tum/cit/aet/artemis/atlas/service/LearningObjectImportService.java

@@ -338,16 +338,15 @@ private void importOrLoadLectureUnit(CompetencyLectureUnitLink sourceLectureUnit
 
         LectureUnit sourceLectureUnit = sourceLectureUnitLink.getLectureUnit();
         Lecture sourceLecture = sourceLectureUnit.getLecture();
-        Lecture importedLecture = importOrLoadLecture(sourceLecture, courseToImportInto, titleToImportedLectures);
+        Lecture newLecture = importOrLoadLecture(sourceLecture, courseToImportInto, titleToImportedLectures);
 
         Optional<LectureUnit> foundLectureUnit = repositoryApi.findByNameAndLectureTitleAndCourseIdWithCompetencies(sourceLectureUnit.getName(), sourceLecture.getTitle(),
                 courseToImportInto.getId());
         LectureUnit importedLectureUnit;
         if (foundLectureUnit.isEmpty()) {
-            importedLectureUnit = api.importLectureUnit(sourceLectureUnit);
+            importedLectureUnit = api.importLectureUnit(sourceLectureUnit, newLecture);
 
-            importedLecture.getLectureUnits().add(importedLectureUnit);
-            importedLectureUnit.setLecture(importedLecture);
+            newLecture.addLectureUnit(importedLectureUnit);
         }
         else {
             importedLectureUnit = foundLectureUnit.get();
@@ -370,10 +369,10 @@ private Lecture importOrLoadLecture(Lecture sourceLecture, Course courseToImport
         if (foundLecture.isEmpty()) {
             foundLecture = repositoryApi.findUniqueByTitleAndCourseIdWithLectureUnitsElseThrow(sourceLecture.getTitle(), courseToImportInto.getId());
         }
-        Lecture importedLecture = foundLecture.orElseGet(() -> importApi.importLecture(sourceLecture, courseToImportInto, false));
-        titleToImportedLectures.put(importedLecture.getTitle(), importedLecture);
+        Lecture newLecture = foundLecture.orElseGet(() -> importApi.importLecture(sourceLecture, courseToImportInto, false));
+        titleToImportedLectures.put(newLecture.getTitle(), newLecture);
 
-        return importedLecture;
+        return newLecture;
     }
 
     private void setAllDates(Set<Exercise> importedExercises, Set<Lecture> importedLectures, Set<LectureUnit> importedLectureUnits,

src/main/java/de/tum/cit/aet/artemis/atlas/service/competency/CompetencyProgressService.java

@@ -11,6 +11,7 @@
 import java.util.stream.DoubleStream;
 
 import org.jspecify.annotations.NonNull;
+import org.jspecify.annotations.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.context.annotation.Conditional;
@@ -137,20 +138,33 @@ public void updateProgressByCompetencyAsync(CourseCompetency competency) {
      */
     @Async
     public void updateProgressForUpdatedLearningObjectAsync(LearningObject originalLearningObject, Optional<LearningObject> updatedLearningObject) {
-        SecurityUtils.setAuthorizationObject(); // Required for async
-
         Set<Long> originalCompetencyIds = originalLearningObject.getCompetencyLinks().stream().map(CompetencyLearningObjectLink::getCompetency).map(CourseCompetency::getId)
                 .collect(Collectors.toSet());
-        Set<CourseCompetency> updatedCompetencies = updatedLearningObject
-                .map(learningObject -> learningObject.getCompetencyLinks().stream().map(CompetencyLearningObjectLink::getCompetency).collect(Collectors.toSet())).orElse(Set.of());
+        updateProgressForUpdatedLearningObjectAsyncWithOriginalCompetencyIds(originalCompetencyIds, updatedLearningObject.orElse(null));
+    }
+
+    /**
+     * Asynchronously update the existing progress for all changed competencies linked to the given learning object
+     * If new competencies are added, the progress is updated for all users in the course, otherwise only the existing progresses are updated.
+     *
+     * @param originalCompetencyIds The original competency ids before the update
+     * @param updatedLearningObject The updated learning object after the update
+     */
+    @Async
+    public void updateProgressForUpdatedLearningObjectAsyncWithOriginalCompetencyIds(Set<Long> originalCompetencyIds, @Nullable LearningObject updatedLearningObject) {
+        SecurityUtils.setAuthorizationObject(); // Required for async
+
+        Set<CourseCompetency> updatedCompetencies = updatedLearningObject != null
+                ? updatedLearningObject.getCompetencyLinks().stream().map(CompetencyLearningObjectLink::getCompetency).collect(Collectors.toSet())
+                : Set.of();
         Set<Long> updatedCompetencyIds = updatedCompetencies.stream().map(CourseCompetency::getId).collect(Collectors.toSet());
 
         Set<Long> removedCompetencyIds = originalCompetencyIds.stream().filter(id -> !updatedCompetencyIds.contains(id)).collect(Collectors.toSet());
         Set<Long> addedCompetencyIds = updatedCompetencyIds.stream().filter(id -> !originalCompetencyIds.contains(id)).collect(Collectors.toSet());
 
         updateProgressByCompetencyIds(removedCompetencyIds);
         if (!addedCompetencyIds.isEmpty()) {
-            updateProgressByCompetencyIdsAndLearningObject(addedCompetencyIds, originalLearningObject);
+            updateProgressByCompetencyIdsAndLearningObject(addedCompetencyIds, updatedLearningObject);
         }
     }
 
@@ -174,6 +188,8 @@ private void updateProgressByCompetencyIdsAndLearningObject(Set<Long> competency
             };
             existingCompetencyUsers.addAll(existingLearningObjectUsers);
             log.debug("Updating competency progress for {} users.", existingCompetencyUsers.size());
+            // TODO: this could be a very expensive operation if many users and competencies are involved, we MUST optimize this in the future
+            // Minimal changes: load all competencies only once outside the loop and reuse them inside
             existingCompetencyUsers.forEach(user -> updateCompetencyProgress(competencyId, user));
         }
     }

src/main/java/de/tum/cit/aet/artemis/core/service/FileService.java

@@ -64,7 +64,7 @@ public byte[] getFileForPath(Path path) throws IOException {
      */
     @CacheEvict(value = "files", key = "#path")
     public void evictCacheForPath(Path path) {
-        log.info("Invalidate files cache for {}", path);
+        log.debug("Invalidate files cache for {}", path);
         // Intentionally blank
     }
 

src/main/java/de/tum/cit/aet/artemis/core/web/course/CourseOverviewResource.java

@@ -243,6 +243,8 @@ public ResponseEntity<Set<Course>> getCoursesForNotifications() {
      * @param courseId the id of the course to retrieve
      * @return the ResponseEntity with status 200 (OK) and with body the course, or with status 404 (Not Found)
      */
+    // TODO: this method is invoked quite often as part of course management resolve. However, it might not be necessary to fetch tutorial group configuration and online course
+    // configuration in such cases.
     @GetMapping("courses/{courseId}")
     @EnforceAtLeastStudent
     public ResponseEntity<Course> getCourse(@PathVariable Long courseId) {

src/main/java/de/tum/cit/aet/artemis/lecture/api/LectureUnitApi.java

@@ -11,6 +11,7 @@
 
 import de.tum.cit.aet.artemis.core.domain.User;
 import de.tum.cit.aet.artemis.lecture.domain.ExerciseUnit;
+import de.tum.cit.aet.artemis.lecture.domain.Lecture;
 import de.tum.cit.aet.artemis.lecture.domain.LectureUnit;
 import de.tum.cit.aet.artemis.lecture.repository.ExerciseUnitRepository;
 import de.tum.cit.aet.artemis.lecture.repository.LectureUnitRepository;
@@ -41,7 +42,7 @@ public LectureUnitApi(LectureUnitService lectureUnitService, LectureUnitReposito
         this.exerciseUnitRepository = exerciseUnitRepository;
     }
 
-    public void setCompletedForAllLectureUnits(List<? extends LectureUnit> lectureUnits, @NonNull User user, boolean completed) {
+    public <T extends LectureUnit> void setCompletedForAllLectureUnits(List<T> lectureUnits, @NonNull User user, boolean completed) {
         lectureUnitService.setCompletedForAllLectureUnits(lectureUnits, user, completed);
     }
 
@@ -56,7 +57,7 @@ public void removeLectureUnitFromExercise(long exerciseId) {
         }
     }
 
-    public LectureUnit importLectureUnit(LectureUnit sourceLectureUnit) {
-        return lectureUnitImportService.importLectureUnit(sourceLectureUnit);
+    public LectureUnit importLectureUnit(LectureUnit sourceLectureUnit, Lecture newLecture) {
+        return lectureUnitImportService.importLectureUnit(sourceLectureUnit, newLecture);
     }
 }

src/main/java/de/tum/cit/aet/artemis/lecture/domain/Lecture.java

@@ -1,8 +1,9 @@
 package de.tum.cit.aet.artemis.lecture.domain;
 
 import java.time.ZonedDateTime;
-import java.util.ArrayList;
+import java.util.Comparator;
 import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
 
@@ -11,12 +12,17 @@
 import jakarta.persistence.Entity;
 import jakarta.persistence.ManyToOne;
 import jakarta.persistence.OneToMany;
-import jakarta.persistence.OrderColumn;
+import jakarta.persistence.OrderBy;
+import jakarta.persistence.PrePersist;
+import jakarta.persistence.PreUpdate;
 import jakarta.persistence.Table;
 import jakarta.persistence.Transient;
+import jakarta.validation.constraints.NotNull;
 
+import org.hibernate.Hibernate;
 import org.hibernate.annotations.Cache;
 import org.hibernate.annotations.CacheConcurrencyStrategy;
+import org.jspecify.annotations.Nullable;
 
 import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 import com.fasterxml.jackson.annotation.JsonInclude;
@@ -59,11 +65,26 @@ public class Lecture extends DomainObject {
     @JsonIgnoreProperties(value = "lecture", allowSetters = true)
     private Set<Attachment> attachments = new HashSet<>();
 
+    /**
+     * The lecture units of this lecture.
+     * <p>
+     * Note: We use a Set here to avoid issues with Hibernate and JPA when managing the collection.
+     * A list without @OrderColumn is treated as Bag and would lead to issues when fetching data like lecture.lectureUnits.competencyLinks
+     * A Set prevents this, a LinkedHashSet is used to maintain insertion order.
+     * <p>
+     * Note: We cannot use @OrderColumn here because this leads to issues when saving the lecture (there were ugly workarounds needed in the past) and could potentially lead to
+     * null values in the order column which leads to unexpected behavior and unresolvable bugs in the user interface. This has happened in the past, therefore, we decided to
+     * NOT rely on @OrderColumn anymore.
+     * <p>
+     * Instead, we manage the order manually via the lectureUnitOrder field in LectureUnit, other developers who use lecture and lectureUnit do not need to worry about it as
+     * long as they use the provided methods to add/remove/reorder lecture units.
+     *
+     */
     @OneToMany(mappedBy = "lecture", cascade = CascadeType.ALL, orphanRemoval = true)
-    @OrderColumn(name = "lecture_unit_order")
+    @OrderBy("lectureUnitOrder ASC") // DB → Java: always ordered by that column
     @JsonIgnoreProperties("lecture")
     @Cache(usage = CacheConcurrencyStrategy.NONSTRICT_READ_WRITE)
-    private List<LectureUnit> lectureUnits = new ArrayList<>();
+    private Set<LectureUnit> lectureUnits = new LinkedHashSet<>();
 
     @ManyToOne
     @JsonIgnoreProperties(value = { "lectures", "exercises", "posts" }, allowSetters = true)
@@ -136,17 +157,119 @@ public void setAttachments(Set<Attachment> attachments) {
         this.attachments = attachments;
     }
 
+    /**
+     * Get an unmodifiable list of lecture units when the objects are initialized by Hibernate.
+     * This is important so that external code does not modify the list without updating the back-references and order.
+     * <p>
+     * Use {@link #addLectureUnit}, {@link #removeLectureUnit}, {@link #setLectureUnits}, or {@link #reorderLectureUnits} to modify the lecture units.
+     *
+     * @return the lecture units
+     */
     public List<LectureUnit> getLectureUnits() {
-        return lectureUnits;
+        if (Hibernate.isInitialized(lectureUnits)) {
+            return List.copyOf(lectureUnits);
+        }
+        // when not initialized, return an empty list to avoid LazyInitializationExceptions
+        return List.of();
+    }
+
+    /**
+     * Reorder the lecture units based on the given list of ordered IDs.
+     * Makes sure to update the order after reordering.
+     *
+     * @param orderedIds the list of lecture unit IDs in the desired order
+     */
+    public void reorderLectureUnits(@NotNull List<Long> orderedIds) {
+        List<LectureUnit> sorted = lectureUnits.stream().sorted(Comparator.comparing(unit -> orderedIds.indexOf(unit.getId()))).toList();
+        lectureUnits.clear();
+        lectureUnits.addAll(sorted);
+        updateLectureUnitOrder();
     }
 
-    public void setLectureUnits(List<LectureUnit> lectureUnits) {
-        this.lectureUnits = lectureUnits;
+    /**
+     * Set the lecture units, replacing any existing ones.
+     * Makes sure to update back-references and order.
+     *
+     * @param lectureUnits the new list of lecture units
+     */
+    public void setLectureUnits(@Nullable List<LectureUnit> lectureUnits) {
+        // this if statement is important to avoid issues when setting lazy loaded lectureUnits to null or empty which would not work in the else statement
+        if (lectureUnits == null || lectureUnits.isEmpty()) {
+            this.lectureUnits = new LinkedHashSet<>();
+        }
+        else {
+            this.lectureUnits.clear();
+            for (LectureUnit lectureUnit : lectureUnits) {
+                addLectureUnit(lectureUnit);        // ensures back-reference and collection management
+            }
+            updateLectureUnitOrder();
+        }
     }
 
-    public void addLectureUnit(LectureUnit lectureUnit) {
-        this.lectureUnits.add(lectureUnit);
+    /**
+     * Add a lecture unit to the end of the list.
+     * Makes sure to update back-references and order.
+     *
+     * @param lectureUnit the lecture unit to add
+     */
+    public void addLectureUnit(@Nullable LectureUnit lectureUnit) {
+        if (lectureUnit == null) {
+            return;
+        }
+        lectureUnits.add(lectureUnit);              // order is implicit by position
         lectureUnit.setLecture(this);
+        updateLectureUnitOrder();
+    }
+
+    /**
+     * Remove a lecture unit from the list.
+     * Makes sure to update back-references and order.
+     *
+     * @param lectureUnit the lecture unit to remove
+     */
+    public void removeLectureUnit(@Nullable LectureUnit lectureUnit) {
+        if (lectureUnit == null) {
+            return;
+        }
+        lectureUnits.remove(lectureUnit);
+        lectureUnit.setLecture(null);
+        updateLectureUnitOrder();
+    }
+
+    /**
+     * Remove a lecture unit by its ID.
+     * Makes sure to update back-references and order.
+     *
+     * @param lectureUnitId the ID of the lecture unit to remove
+     */
+    public void removeLectureUnitById(@Nullable Long lectureUnitId) {
+        if (lectureUnitId == null) {
+            return;
+        }
+
+        // find the unit to remove
+        LectureUnit toRemove = lectureUnits.stream().filter(unit -> unit != null && lectureUnitId.equals(unit.getId())).findFirst().orElse(null);
+
+        removeLectureUnit(toRemove);
+    }
+
+    /**
+     * Update the lectureUnitOrder field of all lecture units based on their current position in the set.
+     * This method is called before persisting or updating the Lecture entity to ensure consistency.
+     * It can also be called manually after reordering the lecture units or adding/removing some.
+     */
+    @PrePersist
+    @PreUpdate
+    public void updateLectureUnitOrder() {
+        if (Hibernate.isInitialized(lectureUnits)) {
+            int order = 0;
+            for (LectureUnit unit : lectureUnits) {
+                if (unit == null) {
+                    continue;
+                }
+                unit.setLectureUnitOrder(order++);
+            }
+        }
     }
 
     public Course getCourse() {