Skip to content

PHPLIB-516: Append PHPLIB version information to handshake data #731

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.

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

Merged
merged 1 commit into from
Mar 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
"php": "^7.0",
"ext-hash": "*",
"ext-json": "*",
"ext-mongodb": "^1.8"
"ext-mongodb": "^1.8",
"jean85/pretty-package-versions": "^1.2"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This provides a better interface around version strings returned by ocramius/package-versions. It is used to provide the current version of this library without having to maintain it in a version constant.

},
"require-dev": {
"phpunit/phpunit": "^6.4 || ^8.3",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,34 @@ description: |
For the ``keyVaultClient`` option, you may pass a :phpclass:`MongoDB\\Client`
instance, which will be unwrapped to provide a :php:`MongoDB\\Driver\\Manager <class.mongodb-driver-manager>`
to the extension.
.. versionadded:: 1.6
interface: phpmethod
operation: ~
optional: true
---
arg_name: option
name: driver
type: array
description: |
Additional driver metadata to be passed on to the server handshake. This is an
array containing ``name``, ``version``, and ``platform`` fields:

.. code-block:: php

[
'name' => 'my-driver',
'version' => '1.2.3-dev',
'platform' => 'some-platform',
]

.. note::

This feature is primarily designed for custom drivers and ODMs, which may
want to identify themselves to the server for diagnostic purposes.
Applications should use the ``appName`` URI option instead of driver
metadata.

.. versionadded:: 1.7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a note that this is best used for things like wrapping libraries and ODMs rather than applications?

interface: phpmethod
operation: ~
optional: true
Expand Down
54 changes: 54 additions & 0 deletions src/Client.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

namespace MongoDB;

use Jean85\PrettyVersions;
use MongoDB\Driver\ClientEncryption;
use MongoDB\Driver\Exception\InvalidArgumentException as DriverInvalidArgumentException;
use MongoDB\Driver\Exception\RuntimeException as DriverRuntimeException;
Expand All @@ -34,7 +35,9 @@
use MongoDB\Operation\DropDatabase;
use MongoDB\Operation\ListDatabases;
use MongoDB\Operation\Watch;
use Throwable;
use function is_array;
use function is_string;

class Client
{
Expand All @@ -51,6 +54,12 @@ class Client
/** @var integer */
private static $wireVersionForWritableCommandWriteConcern = 5;

/** @var string */
private static $handshakeSeparator = ' / ';

/** @var string|null */
private static $version;

/** @var Manager */
private $manager;

Expand Down Expand Up @@ -108,6 +117,8 @@ public function __construct($uri = 'mongodb://127.0.0.1/', array $uriOptions = [
}
}

$driverOptions['driver'] = $this->mergeDriverInfo($driverOptions['driver'] ?? []);

$this->uri = (string) $uri;
$this->typeMap = $driverOptions['typeMap'] ?? null;

Expand Down Expand Up @@ -354,4 +365,47 @@ public function watch(array $pipeline = [], array $options = [])

return $operation->execute($server);
}

private static function getVersion() : string
{
if (self::$version === null) {
try {
self::$version = PrettyVersions::getVersion('mongodb/mongodb')->getPrettyVersion();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the package-versions package caches package information when they are installed, I thought it would be best to still cache this data to ensure we're not unnecessarily causing delays on connecting.

} catch (Throwable $t) {
return 'unknown';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look through PrettyVersions and it seems that it doesn't throw anything on its own, so I assume this comes from Ocramius' library. If so, I think there's also a case where UnexpectedValueException could be thrown. Would it be safer to just catch any RuntimeException?

Also, I'm totally on board with not exposing the original exception. I don't think "fix your shit" would go over well with users, especially when it might not be clear which library is generating that message 😆

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to catch any throwable. That exception message (along with not wanting to break a connection to MongoDB because we couldn't determine the version) was the main reason for adding a try-catch block.

}
}

return self::$version;
}

private function mergeDriverInfo(array $driver) : array
{
$mergedDriver = [
'name' => 'PHPLIB',
'version' => self::getVersion(),
];

if (isset($driver['name'])) {
if (! is_string($driver['name'])) {
throw InvalidArgumentException::invalidType('"name" handshake option', $driver['name'], 'string');
}

$mergedDriver['name'] .= self::$handshakeSeparator . $driver['name'];
}

if (isset($driver['version'])) {
if (! is_string($driver['version'])) {
throw InvalidArgumentException::invalidType('"version" handshake option', $driver['version'], 'string');
}

$mergedDriver['version'] .= self::$handshakeSeparator . $driver['version'];
}

if (isset($driver['platform'])) {
$mergedDriver['platform'] = $driver['platform'];
}

return $mergedDriver;
}
}
20 changes: 18 additions & 2 deletions tests/ClientTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use MongoDB\Client;
use MongoDB\Driver\ClientEncryption;
use MongoDB\Driver\Exception\InvalidArgumentException as DriverInvalidArgumentException;
use MongoDB\Driver\ReadConcern;
use MongoDB\Driver\ReadPreference;
use MongoDB\Driver\WriteConcern;
Expand Down Expand Up @@ -38,9 +39,9 @@ public function testConstructorAutoEncryptionOpts()
/**
* @dataProvider provideInvalidConstructorDriverOptions
*/
public function testConstructorDriverOptionTypeChecks(array $driverOptions)
public function testConstructorDriverOptionTypeChecks(array $driverOptions, string $exception = InvalidArgumentException::class)
{
$this->expectException(InvalidArgumentException::class);
$this->expectException($exception);
new Client(static::getUri(), [], $driverOptions);
}

Expand All @@ -54,6 +55,21 @@ public function provideInvalidConstructorDriverOptions()

$options[][] = ['autoEncryption' => ['keyVaultClient' => 'foo']];

foreach ($this->getInvalidStringValues() as $value) {
$options[][] = ['driver' => ['name' => $value]];
}

foreach ($this->getInvalidStringValues() as $value) {
$options[][] = ['driver' => ['version' => $value]];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also test platform here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, but requires a little extra work since we only pass platform information through to the driver.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't realize that when I left my first comment. I see now that PHPLIB is only throwing its own InvalidArgumentException for the two options it merges. Thanks for adding this just the same.

}

foreach ($this->getInvalidStringValues() as $value) {
$options[] = [
'driverOptions' => ['driver' => ['platform' => $value]],
'exception' => DriverInvalidArgumentException::class,
];
}

return $options;
}

Expand Down