#37180 don't pass collection query parameter to driver, dsn should ta…

…ke precedence over options
symfony · Jun 15, 2020 · 599c128 · 599c128
1 parent 8bb0897
commit 599c128
Show file tree

Hide file tree

Showing 2 changed files with 75 additions and 24 deletions.
diff --git a/src/Symfony/Component/Lock/Store/MongoDbStore.php b/src/Symfony/Component/Lock/Store/MongoDbStore.php
@@ -72,8 +72,8 @@ class MongoDbStore implements BlockingStoreInterface
      *      driverOptions: Array of driver options. [used when $mongo is a URI]
      *
      * When using a URI string:
-     *      the database is determined from the "database" option, otherwise the uri's path is used.
-     *      the collection is determined from the "collection" option, otherwise the uri's "collection" querystring parameter is used.
+     *      The database is determined from the uri's path, otherwise the "database" option is used. To specify an alternate authentication database; "authSource" uriOption or querystring parameter must be used.
+     *      The collection is determined from the uri's "collection" querystring parameter, otherwise the "collection" option is used.
      *
      * For example: mongodb://myuser:mypass@myhost/mydatabase?collection=mycollection
      *
@@ -104,34 +104,20 @@ public function __construct($mongo, array $options = [], float $initialTtl = 300
         if ($mongo instanceof Collection) {
             $this->collection = $mongo;
         } elseif ($mongo instanceof Client) {
-            if (null === $this->options['database']) {
-                throw new InvalidArgumentException(sprintf('"%s()" requires the "database" option when constructing with a "%s".', __METHOD__, Client::class));
-            }
-            if (null === $this->options['collection']) {
-                throw new InvalidArgumentException(sprintf('"%s()" requires the "collection" option when constructing with a "%s".', __METHOD__, Client::class));
-            }
-
             $this->client = $mongo;
         } elseif (\is_string($mongo)) {
-            if (false === $parsedUrl = parse_url($mongo)) {
-                throw new InvalidArgumentException(sprintf('The given MongoDB Connection URI "%s" is invalid.', $mongo));
-            }
-            $query = [];
-            if (isset($parsedUrl['query'])) {
-                parse_str($parsedUrl['query'], $query);
-            }
-            $this->options['collection'] = $this->options['collection'] ?? $query['collection'] ?? null;
-            $this->options['database'] = $this->options['database'] ?? ltrim($parsedUrl['path'] ?? '', '/') ?: null;
+            $this->uri = $this->skimUri($mongo);
+        } else {
+            throw new InvalidArgumentException(sprintf('"%s()" requires "%s" or "%s" or URI as first argument, "%s" given.', __METHOD__, Collection::class, Client::class, get_debug_type($mongo)));
+        }
+
+        if (!($mongo instanceof Collection)) {
             if (null === $this->options['database']) {
-                throw new InvalidArgumentException(sprintf('"%s()" requires the "database" in the URI path or option when constructing with a URI.', __METHOD__));
+                throw new InvalidArgumentException(sprintf('"%s()" requires the "database" in the URI path or option.', __METHOD__));
             }
             if (null === $this->options['collection']) {
-                throw new InvalidArgumentException(sprintf('"%s()" requires the "collection" in the URI querystring or option when constructing with a URI.', __METHOD__));
+                throw new InvalidArgumentException(sprintf('"%s()" requires the "collection" in the URI querystring or option.', __METHOD__));
             }
-
-            $this->uri = $mongo;
-        } else {
-            throw new InvalidArgumentException(sprintf('"%s()" requires "%s" or "%s" or URI as first argument, "%s" given.', __METHOD__, Collection::class, Client::class, get_debug_type($mongo)));
         }
 
         if ($this->options['gcProbablity'] < 0.0 || $this->options['gcProbablity'] > 1.0) {
@@ -143,6 +129,41 @@ public function __construct($mongo, array $options = [], float $initialTtl = 300
         }
     }
 
+    /**
+     * Extract default database and collection from given connection uri and remove collection querystring.
+     */
+    private function skimUri(string $uri): string
+    {
+        if (false === $parsedUrl = parse_url($uri)) {
+            throw new InvalidArgumentException(sprintf('The given MongoDB Connection URI "%s" is invalid.', $mongo));
+        }
+
+        $query = [];
+        if (isset($parsedUrl['query'])) {
+            parse_str($parsedUrl['query'], $query);
+        }
+
+        if (isset($query['collection'])) {
+            $this->options['collection'] = $query['collection'];
+            $queryStringPos = strrpos($uri, $parsedUrl['query']);
+            unset($query['collection']);
+            $prefix = substr($uri, 0, $queryStringPos);
+            $newQuery = http_build_query($query, '', '&', PHP_QUERY_RFC3986);
+            if (empty($newQuery)) {
+                $prefix = rtrim($prefix, '?');
+            }
+            $suffix = substr($uri, $queryStringPos + \strlen($parsedUrl['query']));
+            $uri = $prefix.$newQuery.$suffix;
+        }
+
+        $pathDb = ltrim($parsedUrl['path'] ?? '', '/') ?: null;
+        if (null !== $pathDb) {
+            $this->options['database'] = $pathDb;
+        }
+
+        return $uri;
+    }
+
     /**
      * Creates a TTL index to automatically remove expired locks.
      *

diff --git a/src/Symfony/Component/Lock/Tests/Store/MongoDbStoreTest.php b/src/Symfony/Component/Lock/Tests/Store/MongoDbStoreTest.php
@@ -141,4 +141,34 @@ public function provideInvalidConstructorArgs()
         yield ['mongodb://localhost/test', []];
         yield ['mongodb://localhost/', []];
     }
+
+    public function testCollectionDsnPrecedence()
+    {
+        $client = self::getMongoClient();
+
+        $store = new MongoDbStore('mongodb://localhost/test?collection=lock_dsn', ['collection' => 'lock_option']);
+        $storeReflection = new \ReflectionObject($store);
+
+        $uriProperty = $storeReflection->getProperty('uri');
+        $uriProperty->setAccessible(true);
+        $uri = $uriProperty->getValue($store);
+        $this->assertSame('mongodb://localhost/test', $uri);
+
+        $optionsProperty = $storeReflection->getProperty('options');
+        $optionsProperty->setAccessible(true);
+        $options = $optionsProperty->getValue($store);
+        $this->assertSame('lock_dsn', $options['collection']);
+    }
+
+    public function testDatabaseDsnPrecedence()
+    {
+        $client = self::getMongoClient();
+
+        $store = new MongoDbStore('mongodb://localhost/test_dsn?collection=lock', ['database' => 'test_option']);
+        $storeReflection = new \ReflectionObject($store);
+        $optionsProperty = $storeReflection->getProperty('options');
+        $optionsProperty->setAccessible(true);
+        $options = $optionsProperty->getValue($store);
+        $this->assertSame('test_dsn', $options['database']);
+    }
 }