From f1431ff490e50ea2559f4576d3b9bcfbc1f1fae8 Mon Sep 17 00:00:00 2001 From: Nicolas Joubert Date: Mon, 28 Sep 2026 16:47:08 +0200 Subject: [PATCH] fix(task) #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. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 1 + docs/reference/tasks/csv_splitter_task.md | 10 +- src/Task/File/Csv/CsvSplitterTask.php | 21 ++- tests/Task/File/Csv/CsvSplitterTaskTest.php | 169 ++++++++++++++++++++ 4 files changed, 194 insertions(+), 7 deletions(-) create mode 100644 tests/Task/File/Csv/CsvSplitterTaskTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 237cb95b..bb427021 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ Latest * [#207](https://github.com/cleverage/process-bundle/issues/207) Fix JsonStreamReaderTask / JsonStreamWriterTask: throw an explicit `\UnexpectedValueException` when a line decodes to a scalar, create the missing parent directory when writing. Update documentation, add tests. * [#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. +* [#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. diff --git a/docs/reference/tasks/csv_splitter_task.md b/docs/reference/tasks/csv_splitter_task.md index 77634e7e..322fa05f 100644 --- a/docs/reference/tasks/csv_splitter_task.md +++ b/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. diff --git a/src/Task/File/Csv/CsvSplitterTask.php b/src/Task/File/Csv/CsvSplitterTask.php index c11a0b9d..a7803ec5 100644 --- a/src/Task/File/Csv/CsvSplitterTask.php +++ b/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); } } diff --git a/tests/Task/File/Csv/CsvSplitterTaskTest.php b/tests/Task/File/Csv/CsvSplitterTaskTest.php new file mode 100644 index 00000000..76aa2e94 --- /dev/null +++ b/tests/Task/File/Csv/CsvSplitterTaskTest.php @@ -0,0 +1,169 @@ + */ + 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}> + */ + 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 $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 + */ + 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> + */ + 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]; + } +}