Hotfix/identifier conversion coverage

CLAUDE.md

@@ -24,10 +24,21 @@ vendor/bin/phpunit test/Feature/Type/EntityTest.php
 # Run a specific test method
 vendor/bin/phpunit --filter testMethodName
 
-# Generate code coverage report (requires Xdebug)
-composer coverage
+# Code coverage (requires Xdebug): a text summary, or an HTML report in coverage-report/
+composer coverage-cli
+composer coverage-html
 ```
 
+### Code Coverage
+
+The project keeps 100% line, method and class coverage. After changing `src/`, check it before reporting the work as done, and list any uncovered statements:
+
+```bash
+XDEBUG_MODE=coverage vendor/bin/phpunit --coverage-text --only-summary-for-coverage-text --coverage-clover "$TMPDIR/clover.xml"
+```
+
+An uncovered line in the clover report is a `<line type="stmt" count="0">`. For each one, write a test if the branch is reachable, or remove the code if it is not, such as a check only static analysis needs (use a `@var` annotation instead). A race, such as a row deleted by another request between two queries, can be tested with `QueryCountingTestCase::beforeExecute()`, which runs a function just before each statement executes. Do not add `@codeCoverageIgnore` to new code to reach 100%.
+
 ### Code Quality
 ```bash
 # Run PHP CodeSniffer (Doctrine Coding Standard)

src/Filter/QueryBuilder.php

