Apply suggestions from code review

tanlisu · jmikola · web-flow · commit 8bfce9b121c5 · 2021-07-19T13:52:01.000-04:00
Co-authored-by: Jeremy Mikola &lt;jmikola@gmail.com&gt;
diff --git a/src/Collection.php b/src/Collection.php
@@ -1020,7 +1020,7 @@ public function mapReduce(JavascriptInterface $map, JavascriptInterface $reduce,
     public function rename(string $toCollectionName, ?string $toDatabaseName = null, array $options = [])
     {
         if (! isset($toDatabaseName)) {
-            $toDatabaseName = $this->getDatabaseName();
+            $toDatabaseName = $this->databaseName;
         }
 
         if (! isset($options['typeMap'])) {
@@ -1033,7 +1033,7 @@ public function rename(string $toCollectionName, ?string $toDatabaseName = null,
             $options['writeConcern'] = $this->writeConcern;
         }
 
-        $operation = new RenameCollection($this->getDatabaseName(), $this->getCollectionName(), $toDatabaseName, $toCollectionName, $options);
+        $operation = new RenameCollection($this->databaseName, $this->collectionName, $toDatabaseName, $toCollectionName, $options);
 
         return $operation->execute($server);
     }
diff --git a/src/Database.php b/src/Database.php
@@ -500,7 +500,7 @@ public function renameCollection(string $fromCollectionName, string $toCollectio
             $options['writeConcern'] = $this->writeConcern;
         }
 
-        $operation = new RenameCollection($this->getDatabaseName(), $fromCollectionName, $toDatabaseName, $toCollectionName, $options);
+        $operation = new RenameCollection($this->databaseName, $fromCollectionName, $toDatabaseName, $toCollectionName, $options);
 
         return $operation->execute($server);
     }
diff --git a/src/Operation/RenameCollection.php b/src/Operation/RenameCollection.php
@@ -125,18 +125,16 @@ public function execute(Server $server)
             throw UnsupportedException::writeConcernNotSupportedInTransaction();
         }
 
-        $commandOptions = [
+        $cmd = [
             'renameCollection' => $this->fromNamespace,
             'to' => $this->toNamespace,
         ];
 
         if (isset($this->options['dropTarget'])) {
-            $commandOptions['dropTarget'] = $this->options['dropTarget'];
+            $cmd['dropTarget'] = $this->options['dropTarget'];
         }
 
-        $command = new Command($commandOptions);
-
-        $cursor = $server->executeWriteCommand('admin', $command, $this->createOptions());
+        $cursor = $server->executeWriteCommand('admin', new Command($cmd), $this->createOptions());
 
         if (isset($this->options['typeMap'])) {
             $cursor->setTypeMap($this->options['typeMap']);
diff --git a/tests/Collection/CollectionFunctionalTest.php b/tests/Collection/CollectionFunctionalTest.php
@@ -346,15 +346,14 @@ public function testRenameToSameDatabase(): void
         $this->assertCollectionDoesNotExist($this->getCollectionName());
         $this->assertCollectionExists($toCollectionName);
 
-        $document = $toCollection->findOne();
-        $this->assertSameDocument(['_id' => 1], $document);
+        $this->assertSameDocument(['_id' => 1], $toCollection->findOne());
         $toCollection->drop();
     }
 
     public function testRenameToDifferentDatabase(): void
     {
         if ($this->isShardedCluster()) {
-            $this->markTestSkipped('Test does not apply on sharded clusters: need source and target databases to be on the same primary shard.');
+            $this->markTestSkipped('TODO: mongos requires the target database to exist');
         }
 
         $toDatabaseName = $this->getDatabaseName() . '_renamed';
@@ -370,10 +369,8 @@ public function testRenameToDifferentDatabase(): void
         $this->assertCollectionDoesNotExist($this->getCollectionName());
         $this->assertCollectionExists($toCollectionName, $toDatabaseName);
 
-        $document = $toCollection->findOne();
-        $this->assertSameDocument(['_id' => 1], $document);
+        $this->assertSameDocument(['_id' => 1], $toCollection->findOne());
 
-        $toCollection->drop();
         $toDatabase->drop();
     }
 
diff --git a/tests/Database/DatabaseFunctionalTest.php b/tests/Database/DatabaseFunctionalTest.php
@@ -233,8 +233,7 @@ public function testRenameCollectionToSameDatabase(): void
         $this->assertCollectionDoesNotExist($this->getCollectionName());
         $this->assertCollectionExists($toCollectionName);
 
-        $document = $toCollection->findOne();
-        $this->assertSameDocument(['_id' => 1], $document);
+        $this->assertSameDocument(['_id' => 1], $toCollection->findOne());
         $toCollection->drop();
     }
 
