diff --git a/CLAUDE.md b/CLAUDE.md index 02f4cc7..6248a48 100644 --- a/CLAUDE.md +++ b/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 ``. 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) diff --git a/src/Filter/QueryBuilder.php b/src/Filter/QueryBuilder.php index 1267a9d..dcd8423 100644 --- a/src/Filter/QueryBuilder.php +++ b/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 $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; } } diff --git a/src/Resolve/ResolveCollectionFactory.php b/src/Resolve/ResolveCollectionFactory.php index 9732e1a..dc29d17 100644 --- a/src/Resolve/ResolveCollectionFactory.php +++ b/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); diff --git a/test/Feature/Resolve/CollectionBatchTest.php b/test/Feature/Resolve/CollectionBatchTest.php index 69d9675..48afb6a 100644 --- a/test/Feature/Resolve/CollectionBatchTest.php +++ b/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 diff --git a/test/Feature/Resolve/DerivedIdentityTest.php b/test/Feature/Resolve/DerivedIdentityTest.php index 01fd8e3..7aa72d1 100644 --- a/test/Feature/Resolve/DerivedIdentityTest.php +++ b/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']); diff --git a/test/QueryCountingTestCase.php b/test/QueryCountingTestCase.php index 5017e37..1263845 100644 --- a/test/QueryCountingTestCase.php +++ b/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