Skip to content

Fix TypeError in SummaryCounters constructor when systemUpdates is false (fixes #265) #266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix TypeError in SummaryCounters constructor when systemUpdates is false (fixes #265) #266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 26 additions & 10 deletions src/Formatter/SummarizedResultFormatter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@
* constraints-added?: int,
* constraints-removed?: int,
* contains-updates?: bool,
* contains-system-updates?: bool,
* system-updates?: int,
* contains-system-updates?: bool|int,
* system-updates?: int|bool,
* db?: string
* }
* @psalm-type CypherError = array{code: string, message: string}
Expand Down Expand Up @@ -138,6 +138,21 @@ public function formatBoltStats(array $response): SummaryCounters
}
}

$systemUpdates = $stats['system-updates'] ?? 0;
if (is_bool($systemUpdates)) {
$systemUpdates = (int) $systemUpdates;
}

$containsSystemUpdates = $stats['contains-system-updates'] ?? null;

if ($containsSystemUpdates === null) {
$containsSystemUpdates = $systemUpdates > 0;
} else {
if (!is_bool($containsSystemUpdates)) {
$containsSystemUpdates = (bool) $containsSystemUpdates;
}
}

return new SummaryCounters(
$stats['nodes-created'] ?? 0,
$stats['nodes-deleted'] ?? 0,
Expand All @@ -151,8 +166,8 @@ public function formatBoltStats(array $response): SummaryCounters
$stats['constraints-added'] ?? 0,
$stats['constraints-removed'] ?? 0,
$updateCount > 0,
($stats['contains-system-updates'] ?? $stats['system-updates'] ?? 0) >= 1,
$stats['system-updates'] ?? 0
$containsSystemUpdates,
$systemUpdates
);
}

Expand Down Expand Up @@ -195,10 +210,11 @@ function (mixed $response) use ($connection, $statement, $runStart, $resultAvail

$formattedResult = $this->processBoltResult($meta, $result, $connection, $holder);

/**
* @var SummarizedResult<CypherMap<OGMTypes>>
*/
return new SummarizedResult($summary, (new CypherList($formattedResult))->withCacheLimit($result->getFetchSize()));
/** @var SummarizedResult */
$result = (new CypherList($formattedResult))->withCacheLimit($result->getFetchSize());
// $keys = $meta['fields'];

return new SummarizedResult($summary, $result);
}

public function formatArgs(array $profiledPlanData): PlanArguments
Expand Down Expand Up @@ -255,7 +271,7 @@ private function formatProfiledPlan(array $profiledPlanData): ProfiledQueryPlan
pageCacheHitRatio: (float) ($profiledPlanData['pageCacheHitRatio'] ?? 0.0),
time: (int) ($profiledPlanData['time'] ?? 0),
operatorType: $profiledPlanData['operatorType'] ?? '',
children: array_map([$this, 'formatProfiledPlan'], $profiledPlanData['children'] ?? []),
children: array_values(array_map([$this, 'formatProfiledPlan'], $profiledPlanData['children'] ?? [])),
identifiers: $profiledPlanData['identifiers'] ?? []
);
}
Expand Down Expand Up @@ -309,7 +325,7 @@ private function formatPlan(array $plan): Plan
{
return new Plan(
$this->formatArgs($plan['args']),
array_map($this->formatPlan(...), $plan['children'] ?? []),
array_values(array_map($this->formatPlan(...), $plan['children'] ?? [])),
$plan['identifiers'] ?? [],
$plan['operatorType'] ?? ''
);
Expand Down
96 changes: 96 additions & 0 deletions tests/Integration/SummarizedResultFormatterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
use Laudis\Neo4j\Contracts\TransactionInterface;
use Laudis\Neo4j\Databags\SummarizedResult;
use Laudis\Neo4j\Databags\SummaryCounters;
use Laudis\Neo4j\Formatter\Specialised\BoltOGMTranslator;
use Laudis\Neo4j\Formatter\SummarizedResultFormatter;
use Laudis\Neo4j\Tests\EnvironmentAwareIntegrationTest;
use Laudis\Neo4j\Types\CartesianPoint;
use Laudis\Neo4j\Types\CypherList;
Expand Down Expand Up @@ -521,4 +523,98 @@ private function articlesQuery(): string
article.readingTime = duration(articleProperties.readingTime)
CYPHER;
}

