Fix review regressions and streamline protocol test fixtures

Author: stevebauman

src/Connection/CommandArgument.php

@@ -40,7 +40,11 @@ public static function literal(array|string $string): array|string
             return $result;
         }
 
-        if (str_contains($string, "\r") || str_contains($string, "\n")) {
+        if (str_contains($string, "\0")) {
+            throw new InvalidArgumentException('IMAP strings cannot contain NUL bytes.');
+        }
+
+        if (preg_match('/[^\x20-\x7E]/', $string)) {
             return ['{'.strlen($string).'}', $string];
         }
 

src/Connection/Streams/FakeStream.php

@@ -45,6 +45,46 @@ class FakeStream implements StreamInterface
         'stream_type' => 'tcp_socket/unknown',
     ];
 
+    /**
+     * The failure to report once all queued responses have been read.
+     */
+    protected ?string $failureWhenEmpty = null;
+
+    /**
+     * The most recently configured read timeout.
+     */
+    protected ?int $timeout = null;
+
+    /**
+     * Simulate a disconnection after the queued responses are consumed.
+     */
+    public function disconnectWhenEmpty(): self
+    {
+        $this->failureWhenEmpty = 'eof';
+
+        return $this;
+    }
+
+    /**
+     * Simulate a timeout after the queued responses are consumed.
+     */
+    public function timeoutWhenEmpty(): self
+    {
+        $this->failureWhenEmpty = 'timed_out';
+
+        return $this;
+    }
+
+    /**
+     * Apply the scripted failure when there are no responses left to read.
+     */
+    protected function failWhenEmpty(): void
+    {
+        if (! $this->buffer && $this->failureWhenEmpty !== null) {
+            $this->meta[$this->failureWhenEmpty] = true;
+        }
+    }
+
     /**
      * Feed a line to the stream buffer with a newline character.
      */
@@ -99,6 +139,8 @@ public function setMeta(string $attribute, mixed $value): self
      */
     public function open(?string $transport = null, ?string $host = null, ?int $port = null, ?int $timeout = null, array $options = []): bool
     {
+        $this->timeout = $timeout;
+
         $this->connection = compact('transport', 'host', 'port', 'timeout', 'options');
 
         return true;
@@ -122,8 +164,10 @@ public function read(int $length): string|false
             return false;
         }
 
-        if ($this->meta['eof'] && empty($this->buffer)) {
-            return false; // EOF and no data left. Indicate end of stream.
+        $this->failWhenEmpty();
+
+        if ($this->meta['timed_out'] || ($this->meta['eof'] && empty($this->buffer))) {
+            return false;
         }
 
         $data = implode('', $this->buffer);
@@ -156,6 +200,8 @@ public function fgets(): string|false
             return false;
         }
 