@@ -553,13 +553,11 @@ private function toDatabaseValue(
     private static function isDbalType(string $type): bool
     {
         if (self::$dbalTypes === null) {
-            self::$dbalTypes = [];
-
-            foreach ((new ReflectionClass(Types::class))->getConstants() as $name) {
-                if (! is_string($name)) {
-                    continue;
-                }
+            /** @var array<string, string> $names Each constant of Types is a type's name */
+            $names = (new ReflectionClass(Types::class))->getConstants();
 
+            self::$dbalTypes = [];
+            foreach ($names as $name) {
                 self::$dbalTypes[$name] = true;
             }
         }

src/Resolve/ResolveCollectionFactory.php

@@ -550,14 +550,14 @@ private function getBatchableIdentifier(string $sourceClassName, string $associa
 
         $idField = $sourceMetadata->getSingleIdentifierFieldName();
 
-        /** @psalm-suppress MixedAssignment An identifier may be of any type */
-        $identifier = $sourceMetadata->getIdentifierValues($source)[$idField] ?? null;
-        if ($identifier === null) {
-            return null;
-        }
-
+        // A type converts null, the identifier of an entity which has none, to null
         /** @psalm-suppress MixedAssignment A database value may be of any type */
-        $identifier = self::databaseValueOf($this->entityManager, $sourceMetadata, $idField, $identifier);
+        $identifier = self::databaseValueOf(
+            $this->entityManager,
+            $sourceMetadata,
+            $idField,
+            $sourceMetadata->getIdentifierValues($source)[$idField] ?? null,
+        );
 
         return is_int($identifier) || is_string($identifier) ? $identifier : null;
     }
@@ -622,14 +622,13 @@ private function restrictToSource(
         /** @psalm-suppress MixedAssignment The source is an entity */
         $parameter = $source;
         $idFields  = $sourceMetadata->getIdentifierFieldNames();
-        if (count($idFields) === 1 && $sourceMetadata->hasField($idFields[0]) && is_object($source)) {
+
+        // An entity without an identifier is bound as it is, for Doctrine to report
+        /** @psalm-suppress MixedAssignment An identifier may be of any type */
+        $identifier = is_object($source) ? $sourceMetadata->getIdentifierValues($source)[$idFields[0]] ?? null : null;
+        if (count($idFields) === 1 && $sourceMetadata->hasField($idFields[0]) && $identifier !== null) {
             /** @psalm-suppress MixedAssignment A database value may be of any type */
-            $parameter = self::databaseValueOf(
-                $this->entityManager,
-                $sourceMetadata,
-                $idFields[0],
-                $sourceMetadata->getIdentifierValues($source)[$idFields[0]],
-            );
+            $parameter = self::databaseValueOf($this->entityManager, $sourceMetadata, $idFields[0], $identifier);
         }
 
         $queryBuilder->setParameter('source', $parameter);

test/Feature/Resolve/CollectionBatchTest.php

@@ -20,6 +20,7 @@
 use function array_sum;
 use function base64_encode;
 use function sprintf;
+use function str_contains;
 
 /**
  * Batched collections return exactly what per-parent queries return
@@ -257,6 +258,55 @@ public function testOnlyTheTargetsOnAPageAreLoaded(): void
         $this->assertSame(10, array_sum($totals));
     }
 
+    /**
+     * A target removed after its identifier was selected, as by another
+     * request, is left off its page rather than given as a null node
+     */
+    public function testTargetRemovedBeforeItIsLoadedIsLeftOff(): void
+    {
+        $query = '{ artist { edges { node { id performances { edges { node { id } } } } } } }';
+
+        [$before] = $this->execute(['batchAssociations' => true], $query);
+        $removed  = $before['data']['artist']['edges'][0]['node']['performances']['edges'][0]['node']['id'];
+
+        $connection = $this->getEntityManager()->getConnection();
+        $table      = $this->getEntityManager()->getClassMetadata(Performance::class)->getTableName();
+
+        self::beforeExecute(static function (string $sql) use ($connection, $table, $removed): void {
+            // The query loading the targets on the pages, not the one selecting
+            // their identifiers as scalars
+            if (! str_contains($sql, 'FROM ' . $table . ' ') || ! str_contains($sql, ' IN (') || str_contains($sql, 'sclr')) {
+                return;
+            }
+
+            self::beforeExecute(null);
+            $connection->executeStatement('DELETE FROM ' . $table . ' WHERE id = ?', [$removed]);
+        });
+
+        [$after] = $this->execute(['batchAssociations' => true], $query);
+
+        $this->assertArrayNotHasKey('errors', $after);
+
+        $expected = [];
+        $actual   = [];
+        foreach ($before['data']['artist']['edges'] as $index => $edge) {
+            foreach ($edge['node']['performances']['edges'] as $performance) {
+                if ($performance['node']['id'] === $removed) {
+                    continue;
+                }
+
+                $expected[$index][] = $performance['node']['id'];
+            }
+
+            foreach ($after['data']['artist']['edges'][$index]['node']['performances']['edges'] as $performance) {
+                $this->assertNotNull($performance['node']);
+                $actual[$index][] = $performance['node']['id'];
+            }
+        }
+
+        $this->assertSame($expected, $actual);
+    }
+
     /**
      * Above the batch limit, the rows are counted with one query only when
      * a page needs the count

test/Feature/Resolve/DerivedIdentityTest.php

@@ -16,6 +16,8 @@
 use GraphQL\Type\Schema;
 use PHPUnit\Framework\Attributes\DataProvider;
 
+use function array_column;
+
 /**
  * An entity of a derived identity has an association as its identifier.  A
  * collection of, or from, such entities is not batched.
@@ -99,6 +101,39 @@ public function testQuery(bool $batchAssociations): void
      * identifier of the entity its identifier refers to, whose value is not
      * checked to be a number
      */
+
+    /**
+     * The members are not loaded by another field, so the to-one loader
+     * loads them by their identifiers, which are associations
+     */
+    #[DataProvider('batchProvider')]
+    public function testToOneAssociationToADerivedIdentity(bool $batchAssociations): void
+    {
+        $driver = new Driver($this->getEntityManager(), new Config([
+            'group' => 'DerivedIdentity',
+            'batchAssociations' => $batchAssociations,
+        ]));
+        $schema = new Schema([
+            'query' => new ObjectType([
+                'name' => 'query',
+                'fields' => ['badges' => $driver->completeConnection(TestDerivedBadge::class)],
+            ]),
+        ]);
+
+        $result = GraphQL::executeQuery($schema, '{ badges { edges { node { name member { role account { name } } } } } }')
+            ->toArray();
+
+        $this->assertArrayNotHasKey('errors', $result);
+        $this->assertSame(
+            [
+                ['name' => 'ann badge', 'member' => ['role' => 'player', 'account' => ['name' => 'ann']]],
+                ['name' => 'bob badge', 'member' => ['role' => 'player', 'account' => ['name' => 'bob']]],
+                ['name' => 'cat badge', 'member' => ['role' => 'player', 'account' => ['name' => 'cat']]],
+            ],
+            array_column($result['data']['badges']['edges'], 'node'),
+        );
+    }
+
     public function testFilterByAnAssociationToADerivedIdentity(): void
     {
         $account = $this->getEntityManager()->getRepository(TestDerivedAccount::class)->findOneBy(['name' => 'bob']);

test/QueryCountingTestCase.php

@@ -4,6 +4,7 @@
 
 namespace ApiSkeletonsTest\Doctrine\ORM\GraphQL;
 
+use Closure;
 use Doctrine\DBAL\Configuration;
 use Doctrine\DBAL\DriverManager;
 use Doctrine\DBAL\Logging\Middleware;
@@ -23,9 +24,13 @@ abstract class QueryCountingTestCase extends TestCase
     /** @var string[] */
     private static array $sql = [];
 
+    /** Called with the SQL of each statement just before it is executed */
+    private static Closure|null $beforeExecute = null;
+
     public function setUp(): void
     {
         self::registerTypes();
+        self::$beforeExecute = null;
 
         $config = ORMSetup::createAttributeMetadataConfiguration(
             paths: [__DIR__ . '/Entity'],
@@ -68,6 +73,23 @@ public function log(mixed $level, mixed $message, array $context = []): void
     public static function record(string $sql): void
     {
         self::$sql[] = $sql;
+
+        if (self::$beforeExecute === null) {
+            return;
+        }
+
+        (self::$beforeExecute)($sql);
+    }
+
+    /**
+     * Call a function with the SQL of each statement just before it is
+     * executed, such as to change the data as another request would
+     *
+     * @param Closure(string): void|null $callback
+     */
+    protected static function beforeExecute(Closure|null $callback): void
+    {
+        self::$beforeExecute = $callback;
     }
 
     protected function resetQueries(): void