fix: issues found in a full package audit

README.md

@@ -878,6 +878,11 @@ foreach (Batch::process($posts, $em) as $post) {
 > `Batch::iterate()` never flushes. Changes you make to an entity while iterating are silently discarded when
 > the chunk is cleared - use `Batch::process()` if you intend to write.
 
+> [!IMPORTANT]
+> `Batch::process()` opens _one_ transaction for the whole loop, not one per chunk. Nothing is left half-done
+> if it fails, but the transaction is held for the entire run - worth keeping in mind when processing very
+> large sets, where it means long-lived locks.
+
 Both take a chunk size, defaulting to `100`:
 
 ```php

src/Collection/ArrayCollection.php

@@ -116,7 +116,11 @@ public static function fill(int $start, int $count, mixed $value): self
 
     public function first(mixed $default = null): mixed
     {
-        return $this->source[\array_key_first($this->source) ?? ''] ?? $default;
+        if (null === $key = \array_key_first($this->source)) {
+            return $default;
+        }
+
+        return $this->source[$key];
     }
 
     /**

src/Collection/Doctrine/ORM/EntityResult.php

@@ -93,13 +93,9 @@ public function readonly(): self
     public function first(mixed $default = null): mixed
     {
         $query = $this->query();
-        $sql = $query->getSQL();
 
-        if (\is_array($sql)) {
-            $sql = \implode(' ', $sql);
-        }
-
-        if (\str_starts_with(\mb_strtolower($sql), 'delete')) {
+        // check the DQL, not the SQL - the latter compiles the query, which setMaxResults() below invalidates
+        if (\str_starts_with(\mb_strtoupper($query->getDQL() ?? ''), 'DELETE')) {
             return $this->normalizeResult($query->execute());
         }
 

src/Collection/Doctrine/ORM/Specification/QueryBuilderInterpreter.php

@@ -110,7 +110,8 @@ private function transform(object $specification): mixed
 
             OrderBy::class => $this->qb->addOrderBy($this->prefix($specification->field), $specification->direction),
 
-            Instance::class => $this->qb->expr()->isInstanceOf($this->alias, $this->param($specification->of())),
+            // the class name must be inlined - as a parameter it is used as the discriminator value verbatim
+            Instance::class => $this->qb->expr()->isInstanceOf($this->alias, $specification->of()),
             Delete::class => $this->qb->delete(),
             Unwritable::class => $this->qb->readonly(),
             Cache::class => $this->qb->cacheResult($specification->lifetime(), $specification->key()),

src/Collection/Page.php

@@ -31,6 +31,9 @@ final class Page implements \IteratorAggregate, \Countable
     private int $limit;
     private bool $strict = false;
 
+    /** @var non-negative-int */
+    private int $totalCount;
+
     /** @var Collection<V,K> */
     private Collection $cachedPage;
 
@@ -97,7 +100,7 @@ public function count(): int
 
     public function totalCount(): int
     {
-        return $this->collection->count();
+        return $this->totalCount ??= $this->collection->count();
     }
 
     public function getIterator(): \Traversable

src/Collection/Pages.php

@@ -45,18 +45,14 @@ public function get(int $page): Page
 
     public function getIterator(): \Traversable
     {
-        if (0 === $this->count()) {
-            return;
-        }
-
-        for ($page = 1; $page <= $this->count(); ++$page) {
+        for ($page = 1, $count = $this->count(); $page <= $count; ++$page) {
             yield $this->get($page);
         }
     }
 
     public function count(): int
     {
-        if (0 === $this->page1()->count()) {
+        if (0 === $this->page1()->totalCount()) {
             return 0;
         }
 

tests/ArrayCollectionTest.php

@@ -194,6 +194,15 @@ public function combine_with_self(): void
         );
     }
 
+    /**
+     * @test
+     */
+    public function first_null_value_is_not_the_default(): void
+    {
+        $this->assertNull(Arr::for([null, 'a'])->first('default'));
+        $this->assertSame('default', Arr::for([])->first('default'));
+    }
+
     /**
      * @test
      */

tests/Doctrine/Fixture/Circle.php

@@ -0,0 +1,22 @@
+<?php
+
+/*
+ * This file is part of the zenstruck/collection package.
+ *
+ * (c) Kevin Bond <kevinbond@gmail.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Zenstruck\Collection\Tests\Doctrine\Fixture;
+
+use Doctrine\ORM\Mapping as ORM;
+
+/**
+ * @author Kevin Bond <kevinbond@gmail.com>
+ */
+#[ORM\Entity]
+class Circle extends Shape
+{
+}

tests/Doctrine/Fixture/Shape.php

@@ -0,0 +1,34 @@
+<?php
+
+/*
+ * This file is part of the zenstruck/collection package.
+ *
+ * (c) Kevin Bond <kevinbond@gmail.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Zenstruck\Collection\Tests\Doctrine\Fixture;
+
+use Doctrine\ORM\Mapping as ORM;
+
+/**
+ * @author Kevin Bond <kevinbond@gmail.com>
+ */
+#[ORM\Entity]
+#[ORM\Table(name: 'shapes')]
+#[ORM\InheritanceType('SINGLE_TABLE')]
+#[ORM\DiscriminatorColumn(name: 'type', type: 'string')]
+#[ORM\DiscriminatorMap(['circle' => Circle::class, 'square' => Square::class])]
+abstract class Shape
+{
+    #[ORM\Id]
+    #[ORM\Column]
+    #[ORM\GeneratedValue]
+    public ?int $id = null;
+
+    public function __construct(#[ORM\Column] public string $name)
+    {
+    }
+}

tests/Doctrine/Fixture/Square.php

@@ -0,0 +1,22 @@
+<?php
+
+/*
+ * This file is part of the zenstruck/collection package.
+ *
+ * (c) Kevin Bond <kevinbond@gmail.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Zenstruck\Collection\Tests\Doctrine\Fixture;
+
+use Doctrine\ORM\Mapping as ORM;
+
+/**
+ * @author Kevin Bond <kevinbond@gmail.com>
+ */
+#[ORM\Entity]
+class Square extends Shape
+{
+}

tests/Doctrine/HasDatabase.php

@@ -20,6 +20,7 @@
 use Symfony\Bridge\Doctrine\Middleware\Debug\DebugDataHolder;
 use Zenstruck\Collection\Tests\Doctrine\Fixture\Entity;
 use Zenstruck\Collection\Tests\Doctrine\Fixture\Relation;
+use Zenstruck\Collection\Tests\Doctrine\Fixture\Shape;
 
 /**
  * @author Kevin Bond <kevinbond@gmail.com>
@@ -52,6 +53,7 @@ protected function setupEntityManager(): void
         $schemaTool->createSchema([
             $this->em->getClassMetadata(Entity::class),
             $this->em->getClassMetadata(Relation::class),
+            $this->em->getClassMetadata(Shape::class),
         ]);
     }
 

tests/Doctrine/ORM/EntityRepositoryTest.php

@@ -14,6 +14,7 @@
 use Doctrine\Common\Collections\Criteria;
 use Doctrine\ORM\QueryBuilder;
 use PHPUnit\Framework\TestCase;
+use Symfony\Component\Cache\Adapter\ArrayAdapter;
 use Zenstruck\Collection\Doctrine\Batch\CountableBatchIterator;
 use Zenstruck\Collection\Doctrine\DoctrineSpec;
 use Zenstruck\Collection\Doctrine\ObjectRepository;
@@ -22,8 +23,11 @@
 use Zenstruck\Collection\Exception\InvalidSpecification;
 use Zenstruck\Collection\Spec;
 use Zenstruck\Collection\Tests\CountableIteratorTests;
+use Zenstruck\Collection\Tests\Doctrine\Fixture\Circle;
 use Zenstruck\Collection\Tests\Doctrine\Fixture\Entity;
 use Zenstruck\Collection\Tests\Doctrine\Fixture\Relation;
+use Zenstruck\Collection\Tests\Doctrine\Fixture\Shape;
+use Zenstruck\Collection\Tests\Doctrine\Fixture\Square;
 use Zenstruck\Collection\Tests\Doctrine\HasDatabase;
 use Zenstruck\Collection\Tests\MatchableObjectTests;
 
@@ -295,6 +299,39 @@ public function sql_wildcards_in_value_are_literal(): void
         $this->assertCount(0, $repo->filter(Spec::endsWith('value', '_')));
     }
 
+    /**
+     * @test
+     */
+    public function instance_of_specification(): void
+    {
+        $this->em->persist(new Circle('c1'));
+        $this->em->persist(new Circle('c2'));
+        $this->em->persist(new Square('s1'));
+        $this->flushAndClear();
+
+        $shapes = new EntityRepository($this->em, Shape::class);
+
+        $this->assertCount(3, $shapes);
+        $this->assertCount(2, $shapes->filter(DoctrineSpec::instanceOf(Circle::class)));
+        $this->assertCount(1, $shapes->filter(DoctrineSpec::instanceOf(Square::class)));
+    }
+
+    /**
+     * @test
+     */
+    public function cache_specification(): void
+    {
+        $this->em->getConfiguration()->setResultCache(new ArrayAdapter());
+
+        $repo = $this->createWithItems(3);
+
+        $this->assertCount(3, $repo->filter(DoctrineSpec::cache(60, 'my-key'))->eager());
+
+        $this->assertQueryCount(0, function() use ($repo) {
+            $this->assertCount(3, $repo->filter(DoctrineSpec::cache(60, 'my-key'))->eager());
+        });
+    }
+
     /**
      * @test
      */

tests/Doctrine/ORM/EntityResultQueryBuilderTest.php

@@ -9,9 +9,11 @@
  * file that was distributed with this source code.
  */
 
-namespace Doctrine\ORM;
+namespace Zenstruck\Collection\Tests\Doctrine\ORM;
 
+use Doctrine\ORM\Query;
 use PHPUnit\Framework\TestCase;
+use Symfony\Component\Cache\Adapter\ArrayAdapter;
 use Zenstruck\Collection\Doctrine\ORM\EntityResultQueryBuilder;
 use Zenstruck\Collection\Tests\Doctrine\Fixture\Entity;
 use Zenstruck\Collection\Tests\Doctrine\HasDatabase;
@@ -23,6 +25,41 @@ final class EntityResultQueryBuilderTest extends TestCase
 {
     use HasDatabase;
 
+    /**
+     * @test
+     */
+    public function can_modify_the_query(): void
+    {
+        $this->persistEntities(3);
+
+        $result = EntityResultQueryBuilder::forEntity($this->em, Entity::class, 'e')
+            ->modifyQuery(static fn(Query $query) => $query->setMaxResults(2))
+            ->result()
+        ;
+
+        $this->assertCount(2, $result->eager());
+    }
+
+    /**
+     * @test
+     */
+    public function can_cache_the_result(): void
+    {
+        $this->em->getConfiguration()->setResultCache(new ArrayAdapter());
+        $this->persistEntities(3);
+
+        $result = EntityResultQueryBuilder::forEntity($this->em, Entity::class, 'e')
+            ->cacheResult(60, 'my-key')
+            ->result()
+        ;
+
+        $this->assertCount(3, $result->eager());
+
+        $this->assertQueryCount(0, function() use ($result) {
+            $this->assertCount(3, $result->eager());
+        });
+    }
+
     /**
      * @test
      */

tests/Doctrine/ORM/Result/SingleScalarTest.php

@@ -9,7 +9,7 @@
  * file that was distributed with this source code.
  */
 
-namespace Doctrine\ORM\Result;
+namespace Zenstruck\Collection\Tests\Doctrine\ORM\Result;
 
 use PHPUnit\Framework\TestCase;
 use Zenstruck\Collection\Doctrine\ORM\EntityResultQueryBuilder;

tests/PagesTest.php

@@ -12,6 +12,7 @@
 namespace Zenstruck\Collection\Tests;
 
 use PHPUnit\Framework\TestCase;
+use Zenstruck\Collection\CallbackCollection;
 use Zenstruck\Collection\LazyCollection;
 use Zenstruck\Collection\Pages;
 
@@ -20,6 +21,46 @@
  */
 final class PagesTest extends TestCase
 {
+    /**
+     * @test
+     */
+    public function counting_does_not_load_the_first_page(): void
+    {
+        $iterations = 0;
+        $collection = new CallbackCollection(
+            static function() use (&$iterations) {
+                ++$iterations;
+
+                yield from \range(1, 10);
+            },
+            static fn() => 10,
+        );
+
+        $this->assertCount(4, new Pages($collection, 3));
+        $this->assertSame(0, $iterations);
+    }
+
+    /**
+     * @test
+     */
+    public function iterating_only_counts_the_collection_once(): void
+    {
+        $counts = 0;
+        $collection = new CallbackCollection(
+            static fn() => yield from \range(1, 10),
+            static function() use (&$counts) {
+                ++$counts;
+
+                return 10;
+            },
+        );
+
+        $pages = new Pages($collection, 3);
+
+        $this->assertCount(4, \iterator_to_array($pages));
+        $this->assertSame(1, $counts);
+    }
+
     /**
      * @test
      */