+        $this->failWhenEmpty();
+
         // Simulate timeout/eof checks.
         if ($this->meta['timed_out'] || $this->meta['eof']) {
             return false;
@@ -199,9 +245,19 @@ public function opened(): bool
      */
     public function setTimeout(int $seconds): bool
     {
+        $this->timeout = $seconds;
+
         return true;
     }
 
+    /**
+     * Get the most recently configured read timeout.
+     */
+    public function timeout(): ?int
+    {
+        return $this->timeout;
+    }
+
     /**
      * {@inheritDoc}
      */

src/Folder.php

@@ -220,6 +220,8 @@ public function examine(): array
      */
     public function expunge(array|int|null $uids = null): array
     {
+        $this->select();
+
         return $this->mailbox->connection()->expunge($uids)->map(
             fn (UntaggedResponse $response) => $response->tokenAt(1)->value
         )->all();

src/Idle.php

@@ -2,11 +2,13 @@
 
 namespace DirectoryTree\ImapEngine;
 
+use DirectoryTree\ImapEngine\Exceptions\ImapConnectionClosedException;
 use DirectoryTree\ImapEngine\Idle\Events\EventInterface;
 use DirectoryTree\ImapEngine\Idle\Events\FolderSelected;
 use DirectoryTree\ImapEngine\Idle\Events\MessagesExist;
 use DirectoryTree\ImapEngine\Selection\OptionInterface;
 use DirectoryTree\ImapEngine\Selection\Result;
+use Generator;
 
 class Idle
 {
@@ -61,11 +63,45 @@ public function await(callable $callback, ?callable $query = null, callable|int
      * Retrieve arrivals and deliver them in UID order.
      */
     protected function deliver(callable $callback, ?callable $query): ?bool
+    {
+        foreach ($this->messages($query) as $message) {
+            if ($callback($message) === false) {
+                return false;
+            }
+
+            $this->nextUid = $message->uid() + 1;
+        }
+
+        return null;
+    }
+
+    /**
+     * Retrieve arrivals, retrying once if the application connection is lost.
+     *
+     * @return Generator<int, MessageInterface>
+     */
+    protected function messages(?callable $query): Generator
+    {
+        try {
+            yield from $this->fetch($query);
+        } catch (ImapConnectionClosedException) {
+            $this->folder->mailbox()->reconnect();
+
+            yield from $this->fetch($query);
+        }
+    }
+
+    /**
+     * Fetch arrivals that belong to the watching connection's UID validity.
+     *
+     * @return Generator<int, MessageInterface>
+     */
+    protected function fetch(?callable $query): Generator
     {
         $current = $this->folder->select(true);
 
         if ($this->selection->uidValidity() !== null && $current->uidValidity() !== $this->selection->uidValidity()) {
-            return null;
+            return;
         }
 
         $messages = $this->folder->messages()->with(MessageData::flags());
@@ -74,18 +110,10 @@ protected function deliver(callable $callback, ?callable $query): ?bool
 
         foreach ($messages->uid($this->nextUid.':*')->orderByUid()->cursor() as $message) {
             // Reversed IMAP ranges can include an older UID when no arrivals exist.
-            if ($message->uid() < $this->nextUid) {
-                continue;
+            if ($message->uid() >= $this->nextUid) {
+                yield $message;
             }
-
-            if ($callback($message) === false) {
-                return false;
-            }
-
-            $this->nextUid = $message->uid() + 1;
         }
-
-        return null;
     }
 
     /**

src/Testing/FakeFolderRepository.php

@@ -25,9 +25,7 @@ class FakeFolderRepository implements FolderRepositoryInterface
     public function __construct(
         protected MailboxInterface $mailbox,
         protected FolderCollection $folders = new FolderCollection
-    ) {
-        $this->folders = clone $folders;
-    }
+    ) {}
 
     /**
      * {@inheritDoc}

tests/Support/ScriptedFolder.php

@@ -0,0 +1,33 @@
+<?php
+
+namespace Tests\Support;
+
+use DirectoryTree\ImapEngine\Folder;
+use DirectoryTree\ImapEngine\Idle\Events\EventInterface;
+use DirectoryTree\ImapEngine\Mailbox;
+use DirectoryTree\ImapEngine\Selection\OptionInterface;
+
+class ScriptedFolder extends Folder
+{
+    /**
+     * Create a folder with events to deliver to its watcher.
+     *
+     * @param  array<EventInterface>  $events
+     */
+    public function __construct(Mailbox $mailbox, string $path, protected array $events)
+    {
+        parent::__construct($mailbox, $path);
+    }
+
+    /**
+     * Deliver scripted events until the consumer stops watching.
+     */
+    public function events(callable $callback, callable|int $timeout = 300, OptionInterface ...$options): void
+    {
+        foreach ($this->events as $event) {
+            if ($callback($event) === false) {
+                break;
+            }
+        }
+    }
+}

tests/Support/ScriptedMailbox.php

@@ -0,0 +1,41 @@
+<?php
+
+namespace Tests\Support;
+
+use DirectoryTree\ImapEngine\Connection\ConnectionInterface;
+use DirectoryTree\ImapEngine\Mailbox;
+use Illuminate\Support\Collection;
+use LogicException;
+
+class ScriptedMailbox extends Mailbox
+{
+    /**
+     * Sessions shared with cloned mailboxes, in connection order.
+     *
+     * @var Collection<int, ConnectionInterface>
+     */
+    protected Collection $connections;
+
+    /**
+     * Create a mailbox with the sessions it is allowed to open.
+     */
+    public function __construct(ConnectionInterface ...$connections)
+    {
+        parent::__construct();
+
+        $this->connections = new Collection($connections);
+    }
+
+    /**
+     * Connect using the next scripted session.
+     */
+    public function connect(?ConnectionInterface $connection = null): void
+    {
+        if ($this->connected()) {
+            return;
+        }
+
+        parent::connect($connection ?? $this->connections->shift()
+            ?? throw new LogicException('No scripted connections remain.'));
+    }
+}

tests/Unit/Connection/CommandArgumentTest.php

@@ -53,3 +53,11 @@
 test('list returns empty parentheses for an empty array', function () {
     expect(CommandArgument::list([]))->toBe('()');
 });
+
+test('literal preserves control and non-ASCII bytes', function (string $input) {
+    expect(CommandArgument::literal($input))->toBe(['{'.strlen($input).'}', $input]);
+})->with(["password\tvalue", "value\x01", "value\x7f", 'café', "value\xff"]);
+
+test('literal rejects NUL bytes', function () {
+    expect(fn () => CommandArgument::literal("pass\0word"))->toThrow(InvalidArgumentException::class);
+});

tests/Unit/Connection/FakeStreamTest.php

@@ -0,0 +1,41 @@
+<?php
+
+use DirectoryTree\ImapEngine\Connection\Streams\FakeStream;
+
+test('scripted stream failures preserve queued responses before failing', function (string $failure, string $metadata, bool $readBytes) {
+    $stream = (new FakeStream)->{$failure}();
+    $stream->open();
+    $stream->feed(['first', 'second']);
+
+    if ($readBytes) {
+        expect($stream->read(3))->toBe('fir');
+        expect($stream->meta()[$metadata])->toBeFalse();
+        expect($stream->read(100))->toBe("st\r\nsecond\r\n");
+    } else {
+        expect($stream->fgets())->toBe("first\r\n");
+        expect($stream->meta()[$metadata])->toBeFalse();
+        expect($stream->fgets())->toBe("second\r\n");
+    }
+
+    expect($stream->meta()[$metadata])->toBeFalse();
+    expect($readBytes ? $stream->read(1) : $stream->fgets())->toBeFalse();
+    expect($stream->meta()[$metadata])->toBeTrue();
+})->with([
+    'disconnect while reading lines' => ['disconnectWhenEmpty', 'eof', false],
+    'disconnect while reading bytes' => ['disconnectWhenEmpty', 'eof', true],
+    'timeout while reading lines' => ['timeoutWhenEmpty', 'timed_out', false],
+    'timeout while reading bytes' => ['timeoutWhenEmpty', 'timed_out', true],
+]);
+
+test('an unscripted empty stream does not report a disconnect or timeout', function () {
+    $stream = new FakeStream;
+    $stream->open();
+
+    expect($stream->fgets())->toBeFalse();
+    expect($stream->read(1))->toBe('');
+    expect($stream->meta())->toMatchArray(['eof' => false, 'timed_out' => false]);
+
+    $stream->feed('arrived later');
+
+    expect($stream->fgets())->toBe("arrived later\r\n");
+});