Implementing CoW for exercise tests (and updating GC to clean abandoned tests).

Author: Martin Krulis

app/V1Module/presenters/ExercisesConfigPresenter.php

@@ -21,6 +21,7 @@
 use App\Model\Entity\ExerciseEnvironmentConfig;
 use App\Model\Entity\ExerciseTest;
 use App\Model\Repository\Exercises;
+use App\Model\Repository\ExerciseTests;
 use App\Model\Repository\HardwareGroups;
 use App\Model\Repository\Pipelines;
 use App\Model\Repository\ReferenceSolutionSubmissions;
@@ -43,6 +44,12 @@ class ExercisesConfigPresenter extends BasePresenter {
    */
   public $exercises;
 
+  /**
+   * @var ExerciseTests
+   * @inject
+   */
+  public $exerciseTests;
+
   /**
    * @var Pipelines
    * @inject
@@ -598,35 +605,52 @@ public function actionSetTests(string $id) {
     $req = $this->getRequest();
     $tests = $req->getPost("tests");
 
+    /*
+     * We need to implement CoW on tests.
+     * All modified tests has to be newly created (with new IDs) and these
+     * new IDs has to be propagated into configuration and limits.
+     * Therefore a replacement mapping of updated tests is kept.
+     */
+
     $newTests = [];
+    $namesToOldIds = [];  // new test name => old test ID
+
     foreach ($tests as $test) {
+      // Perform checks on the test name...
       if (!array_key_exists("name", $test)) {
         throw new InvalidArgumentException("tests", "name item not found in particular test");
       }
 
-      $name = $test["name"];
+      $name = trim($test["name"]);
+      if (!preg_match('/^[-a-zA-Z0-9_()\[\].! ]+$/', $name)) {
+        throw new InvalidArgumentException("tests", "test name contains illicit characters");
+      }
+      if (strlen($name) > 64) {
+        throw new InvalidArgumentException("tests", "test name too long (exceeds 64 characters)");
+      }
+      if (array_key_exists($name, $newTests)) {
+        throw new InvalidArgumentException("tests", "two tests with the same name '$name' were specified");
+      }
+
       $id = Arrays::get($test, "id", null);
-      $description = Arrays::get($test, "description", "");
+      $description = trim(Arrays::get($test, "description", ""));
 
+      // Prepare a test entity that is to be inserted into the new list of tests...
       $testEntity = $id ? $exercise->getExerciseTestById($id) : null;
       if ($testEntity === null) {
         // new exercise test was requested to be created
         if ($exercise->getExerciseTestByName($name)) {
           throw new InvalidArgumentException("tests", "given test name '$name' is already taken");
         }
 
-        $testEntity = new ExerciseTest(trim($name), $description, $this->getCurrentUser());
-      } else {
-        // update of existing exercise test with all appropriate fields
-        $testEntity->setName(trim($name));
-        $testEntity->setDescription($description);
-        $testEntity->updatedNow();
+        $testEntity = new ExerciseTest($name, $description, $this->getCurrentUser());
+      } elseif ($testEntity->getName() !== $name || $testEntity->getDescription() !== $description) {
+        // an update is needed => a copy is made and old ID mapping is kept
+        $namesToOldIds[$name] = $id;
+        $testEntity = new ExerciseTest($name, $description, $testEntity->getAuthor());
       }
+      // otherwise, the $testEntity is unchanged
 
-
-      if (array_key_exists($name, $newTests)) {
-        throw new InvalidArgumentException("tests", "two tests with the same name '$name' were specified");
-      }
       $newTests[$name] = $testEntity;
     }
 
@@ -638,20 +662,39 @@ public function actionSetTests(string $id) {
       );
     }
 
-    // clear old tests and set new ones
-    $exercise->getExerciseTests()->clear();
-    $exercise->setExerciseTests(new ArrayCollection($newTests));
+    $this->exercises->beginTransaction();
+    try {
+      // clear old tests and set new ones
+      $exercise->getExerciseTests()->clear();
+      $exercise->setExerciseTests(new ArrayCollection($newTests));
+      $this->exercises->flush();
 
-    // update exercise configuration and test in here
-    $this->exerciseConfigUpdater->testsUpdated($exercise, $this->getCurrentUser(), false);
+      // now we need to get IDs of newly created tests
+      $idMapping = [];  // old ID => new ID
+      foreach ($newTests as $test) {
+        $this->exerciseTests->refresh($test);
+        if (array_key_exists($test->getName(), $namesToOldIds)) {
+          $oldId = $namesToOldIds[$test->getName()];
+          $idMapping[$oldId] = $test->getId();
+        }
+      }
 
-    $exercise->updatedNow();
-    $this->exercises->flush();
+      // update exercise configuration and test in here
+      $this->exerciseConfigUpdater->testsUpdated($exercise, $this->getCurrentUser(), $idMapping, false);
 
-    $this->configChecker->check($exercise);
-    $this->exercises->flush();
+      $exercise->updatedNow();
+      $this->exercises->flush();
 
-    $this->sendSuccessResponse($exercise->getExerciseTests()->getValues());
+      $this->configChecker->check($exercise);
+      $this->exercises->flush();
+      $this->exercises->commit();
+    }
+    catch (\Exception $e) {
+      $this->exercises->rollBack();
+      throw $e;
+    }
+
+    $this->sendSuccessResponse(array_values($newTests));
   }
 
 }

