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

MongoDB support #2144

Merged
merged 114 commits into from Jan 18, 2019

Conversation

@alanpoulain
Copy link
Contributor

alanpoulain commented Aug 5, 2018

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Continuation of #1293.

What is left:

  • Documentation

Can be done after merging;

samvdb and others added some commits Mar 16, 2016

- add behat on mongo
- add symfony mongo test
- add mongo on travis & appveyor
- add behat on mongo
- add symfony mongo test
- add mongo on travis & appveyor
Merge branch 'odm-support' of https://github.com/samvdb/DunglasApiBundle
 into samvdb-odm-support

# Conflicts:
#	appveyor.yml
#	composer.json
#	src/Bridge/Doctrine/Filter/AbstractFilter.php
#	src/Bridge/Symfony/Bundle/DependencyInjection/Compiler/DoctrineQueryExtensionPass.php
#	src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php
#	tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php
#	tests/Bridge/Symfony/Bundle/DependencyInjection/Compiler/DoctrineQueryExtensionPassTest.php
#	tests/Bridge/Symfony/Bundle/DependencyInjection/ConfigurationTest.php
Merge branch 'odm-support' of https://github.com/samvdb/DunglasApiBundle
 into samvdb-odm-support

# Conflicts:
#	appveyor.yml
#	composer.json
#	src/Bridge/Doctrine/Filter/AbstractFilter.php
#	src/Bridge/Symfony/Bundle/DependencyInjection/Compiler/DoctrineQueryExtensionPass.php
#	src/Bridge/Symfony/Bundle/DependencyInjection/Configuration.php
#	tests/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtensionTest.php
#	tests/Bridge/Symfony/Bundle/DependencyInjection/Compiler/DoctrineQueryExtensionPassTest.php
#	tests/Bridge/Symfony/Bundle/DependencyInjection/ConfigurationTest.php
pagod

@alanpoulain alanpoulain changed the title [WIP] MongoDB support MongoDB support Jan 4, 2019

@alanpoulain

This comment has been minimized.

Copy link
Contributor Author

alanpoulain commented Jan 4, 2019

This PR is finally RFR 🎉 cc @api-platform/core-team

alanpoulain added some commits Jan 9, 2019

CS
@alanpoulain

This comment has been minimized.

Copy link
Contributor Author

alanpoulain commented Jan 13, 2019

Rebased against master using the new Pagination configuration!

@alcaeus
Copy link
Contributor

alcaeus left a comment

😅

Skipped tests and focused on ODM logic. Phew.

Show resolved Hide resolved src/Bridge/Doctrine/Common/Util/IdentifierManagerTrait.php Outdated
Show resolved Hide resolved src/Bridge/Doctrine/MongoDbOdm/CollectionDataProvider.php Outdated
Show resolved Hide resolved src/Bridge/Doctrine/MongoDbOdm/CollectionDataProvider.php Outdated
Show resolved Hide resolved src/Bridge/Doctrine/MongoDbOdm/Extension/FilterExtension.php Outdated
Show resolved Hide resolved src/Bridge/Doctrine/MongoDbOdm/Extension/OrderExtension.php Outdated
Show resolved Hide resolved src/Bridge/Doctrine/MongoDbOdm/Paginator.php Outdated
Show resolved Hide resolved src/Bridge/Doctrine/MongoDbOdm/Paginator.php Outdated
$alias .= $propertyAlias;
$referenceMapping = $classMetadata->getFieldMapping($association);
$aggregationBuilder->lookup($classMetadata->getAssociationTargetClass($association))

This comment has been minimized.

@alcaeus

alcaeus Jan 14, 2019

Contributor

ODM does this for you, throwing a helpful exception if you're using a reference that can't be looked up:

$aggregationBuilder
    ->lookup($association)
        ->alias($alias)

Your code does not handle different reference storage strategies (id, ref) appropriately.

This comment has been minimized.

@alanpoulain

alanpoulain Jan 14, 2019

Author Contributor

I had to do like this to manage to have nested lookups. It's because there is a garding if in the Lookup stage code using $this->class (https://github.com/doctrine/mongodb-odm/blob/55e1b9d87f6d60042878c368262d072931737b53/lib/Doctrine/ODM/MongoDB/Aggregation/Stage/Lookup.php#L154-L156).
I don't understand why you say it does not handle the different strategies. It's nearly the same code as in the Lookup class.

Show resolved Hide resolved src/Bridge/Doctrine/MongoDbOdm/PropertyInfo/DoctrineExtractor.php Outdated
return substr(
$className,
8 + $positionPm,
strrpos($className, '\\') - ($positionPm + 8)

This comment has been minimized.

@alcaeus

alcaeus Jan 14, 2019

Contributor

Maybe @Ocramius might want to chime in here, because I'm not sure if doing this for Proxy Manager is kosher. In MongoDB ODM, we wrote a resolver that internally hits the proxy manager logic to resolve the correct parent class. See https://github.com/doctrine/mongodb-odm/blob/181edb9226d783c319bc788632ec9ce0127395c0/lib/Doctrine/ODM/MongoDB/Proxy/ClassNameResolver.php#L11 for details.

This comment has been minimized.

@alanpoulain

alanpoulain Jan 14, 2019

Author Contributor

The code is duplicated from the Proxy manager code. I needed it to be a trait and not a service because it has too much impacts.

meyerbaptiste added some commits Jan 14, 2019

@alanpoulain

This comment has been minimized.

Copy link
Contributor Author

alanpoulain commented Jan 14, 2019

Thank you @alcaeus for your review (again)! @meyerbaptiste will fix some of the issues 🙂

@meyerbaptiste meyerbaptiste force-pushed the alanpoulain:mongodb branch from 13dd03c to 254a120 Jan 14, 2019

meyerbaptiste and others added some commits Jan 14, 2019

@alanpoulain alanpoulain force-pushed the alanpoulain:mongodb branch from dbfd0d8 to 222d235 Jan 16, 2019

@alanpoulain alanpoulain force-pushed the alanpoulain:mongodb branch from 222d235 to 346c1a2 Jan 16, 2019

@dunglas dunglas merged commit f7c19fa into api-platform:master Jan 18, 2019

9 of 11 checks passed

codecov/patch 92.53% of diff hit (target 95.93%)
Details
codecov/project 95.61% (-0.33%) compared to 3405a42
Details
Scrutinizer Analysis: 40 new issues, 147 updated code elements – Tests: passed
Details
SymfonyInsight Code quality OK.
Details
ci/circleci: behat-coverage Your tests passed on CircleCI!
Details
ci/circleci: merge-and-upload-coverage Your tests passed on CircleCI!
Details
ci/circleci: php-cs-fixer Your tests passed on CircleCI!
Details
ci/circleci: phpstan Your tests passed on CircleCI!
Details
ci/circleci: phpunit-coverage Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Jan 18, 2019

🎆thanks @alanpoulain!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment