fix(task) #37 #38 #39 #40 #41 FileFetchTask and RemoveFileTask fixes, #33 add missing tests

CHANGELOG.md

@@ -1,6 +1,16 @@
 Latest
 ------
 
+### Changes
+* [#33](https://github.com/cleverage/flysystem-process-bundle/issues/33) Add missing tests: FileFetchTask, ListContentTask and RemoveFileTask on local storages, bundle and DI extension.
+
+### Fixes
+* [#37](https://github.com/cleverage/flysystem-process-bundle/issues/37) Fix FileFetchTask: list the source storage once per input (it was listed again before each file). Update documentation, add tests.
+* [#38](https://github.com/cleverage/flysystem-process-bundle/issues/38) Fix FileFetchTask: process each input (with `file_pattern`, only the first input copied the files; a path received twice was skipped). Update documentation, add tests.
+* [#39](https://github.com/cleverage/flysystem-process-bundle/issues/39) Fix FileFetchTask: apply `ignore_missing` to the files given as input (a missing file threw `UnableToReadFile`). Update documentation, add tests.
+* [#40](https://github.com/cleverage/flysystem-process-bundle/issues/40) Fix FileFetchTask and RemoveFileTask: a file named `0` is no longer handled as no file. Add tests.
+* [#41](https://github.com/cleverage/flysystem-process-bundle/issues/41) Fix RemoveFileTask: a missing file is logged as not found (it was logged as deleted), accept a list of paths, fix the deletion failure message. Update documentation, add tests.
+
 v3.1
 ------
 

composer.json

@@ -66,6 +66,7 @@
         "phpunit/phpunit": "^10.5|^11|^12|^13",
         "rector/rector": "*",
         "roave/security-advisories": "dev-latest",
+        "symfony/filesystem": "^6.4|^7.4|^8",
         "symfony/test-pack": "^1.1"
     },
     "config": {

docs/reference/tasks/file_fetch_task.md

@@ -30,19 +30,19 @@ Possible outputs
 `string`: for each copied file, its path relative to the storage root (the same path is used in the
 `source_filesystem` and in the `destination_filesystem`).
 
-When no file matches `file_pattern` (and `ignore_missing` is `true`), or when every received file has already been
-copied during this process execution, no output is produced and the task is skipped.
+When no file matches `file_pattern`, or when no file given as input exists (and `ignore_missing` is `true`), no
+output is produced and the task is skipped.
 
 Options
 -------
 
-| Code                     | Type           | Required | Default | Description                                                                                                                                                                                                                                |
-|--------------------------|----------------|:--------:|---------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
-| `source_filesystem`      | `string`       |  **X**   |         | Name of the Flysystem storage to read files from, as configured under `flysystem.storages` (see [configuration](../../index.md#configuration))                                                                                             |
-| `destination_filesystem` | `string`       |  **X**   |         | Name of the Flysystem storage to write files to, as configured under `flysystem.storages`                                                                                                                                                  |
-| `file_pattern`           | `string\|null` |          | `null`  | Regular expression (see [preg_match](https://www.php.net/manual/en/function.preg-match.php)) tested on the path of each file at the root of `source_filesystem`. If `null` (or empty), the file path(s) are taken from the input |
-| `remove_source`          | `bool`         |          | `false` | Delete the file from `source_filesystem` after the copy (move instead of copy)                                                                                                                                                             |
-| `ignore_missing`         | `bool`         |          | `true`  | Only used with `file_pattern`: if `false`, throw an `\UnexpectedValueException` (`File(s) not found in source filesystem`) when no file matches the pattern. If `true`, the task is skipped                                        |
+| Code                     | Type           | Required | Default | Description                                                                                                                                                                                                                                                       |
+|--------------------------|----------------|:--------:|---------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| `source_filesystem`      | `string`       |  **X**   |         | Name of the Flysystem storage to read files from, as configured under `flysystem.storages` (see [configuration](../../index.md#configuration))                                                                                                                    |
+| `destination_filesystem` | `string`       |  **X**   |         | Name of the Flysystem storage to write files to, as configured under `flysystem.storages`                                                                                                                                                                         |
+| `file_pattern`           | `string\|null` |          | `null`  | Regular expression (see [preg_match](https://www.php.net/manual/en/function.preg-match.php)) tested on the path of each file at the root of `source_filesystem`. If `null` (or empty), the file path(s) are taken from the input                                  |
+| `remove_source`          | `bool`         |          | `false` | Delete the file from `source_filesystem` after the copy (move instead of copy)                                                                                                                                                                                    |
+| `ignore_missing`         | `bool`         |          | `true`  | If `false`, throw an `\UnexpectedValueException` when no file matches `file_pattern` (`File(s) not found in source filesystem`) or when a file given as input does not exist (`File <path> not found in source filesystem`). If `true`, missing files are skipped |
 
 Examples
 --------
@@ -101,14 +101,14 @@ Notes
 * `file_pattern` is only tested on the files located at the root of `source_filesystem` (the listing is not
   recursive and directories are ignored). The pattern is tested on the path relative to the storage root: to target
   a sub-directory, configure a dedicated storage whose root is this directory.
-* The source storage is listed again before each iteration: files added to the source during the iteration are also
-  copied. With an SFTP storage and long-running downstream tasks, see
-  [SFTP stale connection](../../troubleshooting.md).
-* A given file is copied only once per process execution: already copied paths are remembered, so receiving the same
-  path again (or a new input while using `file_pattern`) skips the task.
-* The file is written to `destination_filesystem` with the same path, overwriting any existing file. A file given as
-  input that does not exist in `source_filesystem` throws a `League\Flysystem\UnableToReadFile` exception, whatever
-  the value of `ignore_missing`.
+* With `file_pattern`, the source storage is listed once per input, when the iteration starts: files added to the
+  source during the iteration are copied by the next execution of the task. With an SFTP storage and long-running
+  downstream tasks, see [SFTP stale connection](../../troubleshooting.md).
+* Each input is processed: the files are copied for each input received by the task (e.g. after an iterable task),
+  even if they have already been copied during this process execution. A path given several times in the same input
+  is copied once.
+* The file is written to `destination_filesystem` with the same path, overwriting any existing file. The existence of
+  the files given as input is checked in `source_filesystem` (see `ignore_missing`).
 * A failure while writing to `destination_filesystem` throws a `League\Flysystem\FilesystemException` (e.g.
   `UnableToWriteFile`): the task's `error_strategy` applies, and with `remove_source: true` the source file is kept.
 * To read a copied file with a core task (e.g.

docs/reference/tasks/remove_file_task.md

@@ -12,11 +12,10 @@ Accepted inputs
 ---------------
 
 * When `file_pattern` is set, the input is ignored (but the deletion is run again each time the task is executed).
-* Otherwise, `string`: path of the file to delete, relative to the root of the `filesystem` storage. An empty input
-  throws an `\UnexpectedValueException` (`No pattern neither input provided for the Task`). A list of paths is not
-  supported: iterate over it first (e.g. with
-  [InputIteratorTask](https://github.com/cleverage/process-bundle/blob/main/docs/reference/tasks/input_iterator_task.md)).
-  A `StorageAttributes` output of [ListContentTask](list_content_task.md) must be converted to its `path` first.
+* Otherwise, `string|array<string>`: path, or list of paths, of the file(s) to delete, relative to the root of the
+  `filesystem` storage. An empty input throws an `\UnexpectedValueException` (`No pattern neither input provided for
+  the Task`). A `StorageAttributes` output of [ListContentTask](list_content_task.md) must be converted to its `path`
+  first.
 
 Possible outputs
 ----------------
@@ -65,8 +64,8 @@ Notes
 * `file_pattern` is only tested on the files located at the root of the storage (the listing is not recursive and
   directories are ignored).
 * Deletion errors do not stop the process: each deleted file is logged with the `info` level (`Deleted input file`),
-  and a deletion failure (`League\Flysystem\FilesystemException`) is logged with the `warning` level, with the file
-  path in the log context. Deleting a file that does not exist is not an error for most adapters (e.g. `local`,
-  `sftp`): it is logged as deleted.
+  and a deletion failure (`League\Flysystem\FilesystemException`) is logged with the `warning` level
+  (`Failed to delete input file`), with the file path in the log context. A file given as input that does not exist
+  (or is a directory) is not deleted and is logged with the `warning` level (`Input file not found`).
 * The storage is resolved on each execution: an unknown storage name makes the task fail when it is executed.
 * See the [Remote cleanup](../../cookbooks/remote_cleanup.md) cookbook.

docs/troubleshooting.md

@@ -16,7 +16,7 @@ Connection closed prematurely
 
 Typically the first SFTP task succeeds (a file is fetched/listed), then the process spends a long
 time doing other work (large CSV import, thousands of API calls, …), and a subsequent SFTP task —
-often the archiving step, or the next iteration of an iterable task that re-lists the source — blows
+often the archiving step, or the listing of the source for the next input of an iterable task — blows
 up with the error above.
 
 ### Cause

src/Task/FileFetchTask.php

@@ -32,9 +32,11 @@ class FileFetchTask extends AbstractConfigurableTask implements IterableTaskInte
     protected FilesystemOperator $destinationFS;
 
     /**
-     * @var array<int, string>
+     * Files of the current input, null when no iteration is in progress.
+     *
+     * @var list<string>|null
      */
-    protected array $matchingFiles = [];
+    protected ?array $matchingFiles = null;
 
     /**
      * @param ServiceLocator<FilesystemOperator> $storages
@@ -67,10 +69,12 @@ public function initialize(ProcessState $state): void
      */
     public function execute(ProcessState $state): void
     {
-        $this->findMatchingFiles($state);
+        // The files are listed once per input, when its iteration starts
+        $this->matchingFiles ??= $this->findMatchingFiles($state);
 
         $file = current($this->matchingFiles);
-        if (!$file) {
+        if (false === $file) {
+            $this->matchingFiles = null;
             $state->setSkipped(true);
 
             return;
@@ -82,55 +86,62 @@ public function execute(ProcessState $state): void
         $state->setOutput($file);
     }
 
-    /**
-     * @throws \UnexpectedValueException
-     * @throws \InvalidArgumentException
-     * @throws FilesystemException
-     */
     public function next(ProcessState $state): bool
     {
-        $this->findMatchingFiles($state);
+        if (null === $this->matchingFiles) {
+            return false;
+        }
+        if (false !== next($this->matchingFiles)) {
+            return true;
+        }
+
+        // End of the iteration: the next input lists its files again
+        $this->matchingFiles = null;
 
-        return false !== next($this->matchingFiles);
+        return false;
     }
 
     /**
+     * @return list<string>
+     *
      * @throws \UnexpectedValueException
      * @throws \InvalidArgumentException
      * @throws FilesystemException
      */
-    protected function findMatchingFiles(ProcessState $state): void
+    protected function findMatchingFiles(ProcessState $state): array
     {
+        /** @var bool $ignoreMissing */
+        $ignoreMissing = $this->getOption($state, 'ignore_missing');
+        $matchingFiles = [];
+
         /** @var ?string $filePattern */
         $filePattern = $this->getOption($state, 'file_pattern');
-        if ($filePattern) {
+        if (null !== $filePattern && '' !== $filePattern) {
             foreach ($this->sourceFS->listContents('/') as $file) {
-                if ('file' === $file->type()
-                    && preg_match($filePattern, $file->path())
-                    && !\in_array($file->path(), $this->matchingFiles, true)
-                ) {
-                    $this->matchingFiles[] = $file->path();
+                if ('file' === $file->type() && preg_match($filePattern, $file->path())) {
+                    $matchingFiles[] = $file->path();
                 }
             }
         } else {
-            /** @var array<string>|string|null $input */
             $input = $state->getInput();
-            if (!$input) {
+            if (null === $input || '' === $input || [] === $input) {
                 throw new \UnexpectedValueException('No pattern neither input provided for the Task');
             }
-            if (\is_array($input)) {
-                foreach ($input as $file) {
-                    if (!\in_array($file, $this->matchingFiles, true)) {
-                        $this->matchingFiles[] = $file;
-                    }
+            /** @var list<string> $files */
+            $files = \is_array($input) ? array_values($input) : [$input];
+            foreach (array_unique($files) as $file) {
+                if ($this->sourceFS->fileExists($file)) {
+                    $matchingFiles[] = $file;
+                } elseif (!$ignoreMissing) {
+                    throw new \UnexpectedValueException("File {$file} not found in source filesystem");
                 }
-            } elseif (!\in_array($input, $this->matchingFiles, true)) {
-                $this->matchingFiles[] = $input;
             }
         }
-        if ([] === $this->matchingFiles && !$this->getOption($state, 'ignore_missing')) {
+        if ([] === $matchingFiles && !$ignoreMissing) {
             throw new \UnexpectedValueException('File(s) not found in source filesystem');
         }
+
+        return $matchingFiles;
     }
 
     /**

src/Task/RemoveFileTask.php

@@ -52,26 +52,37 @@ public function execute(ProcessState $state): void
 
         /** @var ?string $filePattern */
         $filePattern = $this->getOption($state, 'file_pattern');
-        if ($filePattern) {
+        if (null !== $filePattern && '' !== $filePattern) {
             foreach ($this->filesystem->listContents('/') as $file) {
                 if ('file' === $file->type() && preg_match($filePattern, $file->path())) {
-                    $this->deleteFile($file->path());
+                    $this->deleteFile($file->path(), false);
                 }
             }
         } else {
-            /** @var ?string $input */
             $input = $state->getInput();
-            if (!$input) {
+            if (null === $input || '' === $input || [] === $input) {
                 throw new \UnexpectedValueException('No pattern neither input provided for the Task');
             }
 
-            $this->deleteFile($input);
+            /** @var list<string> $files */
+            $files = \is_array($input) ? array_values($input) : [$input];
+            foreach ($files as $file) {
+                $this->deleteFile($file, true);
+            }
         }
     }
 
-    private function deleteFile(string $filePath): void
+    /**
+     * @param bool $checkExists Most adapters (e.g. local, sftp) do not fail when deleting a missing file
+     */
+    private function deleteFile(string $filePath, bool $checkExists): void
     {
         try {
+            if ($checkExists && !$this->filesystem->fileExists($filePath)) {
+                $this->logger->warning('Input file not found', ['file' => $filePath]);
+
+                return;
+            }
             $this->filesystem->delete($filePath);
             $result = true;
         } catch (FilesystemException) {
@@ -81,7 +92,7 @@ private function deleteFile(string $filePath): void
         if ($result) {
             $this->logger->info('Deleted input file', ['file' => $filePath]);
         } else {
-            $this->logger->warning('Failed to deleted input file', ['file' => $filePath]);
+            $this->logger->warning('Failed to delete input file', ['file' => $filePath]);
         }
     }
 }

tests/CleverAgeFlysystemProcessBundleTest.php

@@ -0,0 +1,30 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the CleverAge/FlysystemProcessBundle 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\FlysystemProcessBundle\Tests;
+
+use CleverAge\FlysystemProcessBundle\CleverAgeFlysystemProcessBundle;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\TestCase;
+
+#[CoversClass(CleverAgeFlysystemProcessBundle::class)]
+class CleverAgeFlysystemProcessBundleTest extends TestCase
+{
+    public function testPathIsTheBundleRoot(): void
+    {
+        $path = (new CleverAgeFlysystemProcessBundle())->getPath();
+
+        self::assertSame(\dirname(__DIR__), $path);
+        self::assertDirectoryExists($path.'/config/services');
+    }
+}