app/commands/cleanup/CleanupExerciseConfigs.php

@@ -131,16 +131,57 @@ private function cleanupLimits(DateTime $limit): int {
     return $deleteQuery->execute();
   }
 
+  /**
+   * Delete tests and return number of deleted entities.
+   * @param DateTime $limit
+   * @return int
+   */
+  private function cleanupTests(DateTime $limit): int {
+    $usedTests = [];
+
+    /** @var Exercise $exercise */
+    foreach ($this->exercises->findAllAndIReallyMeanAllOkay() as $exercise) {
+      foreach ($exercise->getExerciseTestsIds() as $id) {
+        $usedTests[$id] = true;
+      }
+    }
+
+    /** @var Assignment $assignment */
+    foreach ($this->assignments->findAllAndIReallyMeanAllOkay() as $assignment) {
+      foreach ($assignment->getExerciseTestsIds() as $id) {
+        $usedTests[$id] = true;
+      }
+    }
+
+    $deleteQuery = $this->entityManager->createQuery('
+      DELETE FROM App\Model\Entity\ExerciseTest t
+      WHERE t.createdAt <= :date AND t.id NOT IN (:ids)
+    ');
+
+    $deleteQuery->setParameter(":date", $limit);
+    $deleteQuery->setParameter("ids", array_keys($usedTests), Connection::PARAM_STR_ARRAY);
+    return $deleteQuery->execute();
+  }
+
   protected function execute(InputInterface $input, OutputInterface $output) {
     $now = new DateTime();
     $limit = clone $now;
     $limit->modify("-14 days");
 
-    $deletedEnvsCount = $this->cleanupEnvironmentConfigs($limit);
-    $deletedConfsCount = $this->cleanupExerciseConfigs($limit);
-    $deletedLimsCount = $this->cleanupLimits($limit);
+    $this->exercises->beginTransaction();
+    try {
+      $deletedEnvsCount = $this->cleanupEnvironmentConfigs($limit);
+      $deletedConfsCount = $this->cleanupExerciseConfigs($limit);
+      $deletedLimsCount = $this->cleanupLimits($limit);
+      $deletedTestsCount = $this->cleanupTests($limit);
+      $this->exercises->commit();
+    }
+    catch (\Exception $e) {
+      $this->exercises->rollBack();
+      throw $e;
+    }
 
-    $output->writeln("Removed: {$deletedEnvsCount} environment configs; {$deletedConfsCount} exercise configs; {$deletedLimsCount} exercise limits");
+    $output->writeln("Removed: {$deletedEnvsCount} environment configs; {$deletedConfsCount} exercise configs; {$deletedLimsCount} exercise limits; {$deletedTestsCount} exercise tests");
     return 0;
   }
 }

app/config/config.neon

@@ -343,6 +343,7 @@ services:
   # models - repositories
   - App\Model\Repository\Comments
   - App\Model\Repository\Exercises
+  - App\Model\Repository\ExerciseTests
   - App\Model\Repository\Assignments
   - App\Model\Repository\ExternalLogins
   - App\Model\Repository\Groups

app/helpers/ExerciseConfig/Config/ExerciseConfig.php

