-
Notifications
You must be signed in to change notification settings - Fork 266
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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 | ||
{ | ||
|
@@ -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; | ||
|
||
|
@@ -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; | ||
|
||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😆 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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); | ||
} | ||
|
||
|
@@ -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]]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we also test There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
|
||
foreach ($this->getInvalidStringValues() as $value) { | ||
$options[] = [ | ||
'driverOptions' => ['driver' => ['platform' => $value]], | ||
'exception' => DriverInvalidArgumentException::class, | ||
]; | ||
} | ||
|
||
return $options; | ||
} | ||
|
||
|
There was a problem hiding this comment.
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.