Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Lock] add mongodb store#31889

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromkralos:27345-mongodb-lock-store-43
Dec 11, 2019

Conversation

@kralos
Copy link

@kraloskralos commentedJun 6, 2019•
edited
Loading

QA
Branch?4.4
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes (requiresext-mongodb andmongodb/mongodb to test)
Fixed tickets#27345
LicenseMIT
Original Doc PRsymfony/symfony-docs#9807
Remove from 4.3 Doc PRsymfony/symfony-docs#11686
Add to4.4 5.1 Doc PRsymfony/symfony-docs#11735

Looks like I messed upkralos:27345-lock-mongodb with a force push (trying to fix ci issues) right before it was merged tomaster (4.3.0).

see#27648

Description
We should support Semaphore Locks with a MongoDB back end to allow those that already use MongoDB as a distributed storage engine.

Symfony already partially supports MongoDB for session storage:Symfony\Component\HttpFoundation\Session\Storage\Handler\MongoDbSessionHandler

Example

$client =newMongoDb\Client();$store =newSymfony\Component\Lock\Store\MongoDbStore($client    array('database' =>'my-app',    ));$lockFactory =newSymfony\Component\Lock\Factory($store);$lock =$lockFactory->createLock('my-resource');

@kralos
Copy link
Author

kralos commentedJun 6, 2019•
edited
Loading

@fabpot Sorry, this was supposed to be part of#27648 but got missed due to a messed up force push.

I've based this PR on4.3, is that ok?

rebased to4.4

@kralos
Copy link
Author

This PR was based on the version approved by@jderusse1f4d601

which had a failing test (we last minute removed the return index name fromMongoDbStore::createTtlIndex).

I have then fixed that failing test71091fa..d319a2a

@nicolas-grekasnicolas-grekas added this to the4.3 milestoneJun 6, 2019
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedJun 6, 2019•
edited
Loading

Please targetmaster 4.4, that's a new feature for sure.

@nicolas-grekasnicolas-grekas modified the milestones:4.3,nextJun 6, 2019
@nicolas-grekasnicolas-grekas changed the title27345 mongodb lock store[Lock] add mongodb storeJun 6, 2019
@nicolas-grekasnicolas-grekas changed the base branch from4.3 to4.4June 6, 2019 07:08
@nicolas-grekasnicolas-grekas changed the base branch from4.4 to4.3June 6, 2019 07:09
@kraloskralosforce-pushed the27345-mongodb-lock-store-43 branch from55526dc to0e77ec6CompareJune 6, 2019 09:26
@kralos
Copy link
Author

rebased from4.4

@kralos
Copy link
Author

kralos commentedJun 6, 2019•
edited
Loading

I've stripped it fromsymfony/symfony-docs4.3symfony/symfony-docs#11686

Will add back tosymfony/symfony-docs4.4 once the above PR is merged and upstreamed tosymfony/symfony-docs4.4

Added insymfony/symfony-docs#11735

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestJun 12, 2019
This PR was merged into the 4.3 branch.Discussion----------[Lock] mongodb store removed from symfony 4.3MongoDbStore wasn't actually added in symfony 4.3, it will be added in 4.4.Seesymfony/symfony#31889Commits-------75ad033 #27345 Removed Lock MongoDbStore, it will be added in symfony 4.4

/**
* MongoDbStore is a StoreInterface implementation using MongoDB as a storage
* engine.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about adding that it requires an external lib?

Copy link
Author

Choose a reason for hiding this comment

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

The patternpublic static function isSupported(): bool has been added toMongoDbStore as well as@requires extension mongodb on the class. I've also moved theCAUTION: phpdoc up to the class to be inline with other stores.

noniagriconomie reacted with thumbs up emoji
@kraloskralosforce-pushed the27345-mongodb-lock-store-43 branch 2 times, most recently from753d398 tof3f8568CompareJuly 4, 2019 00:17
@kralos
Copy link
Author

I'm looking into the failing test later today

Copy link
Contributor

@SimperfitSimperfit left a comment

Choose a reason for hiding this comment

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

Nice feature !
Status: Needs Work

*
* @internal
*/
publicstaticfunctionisSupported():bool
Copy link
Contributor

Choose a reason for hiding this comment

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

imho this is not needed

Copy link
Author

Choose a reason for hiding this comment

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

I did this because some of Jérémy's stores do it. I thought it might be useful later if a bundle extension needs to check support to auto default/configure something supported, but maybe we could add an interface for this later to all stores if we ever did something like that. Also thinking about it more... you could never auto configure a MongoDbStore since the database name option is mandatory. So I have removedstatic::isSupported().