@@ -264,10 +263,8 @@ public function testRenameCollectionToDifferentDatabase(): void
         $this->assertCollectionDoesNotExist($this->getCollectionName());
         $this->assertCollectionExists($toCollectionName, $toDatabaseName);
 
-        $document = $toCollection->findOne();
-        $this->assertSameDocument(['_id' => 1], $document);
+        $this->assertSameDocument(['_id' => 1], $toCollection->findOne());
 
-        $toCollection->drop();
         $toDatabase->drop();
     }
 
diff --git a/tests/Operation/RenameCollectionFunctionalTest.php b/tests/Operation/RenameCollectionFunctionalTest.php
@@ -69,7 +69,7 @@ function (array $event): void {
         );
     }
 
-    public function testRenameExistingCollection(): void
+    public function testRenameCollectionToNonexistentTarget(): void
     {
         $server = $this->getPrimaryServer();
 
@@ -90,11 +90,10 @@ public function testRenameExistingCollection(): void
         $this->assertCollectionExists($this->toCollectionName);
 
         $operation = new FindOne($this->getDatabaseName(), $this->toCollectionName, []);
-        $cursor = $operation->execute($server);
-        $this->assertSameDocument(['_id' => 1], $cursor);
+        $this->assertSameDocument(['_id' => 1], $operation->execute($server));
     }
 
-    public function testRenameExistingCollectionExistingTarget(): void
+    public function testRenameCollectionExistingTarget(): void
     {
         $server = $this->getPrimaryServer();
 
@@ -114,7 +113,7 @@ public function testRenameExistingCollectionExistingTarget(): void
             $this->getDatabaseName(),
             $this->toCollectionName
         );
