Skip to content

Commit 58df782

Browse files
alcaeusalcaeus
committed
Handle issues from code review
1 parent 1fa67c8 commit 58df782

File tree

4 files changed

+106
-60
lines changed

4 files changed

+106
-60
lines changed

‎tests/UnifiedSpecTests/EventObserver.php

Lines changed: 25 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -97,59 +97,23 @@ public function __construct(array $observeEvents, array $ignoreCommands, string
9797
*/
9898
public function commandFailed(CommandFailedEvent $event)
9999
{
100-
if (! $this->context->isActiveClient($this->clientId)) {
101-
return;
102-
}
103-
104-
if (! isset($this->observeEvents[CommandFailedEvent::class])) {
105-
return;
106-
}
107-
108-
if (isset($this->ignoreCommands[$event->getCommandName()])) {
109-
return;
110-
}
111-
112-
$this->actualEvents[] = $event;
100+
$this->handleEvent($event);
113101
}
114102

115103
/**
116104
* @see https://www.php.net/manual/en/mongodb-driver-monitoring-commandsubscriber.commandstarted.php
117105
*/
118106
public function commandStarted(CommandStartedEvent $event)
119107
{
120-
if (! $this->context->isActiveClient($this->clientId)) {
121-
return;
122-
}
123-
124-
if (! isset($this->observeEvents[CommandStartedEvent::class])) {
125-
return;
126-
}
127-
128-
if (isset($this->ignoreCommands[$event->getCommandName()])) {
129-
return;
130-
}
131-
132-
$this->actualEvents[] = $event;
108+
$this->handleEvent($event);
133109
}
134110

135111
/**
136112
* @see https://www.php.net/manual/en/mongodb-driver-monitoring-commandsubscriber.commandsucceeded.php
137113
*/
138114
public function commandSucceeded(CommandSucceededEvent $event)
139115
{
140-
if (! $this->context->isActiveClient($this->clientId)) {
141-
return;
142-
}
143-
144-
if (! isset($this->observeEvents[CommandSucceededEvent::class])) {
145-
return;
146-
}
147-
148-
if (isset($this->ignoreCommands[$event->getCommandName()])) {
149-
return;
150-
}
151-
152-
$this->actualEvents[] = $event;
116+
$this->handleEvent($event);
153117
}
154118

155119
public function start()
@@ -193,9 +157,6 @@ public function assert(array $expectedEvents)
193157

194158
foreach ($mi as $keys => $events) {
195159
list($expectedEvent, $actualEvent) = $events;
196-
// TODO: assertNotNull may be redundant since counts are equal
197-
assertNotNull($expectedEvent);
198-
assertNotNull($actualEvent);
199160

200161
assertInternalType('object', $expectedEvent);
201162
$expectedEvent = (array) $expectedEvent;
@@ -276,4 +237,26 @@ private function assertCommandFailedEvent(CommandFailedEvent $actual, stdClass $
276237
assertSame($actual->getCommandName(), $expected->commandName, $message . ': commandName matches');
277238
}
278239
}
240+
241+
/** @param CommandStartedEvent|CommandSucceededEvent|CommandFailedEvent $event */
242+
private function handleEvent($event)
243+
{
244+
if (! $this->context->isActiveClient($this->clientId)) {
245+
return;
246+
}
247+
248+
if (! is_object($event)) {
249+
return;
250+
}
251+
252+
if (! isset($this->observeEvents[get_class($event)])) {
253+
return;
254+
}
255+
256+
if (isset($this->ignoreCommands[$event->getCommandName()])) {
257+
return;
258+
}
259+
260+
$this->actualEvents[] = $event;
261+
}
279262
}

‎tests/UnifiedSpecTests/ExpectedError.php

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,16 +41,16 @@ final class ExpectedError
4141
/** @var bool */
4242
private $isError = false;
4343

44-
/** @var bool */
44+
/** @var bool|null */
4545
private $isClientError;
4646

47-
/** @var string */
47+
/** @var string|null */
4848
private $messageContains;
4949

50-
/** @var int */
50+
/** @var int|null */
5151
private $code;
5252

53-
/** @var string */
53+
/** @var string|null */
5454
private $codeName;
5555

5656
/** @var array */
@@ -59,7 +59,7 @@ final class ExpectedError
5959
/** @var array */
6060
private $excludedLabels = [];
6161

