Merge remote-tracking branch 'origin/develop' into fix/exam-recovery-cdp-fetch-interception

Author: krusche

src/main/java/de/tum/cit/aet/artemis/assessment/domain/Result.java

@@ -113,6 +113,17 @@ public class Result extends DomainObject implements Comparable<Result> {
     @Column(name = "assessment_type")
     private AssessmentType assessmentType;
 
+    /**
+     * Which correction round this result belongs to: 0 for the first correction, 1 for the second, and so on. Null for
+     * automatic and Athena results, which are not correction rounds.
+     * <p>
+     * This used to be the position of the result inside its submission's result list, which is why that list carried an
+     * order column. Keeping the round here means a result can be written without loading and re-saving the whole
+     * submission just so Hibernate can renumber the list.
+     */
+    @Column(name = "correction_round")
+    private Integer correctionRound;
+
     @Column(name = "has_complaint")
     private Boolean hasComplaint;
 
@@ -455,6 +466,15 @@ public AssessmentType getAssessmentType() {
         return assessmentType;
     }
 
+    @Nullable
+    public Integer getCorrectionRound() {
+        return correctionRound;
+    }
+
+    public void setCorrectionRound(@Nullable Integer correctionRound) {
+        this.correctionRound = correctionRound;
+    }
+
     public Result assessmentType(AssessmentType assessmentType) {
         this.assessmentType = assessmentType;
         return this;

src/main/java/de/tum/cit/aet/artemis/assessment/dto/ResultDTO.java

@@ -22,8 +22,8 @@
  */
 @JsonInclude(JsonInclude.Include.NON_EMPTY)
 public record ResultDTO(Long id, ZonedDateTime completionDate, Boolean successful, Double score, Boolean rated, SubmissionDTO submission, ParticipationDTO participation,
-        List<FeedbackDTO> feedbacks, AssessmentType assessmentType, Boolean hasComplaint, Boolean exampleResult, UserNameDTO assessor, AssessmentNoteDTO assessmentNote,
-        Long exerciseId) implements Serializable {
+        List<FeedbackDTO> feedbacks, AssessmentType assessmentType, Integer correctionRound, Boolean hasComplaint, Boolean exampleResult, UserNameDTO assessor,
+        AssessmentNoteDTO assessmentNote, Long exerciseId) implements Serializable {
 
     /**
      * Converts a Result into a ResultDTO.
@@ -57,6 +57,6 @@ public static ResultDTO of(Result result) {
         // results), where the cascade merge writes the column back - without it on the wire the echo merges
         // exercise_id = 0 and dies on the foreign-key constraint.
         return new ResultDTO(result.getId(), result.getCompletionDate(), result.isSuccessful(), result.getScore(), result.isRated(), submissionDTO, participationDTO, feedbackDTOs,
-                result.getAssessmentType(), result.hasComplaint(), result.isExampleResult(), assessorDTO, assessmentNoteDTO, result.getExerciseId());
+                result.getAssessmentType(), result.getCorrectionRound(), result.hasComplaint(), result.isExampleResult(), assessorDTO, assessmentNoteDTO, result.getExerciseId());
     }
 }

src/main/java/de/tum/cit/aet/artemis/assessment/repository/ResultRepository.java

@@ -34,6 +34,7 @@
 import de.tum.cit.aet.artemis.core.repository.base.ArtemisJpaRepository;
 import de.tum.cit.aet.artemis.exercise.domain.Exercise;
 import de.tum.cit.aet.artemis.exercise.domain.Submission;
+import de.tum.cit.aet.artemis.exercise.dto.CorrectionRoundResultDTO;
 import de.tum.cit.aet.artemis.programming.domain.ProgrammingExercise;
 
 /**
@@ -500,6 +501,30 @@ SELECT COUNT(DISTINCT p)
      */
     boolean existsBySubmissionId(long submissionId);
 
+    /**
+     * Returns the manual results of the given submissions together with the correction round each belongs to.
+     * <p>
+     * The scores overview renders assessment actions per correction round, so it needs one entry per round rather than
+     * only the newest result. Automatic and Athena results are left out because they are not correction rounds, which
+     * matches {@link de.tum.cit.aet.artemis.exercise.domain.Submission#getManualResults()} and keeps a result whose
+     * assessment type is not set yet.
+     *
+     * @param submissionIds the submissions whose manual results should be read
+     * @return the manual results of those submissions, in no particular order
+     */
+    @Query("""
+            SELECT new de.tum.cit.aet.artemis.exercise.dto.CorrectionRoundResultDTO(
+                result.submission.id, result.id, result.correctionRound, result.assessmentType, result.completionDate, result.hasComplaint)
+            FROM Result result
+            WHERE result.submission.id IN :submissionIds
+                AND (result.assessmentType IS NULL
+                    OR result.assessmentType NOT IN (
+                        de.tum.cit.aet.artemis.assessment.domain.AssessmentType.AUTOMATIC,
+                        de.tum.cit.aet.artemis.assessment.domain.AssessmentType.AUTOMATIC_ATHENA
+                    ))
+            """)
+    List<CorrectionRoundResultDTO> findCorrectionRoundResultsBySubmissionIds(@Param("submissionIds") Set<Long> submissionIds);
+
     /**
      * Returns true if there is at least one result for the given exercise.
      * Uses the denormalized result.exerciseId for efficient filtering.

src/main/java/de/tum/cit/aet/artemis/exam/service/ExamService.java

@@ -285,11 +285,11 @@ private static void setResultIfNecessary(StudentExam studentExam, StudentPartici
                 if (latestResult != null) {
                     latestResult.setSubmission(lastSubmission);
                     latestResult.filterSensitiveInformation();
-                    lastSubmission.setResults(List.of(latestResult));
+                    lastSubmission.setResults(Set.of(latestResult));
                 }
             }
             else {
-                lastSubmission.setResults(List.of());
+                lastSubmission.setResults(Set.of());
             }
         }
     }
@@ -434,15 +434,15 @@ private void attachResultsToSubmissions(Set<ProgrammingExercise> programmingExer
                 ex.getSolutionParticipation().getSubmissions().forEach(sub -> {
                     Result result = latestSolutionResults.get(sub.getId());
                     if (result != null) {
-                        sub.setResults(List.of(result));
+                        sub.setResults(Set.of(result));
                     }
                 });
             }
             if (ex.getTemplateParticipation() != null && ex.getTemplateParticipation().getSubmissions() != null) {
                 ex.getTemplateParticipation().getSubmissions().forEach(sub -> {
                     Result result = latestTemplateResults.get(sub.getId());
                     if (result != null) {
-                        sub.setResults(List.of(result));
+                        sub.setResults(Set.of(result));
                     }
                 });
             }

src/main/java/de/tum/cit/aet/artemis/exercise/domain/Exercise.java

@@ -523,7 +523,7 @@ public void filterResultsForStudents(Participation participation) {
         boolean isAssessmentOver = getAssessmentDueDate() == null || getAssessmentDueDate().isBefore(ZonedDateTime.now());
 
         participation.getSubmissions().forEach(submission -> {
-            List<Result> results = submission.getResults();
+            Set<Result> results = submission.getResults();
             if (results != null && !results.isEmpty()) {
                 if (!isAssessmentOver) {
                     // For assessment that's not over yet

src/main/java/de/tum/cit/aet/artemis/exercise/domain/Submission.java

@@ -2,10 +2,9 @@
 
 import java.time.Duration;
 import java.time.ZonedDateTime;
-import java.util.ArrayList;
 import java.util.Comparator;
 import java.util.HashSet;
-import java.util.List;
+import java.util.LinkedHashSet;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
@@ -24,11 +23,10 @@
 import jakarta.persistence.InheritanceType;
 import jakarta.persistence.ManyToOne;
 import jakarta.persistence.OneToMany;
-import jakarta.persistence.OrderColumn;
+import jakarta.persistence.OrderBy;
 import jakarta.persistence.Table;
 
 import org.hibernate.annotations.ConcreteProxy;
-import org.jspecify.annotations.NonNull;
 import org.jspecify.annotations.Nullable;
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
@@ -87,13 +85,26 @@ public abstract class Submission extends DomainObject implements Comparable<Subm
     @OneToMany(mappedBy = "submission", cascade = CascadeType.REMOVE)
     private Set<SubmissionVersion> versions = new HashSet<>();
 
+    /**
+     * Orders results by their id, putting a result that was not saved yet at the end: it has just been created, so it
+     * is the newest one. Without the null handling every lookup here would throw for an in-memory result.
+     */
+    private static final Comparator<Result> BY_ID = Comparator.comparing(Result::getId, Comparator.nullsLast(Comparator.naturalOrder()));
+
     /**
      * A submission can have multiple results, therefore, results are persisted and removed with a submission.
+     * <p>
+     * A set, not a list: this used to be an ordered list whose position carried the correction round, which meant every
+     * write in this area had to load and re-save the whole submission so that Hibernate could renumber the order
+     * column. The correction round now lives on {@link Result#getCorrectionRound()}, so nothing needs the position.
+     * <p>
+     * Ordered by id all the same. Nothing on the server depends on it, but the collection is serialized to the client,
+     * and a hash set would hand it the results in an order that can change between two requests for the same data.
      */
     @OneToMany(mappedBy = "submission", fetch = FetchType.LAZY, cascade = CascadeType.ALL, orphanRemoval = true)
-    @OrderColumn(name = "results_order")
+    @OrderBy("id")
     @JsonIgnoreProperties({ "submission", "participation" })
-    private List<Result> results = new ArrayList<>();
+    private Set<Result> results = new LinkedHashSet<>();
 
     @Column(name = "submission_date")
     private ZonedDateTime submissionDate;
@@ -127,7 +138,7 @@ public Long getDurationInMinutes() {
     @Nullable
     @JsonIgnore
     public Result getLatestResult() {
-        Result latestResult = Optional.ofNullable(results).orElse(List.of()).stream().filter(Objects::nonNull).max(Comparator.comparing(Result::getId)).orElse(null);
+        Result latestResult = Optional.ofNullable(results).orElse(Set.of()).stream().filter(Objects::nonNull).max(BY_ID).orElse(null);
 
         if (latestResult != null) {
             latestResult.setSubmission(this);
@@ -146,8 +157,8 @@ public Result getLatestResult() {
     @Nullable
     @JsonIgnore
     public Result getLatestCompletedResult() {
-        Result latestResult = Optional.ofNullable(results).orElse(List.of()).stream().filter(result -> result != null && result.getCompletionDate() != null)
-                .max(Comparator.comparing(Result::getCompletionDate)).orElse(null);
+        Result latestResult = Optional.ofNullable(results).orElse(Set.of()).stream().filter(result -> result != null && result.getCompletionDate() != null)
+                .max(Comparator.comparing(Result::getCompletionDate).thenComparing(BY_ID)).orElse(null);
 
         if (latestResult != null) {
             latestResult.setSubmission(this);
@@ -166,19 +177,12 @@ public Result getLatestCompletedResult() {
     @Nullable
     @JsonIgnore
     public Result getResultForCorrectionRound(int correctionRound) {
-        List<Result> filteredResults = filterNonAutomaticResults();
-        if (correctionRound >= 0 && filteredResults.size() > correctionRound) {
-            return filteredResults.get(correctionRound);
+        if (correctionRound < 0) {
+            return null;
         }
-        return null;
-    }
-
-    /**
-     * @return an unmodifiable list or all non-automatic results
-     */
-    @NonNull
-    private List<Result> filterNonAutomaticResults() {
-        return results.stream().filter(result -> result == null || !(result.isAutomatic() || result.isAthenaBased())).toList();
+        // The lowest id among the matches, so that the answer does not depend on the iteration order of an unordered
+        // set. There should only ever be one result per round, and picking the earliest is what the ordered list did.
+        return getManualResults().stream().filter(result -> Objects.equals(result.getCorrectionRound(), correctionRound)).min(BY_ID).orElse(null);
     }
 
     /**
@@ -191,11 +195,7 @@ private List<Result> filterNonAutomaticResults() {
      */
     @JsonIgnore
     public boolean hasResultForCorrectionRound(int correctionRound) {
-        List<Result> withoutAutomaticResults = filterNonAutomaticResults();
-        if (withoutAutomaticResults.size() > correctionRound) {
-            return withoutAutomaticResults.get(correctionRound) != null;
-        }
-        return false;
+        return getResultForCorrectionRound(correctionRound) != null;
     }
 
     /**
@@ -204,7 +204,8 @@ public boolean hasResultForCorrectionRound(int correctionRound) {
      */
     @JsonIgnore
     public void removeAutomaticResults() {
-        this.results = this.results.stream().filter(result -> result == null || !(result.isAutomatic() || result.isAthenaBased())).collect(Collectors.toCollection(ArrayList::new));
+        this.results = this.results.stream().filter(result -> result == null || !(result.isAutomatic() || result.isAthenaBased()))
+                .collect(Collectors.toCollection(LinkedHashSet::new));
     }
 
     /**
@@ -219,22 +220,22 @@ public void removeAutomaticResults() {
      */
     @JsonIgnore
     public void removeNullResults() {
-        this.results = this.results.stream().filter(Objects::nonNull).collect(Collectors.toCollection(ArrayList::new));
+        this.results = this.results.stream().filter(Objects::nonNull).collect(Collectors.toCollection(LinkedHashSet::new));
     }
 
     @JsonProperty(value = "results", access = JsonProperty.Access.READ_ONLY)
-    public List<Result> getResults() {
+    public Set<Result> getResults() {
         return results;
     }
 
     @JsonIgnore
-    public List<Result> getAutomaticResults() {
-        return results.stream().filter(result -> result != null && (result.isAutomatic() || result.isAthenaBased())).collect(Collectors.toCollection(ArrayList::new));
+    public Set<Result> getAutomaticResults() {
+        return results.stream().filter(result -> result != null && (result.isAutomatic() || result.isAthenaBased())).collect(Collectors.toCollection(LinkedHashSet::new));
     }
 
     @JsonIgnore
-    public List<Result> getManualResults() {
-        return results.stream().filter(result -> result != null && !result.isAutomatic() && !result.isAthenaBased()).collect(Collectors.toCollection(ArrayList::new));
+    public Set<Result> getManualResults() {
+        return results.stream().filter(result -> result != null && !result.isAutomatic() && !result.isAthenaBased()).collect(Collectors.toCollection(LinkedHashSet::new));
     }
 
     /**
@@ -243,8 +244,8 @@ public List<Result> getManualResults() {
      * @return non athena automatic results excluding null results
      */
     @JsonIgnore
-    public List<Result> getNonAthenaResults() {
-        return results.stream().filter(result -> result != null && !result.isAthenaBased()).collect(Collectors.toCollection(ArrayList::new));
+    public Set<Result> getNonAthenaResults() {
+        return results.stream().filter(result -> result != null && !result.isAthenaBased()).collect(Collectors.toCollection(LinkedHashSet::new));
     }
 
     /**
@@ -267,10 +268,10 @@ public Result getManualResultsById(long resultId) {
     @Nullable
     @JsonIgnore
     public Result getFirstResult() {
-        if (results != null && !results.isEmpty()) {
-            return results.getFirst();
+        if (results == null || results.isEmpty()) {
+            return null;
         }
-        return null;
+        return results.stream().filter(Objects::nonNull).min(BY_ID).orElse(null);
     }
 
     /**
@@ -281,10 +282,12 @@ public Result getFirstResult() {
     @Nullable
     @JsonIgnore
     public Result getFirstManualResult() {
-        // Guard on the manual results, not on all results: a submission can carry only automatic or Athena results, and
-        // getFirst() on the then empty manual list would throw instead of returning null as declared.
-        List<Result> manualResults = results == null ? List.of() : getManualResults();
-        return manualResults.isEmpty() ? null : manualResults.getFirst();
+        // The earliest manual result, which is the one of the first correction round. Guard on the manual results, not
+        // on all results: a submission can carry only automatic or Athena results and then there is none.
+        if (results == null) {
+            return null;
+        }
+        return getManualResults().stream().min(BY_ID).orElse(null);
     }
 
     /**
@@ -299,29 +302,49 @@ public Result getFirstManualResult() {
     @Nullable
     @JsonIgnore
     public Result getLatestManualResult() {
-        List<Result> manualResults = results == null ? List.of() : getManualResults();
-        return manualResults.isEmpty() ? null : manualResults.getLast();
+        // The most recent manual result, which is the one of the highest correction round.
+        if (results == null) {
+            return null;
+        }
+        return getManualResults().stream().max(BY_ID).orElse(null);
     }
 
     /**
-     * Add a result to the list.
-     * NOTE: You must make sure to correctly persist the result in the database!
+     * Adds a result to this submission and, if it is a correction-round result that does not have a round yet, assigns
+     * the next one.
+     * <p>
+     * This is where the round used to come from implicitly: the results were an ordered list and the position carried
+     * the round, so adding a result to the list decided which round it belonged to. The round now lives on the result,
+     * and this is the same moment, so the behaviour is unchanged for every caller that does not set it itself.
+     * {@code SubmissionService.lockSubmission} does set it, from the round the tutor asked for, and that takes
+     * precedence. Automatic and Athena results are not correction rounds and keep no round.
      *
-     * @param result the {@link Result} which should be added
+     * @param result the result to add
      */
     public void addResult(Result result) {
+        if (result != null && result.getCorrectionRound() == null && !result.isAutomatic() && !result.isAthenaBased()) {
+            result.setCorrectionRound(countCorrectionRoundResults(result));
+        }
         this.results.add(result);
     }
 
+    /**
+     * @param resultToAdd the result that is about to be added, which must not count itself
+     * @return how many correction-round results this submission already holds
+     */
+    private int countCorrectionRoundResults(Result resultToAdd) {
+        return (int) results.stream().filter(other -> other != null && other != resultToAdd && !other.isAutomatic() && !other.isAthenaBased()).count();
+    }
+
     /**
      * Set the results list to the specified list.
      * NOTE: You must correctly persist this change in the database manually!
      *
      * @param results The list of {@link Result} which should replace the existing results of the submission
      */
     @JsonProperty(value = "results", access = JsonProperty.Access.WRITE_ONLY)
-    public void setResults(List<Result> results) {
-        this.results = results;
+    public void setResults(Set<Result> results) {
+        this.results = results != null ? results : new LinkedHashSet<>();
     }
 
     public Participation getParticipation() {
@@ -398,9 +421,12 @@ public void setExampleSubmission(Boolean exampleSubmission) {
      */
     public void removeNotNeededResults(int correctionRound, Long resultId) {
         if (correctionRound == 0 && resultId == null && getResults().size() >= 2) {
-            var resultList = new ArrayList<Result>();
-            resultList.add(getFirstManualResult());
-            setResults(resultList);
+            var remainingResults = new HashSet<Result>();
+            var firstManualResult = getFirstManualResult();
+            if (firstManualResult != null) {
+                remainingResults.add(firstManualResult);
+            }
+            setResults(remainingResults);
         }
     }