-        $commandResult = $operation->execute($server);
+        $operation->execute($server);
     }
 
     /**
@@ -133,7 +132,7 @@ public function testRenameNonexistentCollection(): void
             $this->toCollectionName,
             ['writeConcern' => $this->createDefaultWriteConcern()]
         );
-        $commandResult = $operation->execute($this->getPrimaryServer());
+        $operation->execute($this->getPrimaryServer());
     }
 
     public function testSessionOption(): void

Original file line number	Diff line number	Diff line change
`@@ -1020,7 +1020,7 @@ public function mapReduce(JavascriptInterface $map, JavascriptInterface $reduce,`
`1020`	`1020`	`public function rename(string $toCollectionName, ?string $toDatabaseName = null, array $options = [])`
`1021`	`1021`	`{`
`1022`	`1022`	`if (! isset($toDatabaseName)) {`
`1023`		`- $toDatabaseName = $this->getDatabaseName();`
	`1023`	`+ $toDatabaseName = $this->databaseName;`
`1024`	`1024`	`}`
`1025`	`1025`
`1026`	`1026`	`if (! isset($options['typeMap'])) {`
`@@ -1033,7 +1033,7 @@ public function rename(string $toCollectionName, ?string $toDatabaseName = null,`
`1033`	`1033`	`$options['writeConcern'] = $this->writeConcern;`
`1034`	`1034`	`}`
`1035`	`1035`
`1036`		`- $operation = new RenameCollection($this->getDatabaseName(), $this->getCollectionName(), $toDatabaseName, $toCollectionName, $options);`
	`1036`	`+ $operation = new RenameCollection($this->databaseName, $this->collectionName, $toDatabaseName, $toCollectionName, $options);`
`1037`	`1037`
`1038`	`1038`	`return $operation->execute($server);`
`1039`	`1039`	`}`
Original file line number	Diff line number	Diff line change
`@@ -500,7 +500,7 @@ public function renameCollection(string $fromCollectionName, string $toCollectio`
`500`	`500`	`$options['writeConcern'] = $this->writeConcern;`
`501`	`501`	`}`
`502`	`502`
`503`		`- $operation = new RenameCollection($this->getDatabaseName(), $fromCollectionName, $toDatabaseName, $toCollectionName, $options);`
	`503`	`+ $operation = new RenameCollection($this->databaseName, $fromCollectionName, $toDatabaseName, $toCollectionName, $options);`
`504`	`504`
`505`	`505`	`return $operation->execute($server);`
`506`	`506`	`}`
Original file line number	Diff line number	Diff line change
`@@ -346,15 +346,14 @@ public function testRenameToSameDatabase(): void`
`346`	`346`	`$this->assertCollectionDoesNotExist($this->getCollectionName());`
`347`	`347`	`$this->assertCollectionExists($toCollectionName);`
`348`	`348`
`349`		`- $document = $toCollection->findOne();`
`350`		`- $this->assertSameDocument(['_id' => 1], $document);`
	`349`	`+ $this->assertSameDocument(['_id' => 1], $toCollection->findOne());`
`351`	`350`	`$toCollection->drop();`
`352`	`351`	`}`
`353`	`352`
`354`	`353`	`public function testRenameToDifferentDatabase(): void`
`355`	`354`	`{`
`356`	`355`	`if ($this->isShardedCluster()) {`
`357`		`- $this->markTestSkipped('Test does not apply on sharded clusters: need source and target databases to be on the same primary shard.');`
	`356`	`+ $this->markTestSkipped('TODO: mongos requires the target database to exist');`
`358`	`357`	`}`
`359`	`358`
`360`	`359`	`$toDatabaseName = $this->getDatabaseName() . '_renamed';`
`@@ -370,10 +369,8 @@ public function testRenameToDifferentDatabase(): void`
`370`	`369`	`$this->assertCollectionDoesNotExist($this->getCollectionName());`
`371`	`370`	`$this->assertCollectionExists($toCollectionName, $toDatabaseName);`
`372`	`371`
`373`		`- $document = $toCollection->findOne();`
`374`		`- $this->assertSameDocument(['_id' => 1], $document);`
	`372`	`+ $this->assertSameDocument(['_id' => 1], $toCollection->findOne());`
`375`	`373`
`376`		`- $toCollection->drop();`
`377`	`374`	`$toDatabase->drop();`
`378`	`375`	`}`
`379`	`376`
Original file line number	Diff line number	Diff line change
`@@ -69,7 +69,7 @@ function (array $event): void {`
`69`	`69`	`);`
`70`	`70`	`}`
`71`	`71`
`72`		`- public function testRenameExistingCollection(): void`
	`72`	`+ public function testRenameCollectionToNonexistentTarget(): void`
`73`	`73`	`{`
`74`	`74`	`$server = $this->getPrimaryServer();`
`75`	`75`
`@@ -90,11 +90,10 @@ public function testRenameExistingCollection(): void`
`90`	`90`	`$this->assertCollectionExists($this->toCollectionName);`
`91`	`91`
`92`	`92`	`$operation = new FindOne($this->getDatabaseName(), $this->toCollectionName, []);`
`93`		`- $cursor = $operation->execute($server);`
`94`		`- $this->assertSameDocument(['_id' => 1], $cursor);`
	`93`	`+ $this->assertSameDocument(['_id' => 1], $operation->execute($server));`
`95`	`94`	`}`
`96`	`95`
`97`		`- public function testRenameExistingCollectionExistingTarget(): void`
	`96`	`+ public function testRenameCollectionExistingTarget(): void`
`98`	`97`	`{`
`99`	`98`	`$server = $this->getPrimaryServer();`
`100`	`99`
`@@ -114,7 +113,7 @@ public function testRenameExistingCollectionExistingTarget(): void`
`114`	`113`	`$this->getDatabaseName(),`
`115`	`114`	`$this->toCollectionName`
`116`	`115`	`);`
`117`		`- $commandResult = $operation->execute($server);`
	`116`	`+ $operation->execute($server);`
`118`	`117`	`}`
`119`	`118`
`120`	`119`	`/**`
`@@ -133,7 +132,7 @@ public function testRenameNonexistentCollection(): void`
`133`	`132`	`$this->toCollectionName,`
`134`	`133`	`['writeConcern' => $this->createDefaultWriteConcern()]`
`135`	`134`	`);`
`136`		`- $commandResult = $operation->execute($this->getPrimaryServer());`
	`135`	`+ $operation->execute($this->getPrimaryServer());`
`137`	`136`	`}`
`138`	`137`
`139`	`138`	`public function testSessionOption(): void`