62-
/** @var ExpectedResult */
62+
/** @var ExpectedResult|null */
6363
private $expectedResult;
6464

6565
public function __construct(stdClass $o = null, EntityMap $entityMap)

‎tests/UnifiedSpecTests/Operation.php

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,11 @@ private function executeForCollection(Collection $collection)
308308
array_diff_key($args, ['filter' => 1])
309309
);
310310
case 'distinct':
311+
if (isset($args['session']) && $args['session']->isInTransaction()) {
312+
// Transaction, but sharded cluster?
313+
$collection->distinct('foo');
314+
}
315+
311316
return $collection->distinct(
312317
$args['fieldName'],
313318
$args['filter'] ?? [],
@@ -498,20 +503,29 @@ private function executeForTestRunner()
498503
$args['client'] = $this->entityMap->getClient($args['client']);
499504
}
500505

501-
// TODO: validate arguments
502506
switch ($this->name) {
503507
case 'assertCollectionExists':
508+
assertInternalType('string', $args['databaseName']);
509+
assertInternalType('string', $args['collectionName']);
504510
$database = $this->context->getInternalClient()->selectDatabase($args['databaseName']);
505511
assertContains($args['collectionName'], $database->listCollectionNames());
506512
break;
507513
case 'assertCollectionNotExists':
514+
assertInternalType('string', $args['databaseName']);
515+
assertInternalType('string', $args['collectionName']);
508516
$database = $this->context->getInternalClient()->selectDatabase($args['databaseName']);
509517
assertNotContains($args['collectionName'], $database->listCollectionNames());
510518
break;
511519
case 'assertIndexExists':
520+
assertInternalType('string', $args['databaseName']);
521+
assertInternalType('string', $args['collectionName']);
522+
assertInternalType('string', $args['indexName']);
512523
assertContains($args['indexName'], $this->getIndexNames($args['databaseName'], $args['collectionName']));
513524
break;
514525
case 'assertIndexNotExists':
526+
assertInternalType('string', $args['databaseName']);
527+
assertInternalType('string', $args['collectionName']);
528+
assertInternalType('string', $args['indexName']);
515529
assertNotContains($args['indexName'], $this->getIndexNames($args['databaseName'], $args['collectionName']));
516530
break;
517531
case 'assertSameLsidOnLastTwoCommands':
@@ -529,18 +543,24 @@ private function executeForTestRunner()
529543
assertFalse($this->context->isDirtySession($this->arguments['session']));
530544
break;
531545
case 'assertSessionPinned':
546+
assertInstanceOf(Session::class, $args['session']);
532547
assertInstanceOf(Server::class, $args['session']->getServer());
533548
break;
534549
case 'assertSessionTransactionState':
550+
assertInstanceOf(Session::class, $args['session']);
535551
assertSame($this->arguments['state'], $args['session']->getTransactionState());
536552
break;
537553
case 'assertSessionUnpinned':
554+
assertInstanceOf(Session::class, $args['session']);
538555
assertNull($args['session']->getServer());
539556
break;
540557
case 'failPoint':
558+
assertInternalType('array', $args['failPoint']);
541559
$args['client']->selectDatabase('admin')->command($args['failPoint']);
542560
break;
543561
case 'targetedFailPoint':
562+
assertInstanceOf(Session::class, $args['session']);
563+
assertInternalType('array', $args['failPoint']);
544564
assertNotNull($args['session']->getServer(), 'Session is pinned');
545565
$operation = new DatabaseCommand('admin', $args['failPoint']);
546566
$operation->execute($args['session']->getServer());

‎tests/UnifiedSpecTests/UnifiedSpecTest.php

Lines changed: 55 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
namespace MongoDB\Tests\UnifiedSpecTests;
44

55
use MongoDB\Client;
6+
use MongoDB\Collection;
67
use MongoDB\Driver\Exception\ServerException;
78
use MongoDB\Driver\ReadPreference;
89
use MongoDB\Driver\Server;
@@ -13,6 +14,7 @@
1314
use Throwable;
1415
use function assertTrue;
1516
use function file_get_contents;
17+
use function gc_collect_cycles;
1618
use function glob;
1719
use function MongoDB\BSON\fromJSON;
1820
use function MongoDB\BSON\toPHP;
@@ -30,6 +32,9 @@ class UnifiedSpecTest extends FunctionalTestCase
3032

3133
const SERVER_ERROR_INTERRUPTED = 11601;
3234

35+
const MIN_SCHEMA_VERSION = '1.0';
36+
const MAX_SCHEMA_VERSION = '1.1';
37+
3338
const TOPOLOGY_SINGLE = 'single';
3439
const TOPOLOGY_REPLICASET = 'replicaset';
3540
const TOPOLOGY_SHARDED = 'sharded';
@@ -55,11 +60,10 @@ private function doSetUp()
5560
{
5661
parent::setUp();
5762

58-
/* TODO: The transactions spec advises calling killAllSessions only at
59-
* the start of the test suite and after failed tests; however, the
60-
* "unpin after transient error within a transaction" pinning test
61-
* causes the subsequent transaction test to block. This can be removed
62-
* once that is addressed. */
63+
/* The transactions spec advises calling killAllSessions only at the
64+
* start of the test suite and after failed tests; however, the "unpin
65+
* after transient error within a transaction" pinning test causes the
66+
* subsequent transaction test to block. */
6367
self::killAllSessions();
6468

6569
$this->failPointObserver = new FailPointObserver();
@@ -75,9 +79,10 @@ private function doTearDown()
7579
$this->failPointObserver->stop();
7680
$this->failPointObserver->disableFailPoints();
7781

78-
/* TODO: Consider manually invoking gc_collect_cycles since each test is
79-
* prone to create cycles (perhaps due to EntityMap), which can leak and
80-
* prevent sessions from being released back into the pool. */
82+
/* Manually invoking garbage collection since each test is prone to
83+
* create cycles (perhaps due to EntityMap), which can leak and prevent
84+
* sessions from being released back into the pool. */
85+
gc_collect_cycles();
8186

8287
parent::tearDown();
8388
}
@@ -116,12 +121,11 @@ public function testPassingTests(stdClass $test, string $schemaVersion, array $r
116121
$context->createEntities($createEntities);
117122
}
118123

119-
// TODO handle distinct commands in sharded transactions
124+
$this->assertInternalType('array', $test->operations);
125+
$this->preventStaleDbVersionError($test->operations, $context);
120126

121127
$context->startEventObservers();
122128

123-
$this->assertInternalType('array', $test->operations);
124-
125129
foreach ($test->operations as $o) {
126130
$operation = new Operation($o, $context);
127131
$operation->assert();
@@ -288,7 +292,7 @@ private function getCachedTopology()
288292
*/
289293
private function isSchemaVersionSupported($schemaVersion)
290294
{
291-
return version_compare($schemaVersion, '1.0', '>=') && version_compare($schemaVersion, '1.1', '<');
295+
return version_compare($schemaVersion, self::MIN_SCHEMA_VERSION, '>=') && version_compare($schemaVersion, self::MAX_SCHEMA_VERSION, '<');
292296
}
293297

294298
/**
@@ -343,4 +347,43 @@ private function prepareInitialData(array $initialData)
343347
$collectionData->prepareInitialData(self::$internalClient);
344348
}
345349
}
350+
351+
/**
352+
* Work around potential error executing distinct on sharded clusters.
353+
*
354+
* @see https://github.com/mongodb/specifications/tree/master/source/transactions/tests#why-do-tests-that-run-distinct-sometimes-fail-with-staledbversionts.
355+
*/
356+
private function preventStaleDbVersionError(array $operations, Context $context)
357+
{
358+
if (! $this->isShardedCluster()) {
359+
return;
360+
}
361+
362+
$hasStartTransaction = false;
363+
$hasDistinct = false;
364+
$collection = null;
365+
366+
foreach ($operations as $operation) {
367+
switch ($operation->name) {
368+
case 'distinct':
369+
$hasDistinct = true;
370+
$collection = $context->getEntityMap()[$operation->object];
371+
break;
372+
373+
case 'startTransaction':
374+
$hasStartTransaction = true;
375+
break;
376+
377+
default:
378+
continue;
379+
}
380+
381+
if ($hasStartTransaction && $hasDistinct) {
382+
$this->assertInstanceOf(Collection::class, $collection);
383+
$collection->distinct('foo');
384+
385+
return;
386+
}
387+
}
388+
}
346389
}

0 commit comments

Comments
 (0)