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.

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

[WIP] MongoDB Support #461

Closed
wants to merge 24 commits into from
Closed

[WIP] MongoDB Support #461

wants to merge 24 commits into from

Conversation

@samvdb
Copy link
Contributor

@samvdb samvdb commented Mar 16, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets #439, #361
License MIT

This is a work in progress. I will not be working on it but it might help others to get started.
Stuff to fix:

  • Add filters and enable them in the service xml
  • Add tests
  • Abstract ODM and ORM data providers since logic is duplicated
  • Abstract Paginator since it's duplicated
  • Move Query extension interfaces to data layer agnostic namespace
  • Filter/AbstractFilter should not contain ORM logic (see addJoinsForNestedProperty)
  • Add ext-mongo to travis-ci
@dunglas
Copy link
Member

@dunglas dunglas commented Mar 16, 2016

About embedded documents, they are not resources (in sense of API Platform) but raw objects. IMO the best is do nothing: let the standard ObjectNormalizer of Symfony normalizing and denormalizing it (as we already do for ORM @Embeddable and \DateTime).

The only adaptation I can think about is documenting such embedded types in the Hydra ApiDocumentationGenerator and ContextBuilder but it can be done later in another PR.

@dunglas dunglas added the enhancement label Mar 16, 2016
@samvdb
Copy link
Contributor Author

@samvdb samvdb commented Mar 16, 2016

@dunglas, i agree with you that embedded documents should not be processed as resources. However there were some problems in the old serializer (before refactor to symf abstract serializer).
My resources are beeing serialized atm without errors, embedded docs are still empty but that is probably a serialization context problem. Will test it out further. I hope other people are interested in mongo support and are willing to help out.

Thanks

@dunglas
Copy link
Member

@dunglas dunglas commented Mar 16, 2016

Yes this problem has been figured out in the v2.

I'm personally very interested by Mongo support.

use ApiPlatform\Core\Bridge\Doctrine\MongoDB\Extension\QueryCollectionExtensionInterface;

/**
* Collection data provider for the Doctrine ORM.

This comment has been minimized.

@dunglas

dunglas Mar 16, 2016
Member

Doctrine MongoDB ODM.

/**
* Collection data provider for the Doctrine ORM.
*/
class CollectionDataProvider implements CollectionDataProviderInterface

This comment has been minimized.

@dunglas

dunglas Mar 16, 2016
Member

Is this exactly the same class that the class for the ORM?

If it is the case, this class can be moved in the Doctrine and used by both instead of being duplicated. If changes are small, an abstract class can be introduced.

This comment has been minimized.

@samvdb

samvdb Mar 16, 2016
Author Contributor

Yeah, the interfaces for query extensions should be pulled out of ORM also i think. The paginator could be abstracted since the only read difference if the getResult and applyToCollection.

If you want me to proceed with this give me the go ahead :)

This comment has been minimized.

@dunglas

dunglas Mar 16, 2016
Member

Yes 👍 for that.

@@ -61,6 +63,15 @@ public function getConfigTreeBuilder()
->scalarNode('name_converter')->defaultNull()->info('Specify a name converter to use.')->end()
->booleanNode('enable_fos_user')->defaultValue(false)->info('Enable the FOSUserBundle integration.')->end()
->booleanNode('enable_nelmio_api_doc')->defaultTrue()->info('Enable the Nelmio Api doc integration.')->end()
->scalarNode('db_driver')

This comment has been minimized.

@dunglas

dunglas Mar 16, 2016
Member

It would be nice to be able MongoDB and a relational DBMS at the same time.

This comment has been minimized.

@samvdb

samvdb Mar 16, 2016
Author Contributor

mmm, maybe work towards a service definition like - managers: - orm: default_em, - mongodb: default_dm ?

This comment has been minimized.

@dunglas

dunglas Mar 16, 2016
Member

Or just register the service if the MongoODMBundle is registered. If a user want to disable it, it can still use a compiler pass.