@@ -31,21 +31,21 @@ public function getEnvironments(): array {
 
   /**
    * Add environment into this holder.
-   * @param string $name
+   * @param string $id
    * @return $this
    */
-  public function addEnvironment(string $name): ExerciseConfig {
-    $this->environments[] = $name;
+  public function addEnvironment(string $id): ExerciseConfig {
+    $this->environments[] = $id;
     return $this;
   }
 
   /**
    * Remove environment according to given name identification.
-   * @param string $name
+   * @param string $id
    * @return $this
    */
-  public function removeEnvironment(string $name): ExerciseConfig {
-    if(($key = array_search($name, $this->environments)) !== false) {
+  public function removeEnvironment(string $id): ExerciseConfig {
+    if(($key = array_search($id, $this->environments)) !== false) {
       unset($this->environments[$key]);
     }
     return $this;
@@ -61,35 +61,51 @@ public function getTests(): array {
 
   /**
    * Get test for the given test name.
-   * @param string $name
+   * @param string $id
    * @return Test|null
    */
-  public function getTest(string $name): ?Test {
-    if (!array_key_exists($name, $this->tests)) {
+  public function getTest(string $id): ?Test {
+    if (!array_key_exists($id, $this->tests)) {
       return null;
     }
 
-    return $this->tests[$name];
+    return $this->tests[$id];
   }
 
   /**
    * Add test into this holder.
-   * @param string $name
+   * @param string $id
    * @param Test $test
    * @return $this
    */
-  public function addTest(string $name, Test $test): ExerciseConfig {
-    $this->tests[$name] = $test;
+  public function addTest(string $id, Test $test): ExerciseConfig {
+    $this->tests[$id] = $test;
     return $this;
   }
 
   /**
    * Remove test according to given test identification.
-   * @param string $name
+   * @param string $id
    * @return $this
    */
-  public function removeTest(string $name): ExerciseConfig {
-    unset($this->tests[$name]);
+  public function removeTest(string $id): ExerciseConfig {
+    unset($this->tests[$id]);
+    return $this;
+  }
+
+  /**
+   * Remove test according to given test identification.
+   * @param string $id
+   * @return $this
+   */
+  public function changeTestId(string $oldId, string $newId): ExerciseConfig {
+    $test = $this->getTest($oldId);
+    if ($test) {
+      if ($this->getTest($newId)) {
+        throw new \Exception("Serious internal error. Newly created test ID is already present in exercise config!");
+      }
+      $this->removeTest($oldId)->addTest($newId, $test);
+    }
     return $this;
   }
 

app/helpers/ExerciseConfig/Limits/ExerciseLimits.php

@@ -56,6 +56,23 @@ public function removeLimits(string $testId): ExerciseLimits {
     return $this;
   }
 
+  /**
+   * Remove limits for given test identification.
+   * @param string $testId
+   * @return ExerciseLimits
+   */
+  public function changeTestId(string $oldId, string $newId): ExerciseLimits {
+    $limits = $this->getLimits($oldId);
+    if ($limits) {
+      if ($this->getLimits($newId)) {
+        throw new \Exception("Serious internal error. Newly created test ID is already present in exercise limits!");
+      }
+      $this->removeLimits($oldId)->addLimits($newId, $limits);
+    }
+    return $this;
+  }
+
+
   /**
    * Creates and returns properly structured array representing this object.
    * @return array

app/helpers/ExerciseConfig/Updater.php

@@ -65,9 +65,9 @@ public function environmentsUpdated(Exercise $exercise, User $user, bool $flush
    * @param bool $flush
    * @throws ExerciseConfigException
    */
-  public function testsUpdated(Exercise $exercise, User $user, bool $flush = true) {
-    $this->updateTestsInConfig($exercise, $user);
-    $this->updateTestsInLimits($exercise, $user);
+  public function testsUpdated(Exercise $exercise, User $user, array $idMappings = [], bool $flush = true) {
+    $this->updateTestsInConfig($exercise, $user, $idMappings);
+    $this->updateTestsInLimits($exercise, $user, $idMappings);
 
     if ($flush) {
       $this->exercises->flush();
@@ -161,15 +161,20 @@ private function updateEnvironmentsInLimits(Exercise $exercise, User $user) {
    * @param User $user
    * @throws ExerciseConfigException
    */
-  private function updateTestsInConfig(Exercise $exercise, User $user) {
+  private function updateTestsInConfig(Exercise $exercise, User $user, array $idMappings = []) {
     $exerciseConfig = $this->loader->loadExerciseConfig($exercise->getExerciseConfig()->getParsedConfig());
-    $testNames = $exercise->getExerciseTestsIds();
+    $testIds = $exercise->getExerciseTestsIds();
+
+    // rename test IDs according to provided mapping
+    foreach ($idMappings as $oldId => $newId) {
+      $exerciseConfig->changeTestId($oldId, $newId);
+    }
 
     // remove old tests
-    foreach ($exerciseConfig->getTests() as $name => $test) {
-      // test name not found in all newly created or updated tests, terminate it
-      if (!in_array($name, $testNames)) {
-        $exerciseConfig->removeTest($name);
+    foreach ($exerciseConfig->getTests() as $id => $test) {
+      // test ID not found in all newly created or updated tests, terminate it
+      if (!in_array($id, $testIds)) {
+        $exerciseConfig->removeTest($id);
       }
     }
 
@@ -185,17 +190,22 @@ private function updateTestsInConfig(Exercise $exercise, User $user) {
    * @param User $user
    * @throws ExerciseConfigException
    */
-  private function updateTestsInLimits(Exercise $exercise, User $user) {
-    $testNames = $exercise->getExerciseTestsIds();
+  private function updateTestsInLimits(Exercise $exercise, User $user, array $idMappings = []) {
+    $testIds = $exercise->getExerciseTestsIds();
 
     foreach ($exercise->getExerciseLimits() as $exerciseLimits) {
       $limits = $this->loader->loadExerciseLimits($exerciseLimits->getParsedLimits());
 
+      // rename test IDs according to provided mapping
+      foreach ($idMappings as $oldId => $newId) {
+        $limits->changeTestId($oldId, $newId);
+      }
+
       // remove old tests
-      foreach ($limits->getLimitsArray() as $name => $testLimits) {
-        // test name not found in all newly created or updated tests, terminate it
-        if (!in_array($name, $testNames)) {
-          $limits->removeLimits($name);
+      foreach ($limits->getLimitsArray() as $id => $testLimits) {
+        // test ID not found in all newly created or updated tests, terminate it
+        if (!in_array($id, $testIds)) {
+          $limits->removeLimits($id);
         }
       }