*/
publicfunction__construct(Client$mongo,array$options,float$initialTtl =300.0)
{
if (!static::isSupported()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can directly check if it is supported here.

Copy link
Author

Choose a reason for hiding this comment

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

done

*
* @see https://docs.mongodb.com/manual/applications/replication/
*/
publicfunction__construct(Client$mongo,array$options,float$initialTtl =300.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

you should not type-hint the mongodb client, what happens if he does not exist ?

Copy link
Author

Choose a reason for hiding this comment

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

Then the developer wouldn't be able to construct aMongoDB\Client. If they pass something that isn't aMongoDB\Client they would get a TypeError:

Uncaught TypeError: Argument 1 passed to Symfony\Component\Lock\Store\MongoDbStore::__construct() must be an instance of MongoDB\Client, x given

This is what I would want to happen

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you don't have MongoDB ?

Copy link
Author

@kraloskralosJul 5, 2019•
edited
Loading

Choose a reason for hiding this comment

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

if you don't havemongodb/mongodb you can't construct aMongoDB\Client

if you don't haveext-mongodb you can'tcomposer require mongodb/mongodb

if you somehow have installedmongodb/mongodb and don't haveext-mongodb you will fail to construct aMongoDB\Client (this ismongodb/mongodb's responsibility):

<?phprequire__DIR__.'/vendor/autoload.php';$mongoClient =newMongoDB\Client('mongodb://127.0.0.1');$store =newSymfony\Component\Lock\Store\MongoDbStore($mongoClient, ['database' =>'test',]);
$ docker run --rm -it -v $PWD:$PWD -w $PWD php php ./test.phpFatal error: Uncaught Error: Class 'MongoDB\Driver\Manager' not found in /home/kralos/src/symfony/vendor/mongodb/mongodb/src/Client.php:87Stack trace:#0 /home/kralos/src/symfony/test.php(5): MongoDB\Client->__construct('mongodb://127.0...')#1 {main}  thrown in /home/kralos/src/symfony/vendor/mongodb/mongodb/src/Client.php on line 87

Copy link
Author

Choose a reason for hiding this comment

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

MongoDB\Driver\Manager is provided byext-mongodb

MongoDB\Client is provided bymongodb/mongodb

Symfony\Component\Lock\Store\MongoDbStore depends onmongodb/mongodb

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you not type-hint MongoDB\Client for that reason. maybe i'm wrong cc@jderusse

Copy link
Author

@kraloskralosJul 6, 2019•
edited
Loading

Choose a reason for hiding this comment

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

I removed theif (!\extension_loaded('mongodb')) { check in the constructor since it's impossible to hit it. You can't pass aMongoDB\Client withoutmongodb/mongodb which you can't have withoutext-mongodb.

We still have in the class docblock@requires extension mongodb


if (!isset($this->options['database'])) {
thrownewInvalidArgumentException(
'You must provide the "database" option for MongoDBStore'
Copy link
Contributor

Choose a reason for hiding this comment

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

you should inline this

Copy link
Author

Choose a reason for hiding this comment

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

done


if ($this->options['gcProbablity'] <0.0 ||$this->options['gcProbablity'] >1.0) {
thrownewInvalidArgumentException(sprintf(
'"%s" gcProbablity must be a float from 0.0 to 1.0, "%f" given.',
Copy link
Contributor

Choose a reason for hiding this comment

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

you should inline this

Copy link
Author

Choose a reason for hiding this comment

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

done

@kraloskralosforce-pushed the27345-mongodb-lock-store-43 branch 2 times, most recently from6df4973 toa1fe6d0CompareAugust 22, 2019 04:22
@kralos
Copy link
Author

git rebase origin/4.4 and squashed

@javiereguiluz
Copy link
Member

This one looks ready for Symfony 4.4/5.0 👍

OskarStark reacted with thumbs up emoji

|| (random_int(0,PHP_INT_MAX) /PHP_INT_MAX) <=$this->options['gcProbablity']
)
) {
if ($this->getDatabaseVersion() <'2.2') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This works by accident,version_compare should be used for these

>>> '2.10' < '2.2'=> true

instances: 2

But why are we adding support for legacy version anyway? 2.2 was EOL-ed in2014. We are paying the price not just with added complexity but with extra query on each request as well.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I'd prefer not to support<2.2. removing support and documenting requirements

{
$this->getCollection()->deleteMany([// filter
'expires_at' => [
'$lte' =>$this->createDateTime(microtime(true)),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is inconsistent. IMHO parameter should either be non-optional, or not specified here

Copy link
Author

Choose a reason for hiding this comment

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

agreed, I've opted for not optional. the objective of the function is to create a MongoDb compliantUTCDateTime to millisecond precision.

if (1 ===\count($writeErrors)) {
$code =$writeErrors[0]->getCode();
}else {
$code =$e->getCode();
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor thing, but else branch could be easily avoided here by specifying this as default. This is super cheap call

Copy link
Author

Choose a reason for hiding this comment

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

updated

@kralos
Copy link
Author

kralos commentedOct 15, 2019•
edited
Loading

@nicolas-grekas can you let me know if you're happy for this to be merged? I'll rebase/squash etc if everyone is happy. The appveyor CI failure is not Lock component by the way...

Testing src\Symfony/Component/HttpClient1) Symfony\Component\HttpClient\Tests\CurlHttpClientTest::testToStreamSymfony\Component\HttpClient\Exception\TransportException: Couldn't connect to server for "http://localhost:8057/".

Copy link
Member

@jderussejderusse left a comment

Choose a reason for hiding this comment

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

Could you update theStoreFactory to add theMongoDB\Client andmongodb:// in the list of managed DSN.
Need to update theCHANGELOG.md

@kralos
Copy link
Author

kralos commentedNov 5, 2019•
edited
Loading

Could you update theStoreFactory to add theMongoDB\Client andmongodb:// in the list of managed DSN.
Need to update theCHANGELOG.md

In order to addMongoDbStore support toStoreFactory theMongoDbStore::__construct needs to be able to construct without$options.

I have therefore added support for passing aMongoDB\Collection toMongoDbStore::__construct.

I have also added DSN support toMongoDbStore::__construct.

NeitherClient nor DSN alone will be enough to construct aMongoDbStore as thedatabase needs to be specified in$options thereforeStoreFactory only supportsMongoDB\Collection.

CHANGELOG.md has been updated.

CI is currently passing (the errors from travis are related to components other thenComponent\Lock). AllComponent\Lock tests are passing.

I've left the commit history for review purposes, if we are happy with the current iteration let me know and I can squash (if required).


case0 ===strpos($connection,'flock://'):
returnnewFlockStore(substr($connection,8));

Copy link
Member

Choose a reason for hiding this comment

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

revert

Copy link
Author

Choose a reason for hiding this comment

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

fixed, something funny happened with myrebase


if ($this->options['gcProbablity'] <0.0 ||$this->options['gcProbablity'] >1.0) {
thrownewInvalidArgumentException(sprintf('"%s" gcProbablity must be a float from 0.0 to 1.0, "%f" given.',__METHOD__,$this->options['gcProbablity']));
thrownewInvalidArgumentException(sprintf('%s() gcProbablity must be a float from 0.0 to 1.0, "%f" given.',__METHOD__,$this->options['gcProbablity']));
Copy link
Member

Choose a reason for hiding this comment

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

remove" in"%f" given too (for consistency)

Copy link
Author

Choose a reason for hiding this comment

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

updated

}

if (null ===$this->collection && !isset($this->options['database'])) {
thrownewInvalidArgumentException(sprintf('%s() requires the "database" option when constructing with a %s',__METHOD__,\is_object($mongo) ?\get_class($mongo) :\gettype($mongo)));
Copy link
Member

Choose a reason for hiding this comment

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

database and collection names could be defined in the DSN (similar to PDO). dsn =mongodb://[username:password@]host1[:port1][,host2[:port2:],...]/[database/][collection] ?

Copy link
Author

Choose a reason for hiding this comment

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

The database you see there is the authentication database, not the database to use for data

Copy link
Member

@jderussejderusseNov 5, 2019•
edited
Loading

Choose a reason for hiding this comment

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

isn't it the purpose of the querystring parameterauthSource?https://symfony.com/doc/master/bundles/DoctrineMongoDBBundle/installation.html#authentication

anyway, if thepath component is a standard to declare the auth Database, we can use custom queryString parameters likemongodb://localhost/?lockDatabase=foo&lockCollection=bar

If we can not create the Store from a DSN, developpers using the FrameworkBundle won't be able to use it from the configurationframework.lock = '%env(LOCK_DNS)% (seesymfony/symfony-docs#12523)

Copy link
Author

Choose a reason for hiding this comment

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

isn't it the purpose of the querystring parameterauthSource?

Actually I just tested this (mongodb 4.2.0) and if both/path and?authSource are specified/path means application database and?authSource means authentication database. We are still stuck for getting the collection name.

we can use custom queryString parameters

Seems to be the only way, do you have any objection to us taking?collection=bar? I feel like if we ever had a conflict it would be because MongoDB is using this param for the exact same purpose.

I'm going to enforce when$mongo is a:

  • MongoDB\Collection:
    • $options['database'] is ignored
    • $options['collection'] is ignored
  • MongoDB\Client:
    • $options['database'] is mandatory
    • $options['collection'] is mandatory
  • MongoDB Connection URI string:
    • /path is used otherwise$options['database'], at least one is mandatory
    • ?collection= is used otherwise$options['collection'], at least one is mandatory

Comment on lines 120 to 126
$database =parse_url($mongo,PHP_URL_PATH);
if (null !==$database) {
$this->options['database'] =$database;
}
$query =parse_url($mongo,PHP_URL_QUERY);
$queryParams = [];
parse_str($query ??'',$queryParams);
$collection =$queryParams['collection'] ??null;
if (null !==$collection) {
$this->options['collection'] =$collection;
}
Copy link
Member

Choose a reason for hiding this comment

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

what's about something similare to Messenger PDO

if (false ===$parsedUrl =parse_url($dsn)) {thrownewInvalidArgumentException(sprintf('The given Mongodb DSN "%s" is invalid.',$dsn));}$query = [];if (isset($parsedUrl['query'])) {parse_str($parsedUrl['query'],$query);}$this->option =$option +$query;$this->options['database'] =$this->options['database'] ??ltrim($parsedUrl['path'] ??'','/') ?:null;

Copy link
Author

Choose a reason for hiding this comment

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

You prefer$options to take precedence over/path?collection=?

<?phpdeclare(strict_types=1);$mongo ='mongodb://127.0.0.1/uri-db?collection=uri-col';$options = ['database' =>'option-db','collection' =>'option-col',];if (false ===$parsedUrl =parse_url($mongo)) {thrownew \InvalidArgumentException(sprintf('The given MongoDB Connection URI "%s" is invalid.',$mongo));}$query = [];if (isset($parsedUrl['query'])) {parse_str($parsedUrl['query'],$query);}$options +=$query;$options['database'] =$options['database'] ??ltrim($parsedUrl['path'] ??'','/') ?:null;print_r($options);/* Array(    [database] => option-db    [collection] => option-col) */

Copy link
Author

@kraloskralosNov 5, 2019•
edited
Loading

Choose a reason for hiding this comment

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

I've implemented this but without polluting$this->options with querystring params other thancollection.

if (false ===$parsedUrl =parse_url($mongo)) {thrownewInvalidArgumentException(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;

@kralos
Copy link
Author

@jderusse happy with current implementation? want me to squash?

Copy link
Member

@jderussejderusse left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks@kralos

@kralos
Copy link
Author

@nicolas-grekas can we get this merged? do you need a squash first? I don't want to missv4.4

@fabpotfabpot changed the base branch from4.4 tomasterDecember 11, 2019 00:40
@fabpotfabpotforce-pushed the27345-mongodb-lock-store-43 branch fromaf7825d toa6bfa59CompareDecember 11, 2019 00:40
@fabpot
Copy link
Member

Thank you@kralos.

@fabpotfabpot merged commita6bfa59 intosymfony:masterDec 11, 2019
@nicolas-grekasnicolas-grekas modified the milestones:next,5.1May 4, 2020
@fabpotfabpot mentioned this pull requestMay 5, 2020
@kraloskralos deleted the 27345-mongodb-lock-store-43 branchAugust 17, 2020 23:40
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@jderussejderussejderusse approved these changes

@dunglasdunglasAwaiting requested review from dunglas

@lyrixxlyrixxAwaiting requested review from lyrixx

@srozesrozeAwaiting requested review from sroze

@xabbuhxabbuhAwaiting requested review from xabbuh

+3 more reviewers

@ostroluckyostroluckyostrolucky left review comments

@SimperfitSimperfitSimperfit approved these changes

@noniagriconomienoniagriconomienoniagriconomie left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.1

Development

Successfully merging this pull request may close these issues.

10 participants

@kralos@nicolas-grekas@OskarStark@javiereguiluz@fabpot@ostrolucky@jderusse@Simperfit@noniagriconomie@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp