Make iterator caching less aggressive.

CHANGELOG.md

@@ -4,6 +4,13 @@ All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
+## [0.0.3] - UNRELEASED
+
+### Fixed
+
+- Nextcloud supports only numeric log levels (in violation of PSR-3).
+- Less aggressive iterator caching, will matter once we implement pagination.
+
 ## [0.0.2] - 2023-10-14
 
 ### Changed

lib/Util/CachedIterator.php

@@ -0,0 +1,109 @@
+<?php
+
+declare(strict_types=1);
+// SPDX-FileCopyrightText: 2023 Alec Kojaev <[email protected]>
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+namespace OCA\Calibre2OPDS\Util;
+
+use ArrayIterator;
+use Iterator;
+
+/**
+ * Caching iterator.
+ *
+ * Unlike SPL's `CachingIterator`, this one doesn't touch the original iterator after the first pass.
+ *
+ * @template TKey
+ * @template TValue
+ * @implements Iterator<TKey,TValue>
+ */
+class CachedIterator implements Iterator {
+	/**
+	 * Inner iterator.
+	 *
+	 * @var Iterator<TKey,TValue>
+	 */
+	private Iterator $inner;
+	/**
+	 * Flag set on initial pass.
+	 */
+	private bool $initialPass;
+	/**
+	 * Cached iterator data.
+	 *
+	 * @var array<array<TKey,TValue>>
+	 */
+	private array $cache;
+
+	/**
+	 * Create an instance.
+	 *
+	 * @param Iterator<TKey,TValue> $inner inner iterator.
+	 */
+	public function __construct(Iterator $inner) {
+		$this->inner = $inner;
+		$this->initialPass = true;
+		$this->cache = [];
+		if ($this->inner->valid()) {
+			$this->cache[] = [ $this->key() => $this->current() ];
+		}
+	}
+
+	public function isInitialPass(): bool {
+		return $this->initialPass;
+	}
+
+	/**
+	 * @return TValue
+	 */
+	public function current(): mixed {
+		return $this->inner->current();
+	}
+
+	/**
+	 * @return TKey
+	 */
+	public function key(): mixed {
+		return $this->inner->key();
+	}
+
+	public function next(): void {
+		$this->inner->next();
+		if ($this->initialPass && $this->inner->valid()) {
+			$this->cache[] = [ $this->key() => $this->current() ];
+		}
+	}
+
+	public function rewind(): void {
+		if ($this->initialPass) {
+			while ($this->valid()) {
+				$this->next();
+			}
+			$this->initialPass = false;
+			/**
+			 * Psalm gets confused with this.
+			 * @psalm-suppress MissingTemplateParam
+			 * @psalm-suppress MixedAssignment
+			 * @psalm-suppress MixedArrayAccess
+			 * @psalm-suppress MixedArgument
+			 */
+			$this->inner = new class($this->cache) extends ArrayIterator {
+				public function current(): mixed {
+					$val = parent::current();
+					return $val[$this->key()];
+				}
+
+				public function key(): string|int|null {
+					$val = parent::current();
+					return array_key_first($val);
+				}
+			};
+		}
+		$this->inner->rewind();
+	}
+
+	public function valid(): bool {
+		return $this->inner->valid();
+	}
+}

lib/Util/MapAggregate.php

@@ -6,7 +6,8 @@
 
 namespace OCA\Calibre2OPDS\Util;
 
-use ArrayIterator;
+use Generator;
+use Iterator;
 use IteratorAggregate;
 use Traversable;
 
@@ -21,12 +22,30 @@
  * @implements IteratorAggregate<TKey,TValue>
  */
 class MapAggregate implements IteratorAggregate {
+	/**
+	 * Inner iterator.
+	 *
+	 * @var Traversable<TKey,TValueInner>
+	 */
+	private Traversable $inner;
+	/**
+	 * Mapping function.
+	 *
+	 * @var callable(TValueInner):TValue
+	 */
+	private $mapper;
+	/**
+	 * Filtering function (optional).
+	 *
+	 * @var null|callable(TValue):bool
+	 */
+	private $filter;
 	/**
 	 * Value cache.
 	 *
-	 * @var array<TKey,TValue>
+	 * @var Iterator<TKey,TValue>
 	 */
-	private array $cache;
+	private Iterator $cache;
 
 	/**
 	 * Construct an instance.
@@ -36,17 +55,33 @@ class MapAggregate implements IteratorAggregate {
 	 * @param null|callable(TValue):bool $filter optional filtering function.
 	 */
 	public function __construct(Traversable $inner, callable $mapper, ?callable $filter = null) {
-		$this->cache = [];
-		foreach ($inner as $key => $item) {
-			$value = call_user_func($mapper, $item);
-			if (is_null($filter) || call_user_func($filter, $value)) {
-				$this->cache[$key] = $value;
+		$this->inner = $inner;
+		$this->mapper = $mapper;
+		$this->filter = $filter;
+		$this->cache = new CachedIterator($this->valueGenerator());
+	}
+
+	/**
+	 * Generator for iterator values.
+	 *
+	 * @return Generator<TKey,TValue>
+	 */
+	private function valueGenerator(): Generator {
+		/**
+		 * Psalm gets royally confused here
+		 * @var TKey $key
+		 * @var TValueInner $item
+		 */
+		foreach ($this->inner as $key => $item) {
+			/** @var TValue $value */
+			$value = call_user_func($this->mapper, $item);
+			if (is_null($this->filter) || call_user_func($this->filter, $value)) {
+				yield $key => $value;
 			}
 		}
 	}
 
 	public function getIterator(): Traversable {
-		/** @psalm-suppress MixedArgumentTypeCoercion -- psalm is confused about arrays */
-		return new ArrayIterator($this->cache);
+		return $this->cache;
 	}
 }

tests/unit/CalibreTest.php

@@ -71,15 +71,17 @@ private function checkDataItem(?array $expected, ?CalibreItem $actual, string $m
 
 	private function checkData(array $expected, Traversable $actual, string $message): void {
 		reset($expected);
+		$count = 0;
 		foreach ($actual as $actualItem) {
 			$this->assertInstanceOf(CalibreItem::class, $actualItem, $message.' -- wrong type');
 			$key = key($expected);
 			$expectedItem = current($expected);
-			$this->assertFalse($expectedItem === false, $message.' -- result too long');
+			$this->assertFalse($expectedItem === false, $message.' -- result too long, checked '.$count.", expected ".count($expected));
 			$this->checkDataItem($expectedItem, $actualItem, $message.' -- key '.$key);
 			next($expected);
+			++$count;
 		}
-		$this->assertTrue(current($expected) === false, $message.' -- result too short');
+		$this->assertTrue(current($expected) === false, $message.' -- result too short, checked '.$count.", expected ".count($expected));
 	}
 
 	public function testAuthorsAll(): void {