Skip to content
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.

Sign up for GitHub

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

Jump to bottom

PHPORM-56 Replace Collection proxy class with Driver monitoring #3137

Merged
merged 2 commits into from
Sep 6, 2024
Merged

PHPORM-56 Replace Collection proxy class with Driver monitoring #3137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21d8454
PHPORM-56 Replace Collection proxy class with Driver monitoring
GromNaN Sep 5, 2024
6374ad6
Update tests/ConnectionTest.php
GromNaN Sep 6, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ All notable changes to this project will be documented in this file.
* Remove `MongoFailedJobProvider`, replaced by Laravel `DatabaseFailedJobProvider` by @GromNaN in [#3122](https://github.com/mongodb/laravel-mongodb/pull/3122)
* Remove custom `PasswordResetServiceProvider`, use the default `DatabaseTokenRepository` by @GromNaN in [#3124](https://github.com/mongodb/laravel-mongodb/pull/3124)
* Remove `Blueprint::background()` method by @GromNaN in [#3132](https://github.com/mongodb/laravel-mongodb/pull/3132)
* Replace `Collection` proxy class with Driver monitoring by @GromNaN in [#3137]((https://github.com/mongodb/laravel-mongodb/pull/3137)
Copy link
Member

Choose a reason for hiding this comment

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

Might apply to other BC breaks as well, but do we want to leverage deprecations in 1.x to announce the break?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can mark the class as deprecated in 1.x. Unless we add an option to switch the monitoring feature from the proxy class to the monitoring events, there will be no way to disable the deprecation.

Copy link
Member

Choose a reason for hiding this comment

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

Got it - not sure we need to do this, just wanted to check 👍


## [4.8.0] - 2024-08-27

Expand Down
2 changes: 1 addition & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
"illuminate/database": "^11",
"illuminate/events": "^11",
"illuminate/support": "^11",
"mongodb/mongodb": "^1.15"
"mongodb/mongodb": "^1.18"
Copy link
Member Author

Choose a reason for hiding this comment

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

Requires mongodb/mongo-php-library#1195

},
"require-dev": {
"mongodb/builder": "^0.2",
Expand Down
2 changes: 1 addition & 1 deletion docs/includes/query-builder/QueryBuilderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
use Illuminate\Pagination\AbstractPaginator;
use Illuminate\Support\Facades\DB;
use MongoDB\BSON\Regex;
use MongoDB\Laravel\Collection;
use MongoDB\Collection;
use MongoDB\Laravel\Tests\TestCase;

use function file_get_contents;
Expand Down
2 changes: 1 addition & 1 deletion src/Bus/MongoBatchRepository.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
use Illuminate\Support\Carbon;
use MongoDB\BSON\ObjectId;
use MongoDB\BSON\UTCDateTime;
use MongoDB\Collection;
use MongoDB\Driver\ReadPreference;
use MongoDB\Laravel\Collection;
use MongoDB\Laravel\Connection;
use MongoDB\Operation\FindOneAndUpdate;
use Override;
Expand Down
2 changes: 1 addition & 1 deletion src/Cache/MongoLock.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
use Illuminate\Support\Carbon;
use InvalidArgumentException;
use MongoDB\BSON\UTCDateTime;
use MongoDB\Laravel\Collection;
use MongoDB\Collection;
use MongoDB\Operation\FindOneAndUpdate;
use Override;

Expand Down
2 changes: 1 addition & 1 deletion src/Cache/MongoStore.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
use Illuminate\Contracts\Cache\Store;
use Illuminate\Support\Carbon;
use MongoDB\BSON\UTCDateTime;
use MongoDB\Laravel\Collection;
use MongoDB\Collection;
use MongoDB\Laravel\Connection;
use MongoDB\Operation\FindOneAndUpdate;
use Override;
Expand Down
80 changes: 0 additions & 80 deletions src/Collection.php
View file
Open in desktop

This file was deleted.

53 changes: 53 additions & 0 deletions src/CommandSubscriber.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?php

namespace MongoDB\Laravel;

use MongoDB\BSON\Document;
use MongoDB\Driver\Monitoring\CommandFailedEvent;
use MongoDB\Driver\Monitoring\CommandStartedEvent;
use MongoDB\Driver\Monitoring\CommandSubscriber as CommandSubscriberInterface;
use MongoDB\Driver\Monitoring\CommandSucceededEvent;

use function get_object_vars;
use function in_array;

/** @internal */
final class CommandSubscriber implements CommandSubscriberInterface
{
/** @var array<string, CommandStartedEvent> */
private array $commands = [];

public function __construct(private Connection $connection)
{
}

public function commandStarted(CommandStartedEvent $event)
{
$this->commands[$event->getOperationId()] = $event;
}

public function commandFailed(CommandFailedEvent $event)
{
$this->logQuery($event);
}

public function commandSucceeded(CommandSucceededEvent $event)
{
$this->logQuery($event);
}

private function logQuery(CommandSucceededEvent|CommandFailedEvent $event): void
{
$startedEvent = $this->commands[$event->getOperationId()];
unset($this->commands[$event->getOperationId()]);

$command = [];
foreach (get_object_vars($startedEvent->getCommand()) as $key => $value) {
if ($key[0] !== '$' && ! in_array($key, ['lsid', 'txnNumber'])) {
Copy link
Member

Choose a reason for hiding this comment

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

I never checked the performance of this, but why not use str_starts_with?

Copy link
Member Author

@GromNaN GromNaN Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The array key is never empty, and I need to check only 1 char. This skips 1 method call.

public function benchStrStartsWith()
{
    $key = '$db';
    return str_starts_with($key, '$');
}

public function benchStrictCmp()
{
    $key = '$db';
    return $key[0] === '$';
}
benchStrStartsWith......................I2 - Mo0.185μs (±0.26%)
benchStrictCmp..........................I2 - Mo0.135μs (±0.60%)

$command[$key] = $value;
}
}

$this->connection->logQuery(Document::fromPHP($command)->toCanonicalExtendedJSON(), [], $event->getDurationMicros());
}
}
17 changes: 9 additions & 8 deletions src/Connection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use Illuminate\Database\Connection as BaseConnection;
use InvalidArgumentException;
use MongoDB\Client;
use MongoDB\Collection;
use MongoDB\Database;
use MongoDB\Driver\Exception\AuthenticationException;
use MongoDB\Driver\Exception\ConnectionException;
Expand Down Expand Up @@ -47,6 +48,8 @@ class Connection extends BaseConnection
*/
protected $connection;

private ?CommandSubscriber $commandSubscriber;

/**
* Create a new database connection instance.
*/
Expand All @@ -62,6 +65,8 @@ public function __construct(array $config)

// Create the connection
$this->connection = $this->createConnection($dsn, $config, $options);
$this->commandSubscriber = new CommandSubscriber($this);
$this->connection->addSubscriber($this->commandSubscriber);

// Select database
$this->db = $this->connection->selectDatabase($this->getDefaultDatabaseName($dsn, $config));
Expand Down Expand Up @@ -97,9 +102,9 @@ public function table($table, $as = null)
*
* @return Collection
*/
public function getCollection($name)
public function getCollection($name): Collection
{
return new Collection($this, $this->db->selectCollection($this->tablePrefix . $name));
return $this->db->selectCollection($this->tablePrefix . $name);
}

/** @inheritdoc */
Expand Down Expand Up @@ -198,6 +203,8 @@ public function ping(): void
/** @inheritdoc */
public function disconnect()
{
$this->connection?->removeSubscriber($this->commandSubscriber);
$this->commandSubscriber = null;
Copy link
Member

Choose a reason for hiding this comment

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

Unregistering the subscriber seems correct, but I'm more generally curious how the disconnect() method is used in Laravel. Wouldn't it be misleading to users given that PHPC persists the connections by default?

I assume Laravel never uses the disableClientPersistence driver option.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm just making sure that the objects are deleted without requiring a garbage collector cycle.

Copy link
Member

Choose a reason for hiding this comment

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

I understand why you made the additions to this method. It was a more general question about what the purpose of disconnect() was for the Laravel MongoDB library. Is it something we're obligated to implement for Eloquent?

$this->connection = null;
}

Expand Down Expand Up @@ -264,12 +271,6 @@ protected function getDsn(array $config): string
throw new InvalidArgumentException('MongoDB connection configuration requires "dsn" or "host" key.');
}

/** @inheritdoc */
public function getElapsedTime($start)
{
return parent::getElapsedTime($start);
}

/** @inheritdoc */
public function getDriverName()
{
Expand Down
5 changes: 2 additions & 3 deletions src/Query/AggregationBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@
use Iterator;
use MongoDB\Builder\BuilderEncoder;
use MongoDB\Builder\Stage\FluentFactoryTrait;
use MongoDB\Collection as MongoDBCollection;
use MongoDB\Collection;
use MongoDB\Driver\CursorInterface;
use MongoDB\Laravel\Collection as LaravelMongoDBCollection;

use function array_replace;
use function collect;
Expand All @@ -24,7 +23,7 @@ class AggregationBuilder
use FluentFactoryTrait;

public function __construct(
private MongoDBCollection|LaravelMongoDBCollection $collection,
private Collection $collection,
private readonly array $options = [],
) {
}
Expand Down
2 changes: 1 addition & 1 deletion src/Query/Builder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ class Builder extends BaseBuilder
/**
* The database collection.
*
* @var \MongoDB\Laravel\Collection
* @var \MongoDB\Collection
*/
protected $collection;

Expand Down
8 changes: 4 additions & 4 deletions src/Schema/Blueprint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

use Illuminate\Database\Connection;
use Illuminate\Database\Schema\Blueprint as SchemaBlueprint;
use MongoDB\Laravel\Collection;
use MongoDB\Collection;

use function array_flip;
use function implode;
Expand All @@ -21,14 +21,14 @@ class Blueprint extends SchemaBlueprint
/**
* The MongoConnection object for this blueprint.
*
* @var \MongoDB\Laravel\Connection
* @var Connection
*/
protected $connection;

/**
* The MongoCollection object for this blueprint.
* The Collection object for this blueprint.
*
* @var Collection|\MongoDB\Collection
* @var Collection
*/
protected $collection;

Expand Down
2 changes: 1 addition & 1 deletion tests/Cache/MongoLockTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
use Illuminate\Support\Facades\DB;
use InvalidArgumentException;
use MongoDB\BSON\UTCDateTime;
use MongoDB\Collection;
use MongoDB\Laravel\Cache\MongoLock;
use MongoDB\Laravel\Collection;
use MongoDB\Laravel\Tests\TestCase;
use PHPUnit\Framework\Attributes\TestWith;

Expand Down
2 changes: 1 addition & 1 deletion tests/Casts/DecimalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
use MongoDB\BSON\Int64;
use MongoDB\BSON\Javascript;
use MongoDB\BSON\UTCDateTime;
use MongoDB\Laravel\Collection;
use MongoDB\Collection;
use MongoDB\Laravel\Tests\Models\Casting;
use MongoDB\Laravel\Tests\TestCase;

Expand Down
36 changes: 0 additions & 36 deletions tests/CollectionTest.php
View file
Open in desktop

This file was deleted.

Loading
Loading