public function testFormatBoltStatsWithFalseSystemUpdates(): void
{
$formatter = new SummarizedResultFormatter(new BoltOGMTranslator());

$response = [
'stats' => [
'nodes-created' => 1,
'nodes-deleted' => 0,
'relationships-created' => 0,
'relationships-deleted' => 0,
'properties-set' => 2,
'labels-added' => 1,
'labels-removed' => 0,
'indexes-added' => 0,
'indexes-removed' => 0,
'constraints-added' => 0,
'constraints-removed' => 0,
'contains-updates' => true,
'contains-system-updates' => false,
'system-updates' => false,
],
];

$counters = $formatter->formatBoltStats($response);

self::assertInstanceOf(SummaryCounters::class, $counters);
self::assertEquals(1, $counters->nodesCreated());
self::assertEquals(2, $counters->propertiesSet());
self::assertSame(0, $counters->systemUpdates());
}

public function testSystemUpdatesWithPotentialFalseValues(): void
{
$this->getSession()->run('CREATE INDEX duplicate_test_index IF NOT EXISTS FOR (n:TestSystemUpdates) ON (n.duplicateProperty)');
$result = $this->getSession()->run('CREATE INDEX duplicate_test_index IF NOT EXISTS FOR (n:TestSystemUpdates) ON (n.duplicateProperty)');

$summary = $result->getSummary();
$counters = $summary->getCounters();

// For duplicate index creation (IF NOT EXISTS), might not create system updates
$this->assertGreaterThanOrEqual(0, $counters->systemUpdates());
// containsSystemUpdates should be consistent with systemUpdates count
$this->assertEquals($counters->systemUpdates() > 0, $counters->containsSystemUpdates());

$result2 = $this->getSession()->run('DROP INDEX non_existent_test_index IF EXISTS');

$summary2 = $result2->getSummary();
$counters2 = $summary2->getCounters();

// Dropping non-existent index should not create system updates
$this->assertEquals(0, $counters2->systemUpdates());
$this->assertFalse($counters2->containsSystemUpdates());

$this->getSession()->run('DROP INDEX duplicate_test_index IF EXISTS');
}

public function testMultipleSystemOperationsForBug(): void
{
$operations = [
'CREATE INDEX multi_test_1 IF NOT EXISTS FOR (n:MultiTestNode) ON (n.prop1)',
'CREATE INDEX multi_test_2 IF NOT EXISTS FOR (n:MultiTestNode) ON (n.prop2)',
'CREATE CONSTRAINT multi_test_constraint IF NOT EXISTS FOR (n:MultiTestNode) REQUIRE n.id IS UNIQUE',
'DROP INDEX multi_test_1 IF EXISTS',
'DROP INDEX multi_test_2 IF EXISTS',
'DROP CONSTRAINT multi_test_constraint IF EXISTS',
];

foreach ($operations as $operation) {
$result = $this->getSession()->run($operation);

$summary = $result->getSummary();
$counters = $summary->getCounters();

// Test that system operations properly track system updates
$this->assertGreaterThanOrEqual(0, $counters->systemUpdates());
// Verify consistency between systemUpdates count and containsSystemUpdates flag
$this->assertEquals($counters->systemUpdates() > 0, $counters->containsSystemUpdates());
}
}

public function testRegularDataOperationsStillWork(): void
{
$result = $this->getSession()->run('CREATE (n:RegularTestNode {name: "test", id: $id}) RETURN n', ['id' => bin2hex(random_bytes(8))]);

$summary = $result->getSummary();
$counters = $summary->getCounters();

// Regular data operations should not involve system updates
$this->assertEquals(0, $counters->systemUpdates());
$this->assertFalse($counters->containsSystemUpdates());

$this->getSession()->run('MATCH (n:RegularTestNode) DELETE n');
}
}