This comment has been minimized.

@dunglas

dunglas Mar 16, 2016
Member

Or better, flags like for FOSUser and NelmioApiDoc: enable_doctrine_orm, enable_doctrine_mongodb_odm.

->scalarNode('db_driver')
->validate()
->ifNotInArray($supportedDrivers)
->thenInvalid('The driver %s is not supported. Please choose one of '.json_encode($supportedDrivers))

This comment has been minimized.

@dunglas

dunglas Mar 16, 2016
Member

join()?


if (!$fetchData || $manager instanceof DocumentManager) {
// @todo: Check potential problems with using the $id instead of $identifiers
return $manager->getReference($resourceClass, $id);

This comment has been minimized.

@samvdb

samvdb Mar 16, 2016
Author Contributor

@dunglas, any thoughts on this?

This comment has been minimized.

@dunglas

dunglas Mar 16, 2016
Member

IIRC MongoDB doesn't support composed primary key? If so, it can be ignored and just use $identifierValues[0].

This comment has been minimized.

@samvdb

samvdb Mar 16, 2016
Author Contributor

There are severals options for compound keys, i would however not include support for this in this PR.

@samvdb
Copy link
Contributor Author

@samvdb samvdb commented Mar 16, 2016

@dunglas, what about the abstraction of the data providers or pagination extension?
I updated the rest of the comments

@samvdb
Copy link
Contributor Author

@samvdb samvdb commented Mar 16, 2016

I can confirm that the serializer is working properly for embedded documents!

@dunglas
Copy link
Member

@dunglas dunglas commented Mar 16, 2016

@dunglas, what about the abstraction of the data providers or pagination extension?

I've not got it.

@samvdb
Copy link
Contributor Author

@samvdb samvdb commented Mar 16, 2016

@dunglas any idea how to fix the nelmio apidoc parser to support embedded documents?
Example: @Property(iri="https://schema.org/Offer", readableLink=false)
src/Metadata/Property/Factory/SerializerPropertyMetadataFactory.php:66 throws an exception because the document is not a resource and thus marking the property as a readable link.

When parsing the api it is being rendered as an IRI which is not correct.
see src/Bridge/NelmioApiDoc/Parser/ApiPlatformParser.php:194
Do you propose adding an extra property in the Annotation or create a workaround somewhere?

@dunglas
Copy link
Member

@dunglas dunglas commented Mar 16, 2016

Maybe can we use the Symfony PropertyInfo component to list properties of this embedded object. Same apply for Hydra doc. But IMO it should be done in a separate PR. Let's focus on MongoDB support here.

@jonny827
Copy link

@jonny827 jonny827 commented Mar 17, 2016

I am interested in support for DynamoDB and other non-MongoDB projects as well. Would it potentially be helpful to use an existing project to be able to push API-platform to several other platforms without "re-inventing the wheel"? You could benefit from their aligning several systems with a unified API. Use their API to communicate with API-platform. When they add support, we get all the benefits and you would only have to maintain the interface to their their API.

[https://github.com/doctrine/KeyValueStore]

/**
* @param ManagerRegistry $managerRegistry
* @param QueryCollectionExtensionInterface[] $collectionExtensions
* @param CollectionDataProviderInterface|null $decorated

This comment has been minimized.

@sroze

sroze Mar 17, 2016
Member

My personal preference is to inject the decorated object as the first argument, so we can easily see that it's a decorator.

This comment has been minimized.

@samvdb

samvdb Mar 17, 2016
Author Contributor

I believe it is best practice for arguments that are null by default to be listed last.

This comment has been minimized.

@sroze

sroze Mar 17, 2016
Member

Oh I didn't see that the decorated argument could be null. So... that means that it's not the "decorated" one, you should find another name.

}

$manager = $this->managerRegistry->getManagerForClass($resourceClass);
if (null === $manager) {

This comment has been minimized.

@sroze

sroze Mar 17, 2016
Member

Maybe you can merge these lines?

if (null === ($manager = $this->managerRegistry->getManagerForClass($resourceClass))) {
$this->enabled = $enabled;
$this->clientEnabled = $clientEnabled;
$this->clientItemsPerPage = $clientItemsPerPage;
$this->itemsPerPage = $itemsPerPage;

This comment has been minimized.

@sroze

sroze Mar 17, 2016
Member

To me, we have too many parameters here. That would be nice to create a normalised PaginationConfiguration object, that we could use both here and in the Doctrine implementation.

This comment has been minimized.

@samvdb

samvdb Mar 17, 2016
Author Contributor

Totally agree with you!

return new Paginator($queryBuilder->getQuery()->execute());
}

private function isPaginationEnabled(Request $request, ResourceMetadata $resourceMetadata, string $operationName = null) : bool

This comment has been minimized.

@sroze

sroze Mar 17, 2016
Member

That looks like to be copy/pasted, or at least, that's not specific to MongoDb. Is there a way you can decouple that?

This comment has been minimized.

@samvdb

samvdb Mar 17, 2016
Author Contributor

It is copy pasted from the ORM version, don't have much time to work on it to cleanup everything. Like i said it's a PR kickstart mongo support :)

This comment has been minimized.

@sroze

sroze Mar 17, 2016
Member

Yup, but you know... even if we tried to decouple from the ORM, it's always the first one that actually introduce the second real adapter that suffers 😛

This comment has been minimized.

}

$identifiers[$propertyName] = $identifierValues[$i];
++$i;

This comment has been minimized.

@sroze

sroze Mar 17, 2016
Member

Instead of having this $i variable, you can probably use the foreach ... as $i => $propertyName.

This comment has been minimized.

@samvdb

samvdb Mar 17, 2016
Author Contributor

This logic should also be abstracted since it's duplicated for ORM and ODM

@sroze
Copy link
Member

@sroze sroze commented Mar 17, 2016

Thank you @samvdb. Obviously we need to go through the review process before. :)

Can you add tests? Also, I can see that many of your classes are actually not used (because commented in the service.xml file), so could you add in your PR description a checkbox list of the tasks you've done and the tasks you have to do before we review it again?

@nettob
Copy link

@nettob nettob commented Apr 14, 2016

@eopaquette https://github.com/mongofill/mongofill is an pure PHP replacement of the MongoDB driver

@dunglas
Copy link
Member

@dunglas dunglas commented May 9, 2016

Hello @samvdb, any update regarding this PR? It would be so cool to have it done for the v2.

@samvdb
Copy link
Contributor Author

@samvdb samvdb commented May 9, 2016

Sorry kevin,

I'm using this branch as is, works fine but dont have the time to write
tests for it.

Gr

@coudenysj
Copy link

@coudenysj coudenysj commented May 17, 2016

@samvdb Any help needed on this?

@jonny827
Copy link

@jonny827 jonny827 commented May 23, 2016

+1 for version 2. Need any help? I'm sure several of us could pitch in to help!

@samvdb
Copy link
Contributor Author

@samvdb samvdb commented May 23, 2016

@jonny827
Copy link

@jonny827 jonny827 commented May 23, 2016

@samvdb --- Can you please rebase your branch so that tests pass and resolve any conflicts? This will allow me to pull from your repo and try and write some tests(given my below experience level in this particular area) What from your perspective may be needed to allow other NOSQL databases hook into Api-Platform? Thank you for pushing to get this feature supported in the platform!

@coudenysj -- Can you please help with writing some tests? Secondly, thank you interest in helping bring MongoDB to the platform.

I was hoping DynamoDB would lead from the push towards ODM. I have never used a NOSQL DB. I do know there is great use cases for them that would blend with my project. So in short, I have never written a test; especially for a DB I have never used. Anyone have a good tutorial on MongoDB tests? I am not afraid to learn. Also, I was going to help with DOCS but realize the 2.x branch implementation pretty much would only need a few lines changed potentially for the docs to explain the support of ODM in addition to ORM which is probably best done by someone with clearer understanding of how those work.

To recap, if anyone has some resources for test writing for MongoDB or just in general please let me know. Secondly, if anyone find a place I fit in to progress this PR/the platform as a whole then please let me know!

samvdb and others added 10 commits Jul 5, 2016
- add symfony mongo test
- add mongo on travis & appveyor
- add symfony mongo test
- add mongo on travis & appveyor
@Simperfit
Copy link
Member

@Simperfit Simperfit commented Nov 8, 2016

This PR is updated with master.

@samvdb Can you please give me a test project / or a path in order to test this ?

@ppounder
Copy link

@ppounder ppounder commented Feb 23, 2017

Hi all,

Could anyone advise how close we are for this PR to be merged? I can see a few issues above, and possibly more so if it gets rebased. Where I work we are trying to set up using ApiPlatform and MongoDB so would be happy to try and test it in our environment.

We've already tried by running this, but it's not picking up our Document's so if anyone can point us in the right direction with a little help/documentation we can try and assist in getting this over the line.

Regards
Paul

@qneyrat
Copy link

@qneyrat qneyrat commented Mar 22, 2017

up, what's up?

@dunglas
Copy link
Member

@dunglas dunglas commented Mar 22, 2017

Still waiting for a stable version of Mongo ODM for PHP 7 using the new extension (I've not checked but I'm not aware of a new release).

@qneyrat
Copy link

@qneyrat qneyrat commented Mar 22, 2017

I use mongo-php-adapter in prod. It's compulsory at the moment. I am afraid that doctrine upgrade in a long time. This repo is dev by one core developer and it's stable and it's recommended to use odm with php7.

@ad3n
Copy link
Contributor

@ad3n ad3n commented Mar 27, 2017

I use MongoDB in PHP7 and as far as i use, i don't have any major issue about it

@ad3n
Copy link
Contributor

@ad3n ad3n commented Apr 10, 2017

Just because change the root class (MongoDb Extension) that making stack? I don't know about it, but i think, if api platform can support mongodb, it is a good forward... We waiting for a year to support it... That is so long, right?

@dunglas
Copy link
Member

@dunglas dunglas commented Apr 10, 2017

Ok to use the bridge if it is stable enough. But someone needs to finish this PR first (make Travis green and update the documentation).

@ad3n
Copy link
Contributor

@ad3n ad3n commented Apr 11, 2017

@samvdb : can you help us to finish this PR? we need your help...

@qneyrat
Copy link

@qneyrat qneyrat commented Apr 11, 2017

If necessary I am available

@samvdb
Copy link
Contributor Author

@samvdb samvdb commented Apr 11, 2017

I am no longer using this branch and have no time to spare atm. If anyone needs repo access i can grant it.

@soyuka
Copy link
Member

@soyuka soyuka commented Apr 11, 2017

Anyone can fork your branch and create a new PR based on it :). @qneyrat feel free to do so, we will be glad to help reviewing!

rubenrua added a commit to rubenrua/Notes that referenced this pull request Jun 15, 2017
@ihr-it-projekt
Copy link

@ihr-it-projekt ihr-it-projekt commented Jul 13, 2017

i have fork this branch and merged it with the current master. If someone will help to make the lights green, i will be glad. https://github.com/ihr-it-projekt/core

PR: #1248

@samvdb Can you explain what i should do that api-plattform is right configured to use mongo db?

@Zayon Zayon mentioned this pull request Jul 28, 2017
@soyuka
Copy link
Member

@soyuka soyuka commented Aug 28, 2017

Closing in favor of #1293 (continues this one).

@soyuka soyuka closed this Aug 28, 2017
leesiongchan pushed a commit to leesiongchan/api-platform-core that referenced this pull request Mar 6, 2018
[client] fixed the package name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.