#221 - Fix CsvSplitterTask max_lines and infinite loop

CHANGELOG.md

@@ -21,6 +21,7 @@ Latest
 * [#208](https://github.com/cleverage/process-bundle/issues/208) Fix FolderBrowserTask, InputFolderBrowserTask, CsvReaderTask and LineReaderTask (and their `Input*` variants): reset the state at the end of the iteration, so that a following input is read from its beginning. Update documentation, add tests.
 * [#201](https://github.com/cleverage/process-bundle/issues/201) Fix minor defects: error messages of TransformerTrait, RulesTransformer and ExpressionLanguageMapTransformer, useless `setRequired()` in ImplodeTransformer and SprintfTransformer, stray namespace in TrimTransformer, wrong or missing docblocks. Add tests.
 * [#220](https://github.com/cleverage/process-bundle/issues/220) Fix the stop error strategy: throw a `ProcessFailedException` (with the original exception as `previous`) instead of a `FatalError`, so that the command exits with a non-zero code when a process fails and `ProcessLauncherTask` detects failed sub-processes. Update documentation, add tests.
+* [#221](https://github.com/cleverage/process-bundle/issues/221) Fix CsvSplitterTask: each produced file contains `max_lines` data lines (instead of `max_lines - 2`), no infinite loop with `max_lines` <= 2 (`max_lines` must now be an integer greater than 0), no header-only file at the end. Update documentation, add tests.
 
 ## Deprecated
 * [#189](https://github.com/cleverage/process-bundle/issues/189) EventDispatcherTask: when `event_name` is set, listening to `CleverAge\ProcessBundle\Event\EventDispatcherTaskEvent` is deprecated (the event is still dispatched under its class name, with an `E_USER_DEPRECATED` error, if it has listeners). Listen to the configured `event_name` instead: the BC layer will be removed in v6.0.

docs/reference/tasks/csv_splitter_task.md

@@ -19,15 +19,15 @@ Accepted inputs
 Possible outputs
 ----------------
 
-`string`: path of a temporary CSV file (created in the system temporary directory) containing the headers and a chunk
-of lines of the source file.
+`string`: path of a temporary CSV file (created in the system temporary directory) containing the headers and the next
+`max_lines` data lines of the source file (the last file may contain fewer lines).
 
 Options
 -------
 
 | Code              | Type          | Required | Default | Description                                                                                          |
 |-------------------|---------------|:--------:|---------|------------------------------------------------------------------------------------------------------|
-| `max_lines`       | `int`         |          | `1000`  | Maximum number of lines per produced file (see notes)                                                |
+| `max_lines`       | `int`         |          | `1000`  | Maximum number of data lines (headers excluded) per produced file, must be greater than 0            |
 | `base_path`       | `string`      |          | `''`    | Prepended (with a `/` separator) to the input path. If empty, the input path is used as is           |
 | `delimiter`       | `string`      |          | `;`     | CSV delimiter (used for both the source and the produced files)                                      |
 | `enclosure`       | `string`      |          | `"`     | CSV enclosure character                                                                              |
@@ -65,5 +65,5 @@ Notes
 
 * Lines are copied as is: they are not checked against the headers.
 * Temporary files are not deleted by the task, use [FileRemoverTask](file_remover_task.md) if needed.
-* The line counter of the produced file includes the header line and starts at 1, so each produced file actually
-  contains `max_lines - 2` data lines.
+* Empty lines of the source file are copied and counted like the other lines.
+* No empty file is produced: when the source file has no (more) data line, the task is skipped.

src/Task/File/Csv/CsvSplitterTask.php

@@ -23,6 +23,11 @@
  */
 class CsvSplitterTask extends InputCsvReaderTask
 {
+    /**
+     * Number of data lines written in the last produced file.
+     */
+    protected int $splitLineCount = 0;
+
     #[\Override]
     public function execute(ProcessState $state): void
     {
@@ -42,7 +47,15 @@ public function execute(ProcessState $state): void
         }
 
         // Return a temporary file containing a limited number of lines
-        $state->setOutput($this->splitCsv($this->csv, $options['max_lines']));
+        $splitFilePath = $this->splitCsv($this->csv, $options['max_lines']);
+        if (0 === $this->splitLineCount) {
+            // The end of the source file is only detected after trying to read past its last line: no empty file
+            unlink($splitFilePath);
+            $state->setSkipped(true);
+
+            return;
+        }
+        $state->setOutput($splitFilePath);
     }
 
     /**
@@ -91,12 +104,14 @@ protected function splitCsv(CsvResource $csv, int $maxLines): string
         );
         $splitCsv->writeHeaders();
 
-        while ($splitCsv->getLineNumber() < $maxLines && !$csv->isEndOfFile()) {
+        $this->splitLineCount = 0;
+        while ($this->splitLineCount < $maxLines && !$csv->isEndOfFile()) {
             $raw = $csv->readRaw();
             if (false === $raw) {
                 continue; // This is probably an empty line, no harm to skip it
             }
             $splitCsv->writeRaw($raw);
+            ++$this->splitLineCount;
         }
         $splitCsv->close();
 
@@ -110,5 +125,7 @@ protected function configureOptions(OptionsResolver $resolver): void
         $resolver->setDefaults([
             'max_lines' => 1000,
         ]);
+        $resolver->setAllowedTypes('max_lines', ['int']);
+        $resolver->setAllowedValues('max_lines', static fn (int $value): bool => $value >= 1);
     }
 }

tests/Task/File/Csv/CsvSplitterTaskTest.php

@@ -0,0 +1,169 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the CleverAge/ProcessBundle package.
+ *
+ * Copyright (c) Clever-Age
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace CleverAge\ProcessBundle\Tests\Task\File\Csv;
+
+use CleverAge\ProcessBundle\Configuration\ProcessConfiguration;
+use CleverAge\ProcessBundle\Configuration\TaskConfiguration;
+use CleverAge\ProcessBundle\Context\ContextualOptionResolver;
+use CleverAge\ProcessBundle\Filesystem\CsvFile;
+use CleverAge\ProcessBundle\Filesystem\CsvResource;
+use CleverAge\ProcessBundle\Model\AbstractConfigurableTask;
+use CleverAge\ProcessBundle\Model\ProcessHistory;
+use CleverAge\ProcessBundle\Model\ProcessState;
+use CleverAge\ProcessBundle\Task\File\Csv\AbstractCsvResourceTask;
+use CleverAge\ProcessBundle\Task\File\Csv\AbstractCsvTask;
+use CleverAge\ProcessBundle\Task\File\Csv\CsvReaderTask;
+use CleverAge\ProcessBundle\Task\File\Csv\CsvSplitterTask;
+use CleverAge\ProcessBundle\Task\File\Csv\InputCsvReaderTask;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+use Psr\Log\NullLogger;
+use Symfony\Component\Filesystem\Filesystem;
+use Symfony\Component\OptionsResolver\Exception\InvalidOptionsException;
+
+#[\PHPUnit\Framework\Attributes\CoversClass(CsvSplitterTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(CsvReaderTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(InputCsvReaderTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(AbstractCsvTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(AbstractCsvResourceTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(CsvFile::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(CsvResource::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(AbstractConfigurableTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(TaskConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ContextualOptionResolver::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessHistory::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessState::class)]
+class CsvSplitterTaskTest extends TestCase
+{
+    private string $tmpDir;
+
+    /** @var list<string> */
+    private array $producedFiles = [];
+
+    protected function setUp(): void
+    {
+        $this->tmpDir = sys_get_temp_dir().\DIRECTORY_SEPARATOR.uniqid('csv_splitter_test_', true);
+    }
+
+    protected function tearDown(): void
+    {
+        (new Filesystem())->remove([$this->tmpDir, ...$this->producedFiles]);
+    }
+
+    /**
+     * @return iterable<string, array{int, int, list<int>}>
+     */
+    public static function splitProvider(): iterable
+    {
+        yield 'last file shorter' => [10, 4, [4, 4, 2]];
+        yield 'exact multiple of max_lines' => [10, 5, [5, 5]];
+        yield 'one line per file' => [3, 1, [1, 1, 1]];
+        yield 'max_lines greater than the line count' => [3, 1000, [3]];
+    }
+
+    /**
+     * @param list<int> $expectedLineCounts
+     */
+    #[DataProvider('splitProvider')]
+    public function testEachFileContainsMaxLinesDataLines(int $lineCount, int $maxLines, array $expectedLineCounts): void
+    {
+        $source = $this->createSource($lineCount);
+
+        $files = $this->split($source, $maxLines);
+
+        self::assertSame($expectedLineCounts, array_map(static fn (array $lines): int => \count($lines) - 1, $files));
+        // Every file starts with the headers, and every data line is kept once, in order
+        $dataLines = [];
+        foreach ($files as $lines) {
+            self::assertSame("id;name\n", $lines[0]);
+            array_push($dataLines, ...\array_slice($lines, 1));
+        }
+        self::assertSame(\array_slice(file($source) ?: [], 1), $dataLines);
+    }
+
+    public function testNoFileForASourceWithoutDataLine(): void
+    {
+        self::assertSame([], $this->split($this->createSource(0), 5));
+    }
+
+    /**
+     * @return iterable<string, array{mixed}>
+     */
+    public static function invalidMaxLinesProvider(): iterable
+    {
+        yield 'zero' => [0];
+        yield 'negative' => [-1];
+        yield 'string' => ['5'];
+    }
+
+    #[DataProvider('invalidMaxLinesProvider')]
+    public function testInvalidMaxLinesIsRejected(mixed $maxLines): void
+    {
+        $this->expectException(InvalidOptionsException::class);
+
+        $this->createTask($maxLines);
+    }
+
+    private function createSource(int $lineCount): string
+    {
+        $content = "id;name\n";
+        for ($i = 1; $i <= $lineCount; ++$i) {
+            $content .= "{$i};name{$i}\n";
+        }
+        $path = $this->tmpDir.'/source.csv';
+        (new Filesystem())->dumpFile($path, $content);
+
+        return $path;
+    }
+
+    /**
+     * Iterate over the task like the ProcessManager does, returning the lines of each produced file.
+     *
+     * @return list<list<string>>
+     */
+    private function split(string $source, int $maxLines): array
+    {
+        [$task, $state] = $this->createTask($maxLines);
+        $files = [];
+        $iterations = 0;
+        do {
+            $state->reset(false);
+            $state->setInput($source);
+            $task->execute($state);
+            if (!$state->isSkipped()) {
+                $this->producedFiles[] = $state->getOutput();
+                $files[] = file($state->getOutput()) ?: [];
+            }
+        } while ($task->next($state) && ++$iterations < 100);
+
+        return $files;
+    }
+
+    /**
+     * @return array{CsvSplitterTask, ProcessState}
+     */
+    private function createTask(mixed $maxLines): array
+    {
+        $processConfiguration = new ProcessConfiguration('test', []);
+        $state = new ProcessState($processConfiguration, new ProcessHistory($processConfiguration));
+        $state->setContextualOptionResolver(new ContextualOptionResolver());
+        $state->setContext([]);
+        $state->setTaskConfiguration(new TaskConfiguration('split', CsvSplitterTask::class, ['max_lines' => $maxLines]));
+        $task = new CsvSplitterTask(new NullLogger());
+        $task->initialize($state);
+
+        return [$task, $state];
+    }
+}