joelittlejohn / embedmongo-maven-plugin

Maven plugin wrapper for the flapdoodle.de embedded MongoDB API
Apache License 2.0
88 stars 51 forks source link

MongoImport support #41

Closed padilo closed 9 years ago

padilo commented 9 years ago

Support to MongoImport.

This PR adds a import goal to import some files to MongoDB, also supports wait and skip features.

I used the MongoD in the plugin context to take the port and version.

Take a look at example4 and Readme to see how it works :)

joelittlejohn commented 9 years ago

It looks like this PR effectively competes with #40. Would that PR satisfy your needs? I'd prefer that we have one way of importing data rather than two.

I'd be interested to hear your thoughts on the pros and cons of each approach, and @pvardanega what do you think about the approach used here? Would it satisfy your needs?

I guess #40 allows arbitrary operations, and this PR allows more idiomatic data import (via mongoimport).

padilo commented 9 years ago

I think the #40 is more hackeable as it enables you to execute scripts which is very useful and allows you to do whatever you need.

If I'm not wrong, you can import data with #40 PR too, but not directly using mongoexport because mongoexport doesn't export as script (Am I wrong?)

I think it would be very nice too have this two features, but I think that given the possibility of having a goal only to do scripting instead of a parameter of start goal would be more useful as you can use my import goal before executing scripts and also the ability to intercalate scripts, this give you a lot of customization to use it for whatever purpose.

pvardanega commented 9 years ago

First, both solutions works.

What I like with current PR is that you can easily deploy production database to test a behaviour or something else. This is really usefull but only when you export all the database (with indexes, etc.).

The purpose of the #40 is to do a system like Rails' migrations. Each new version, you can add some data and/or some structure changes like adding/removing indexes. I might be wrong but I think that indexes are not exported when you use mongoexport on a specific collection because they are stored in a specific collection (system.indexes).

Also, using #40, you have to provide files with actions (create, update, upsert, etc.) and data whereas with this PR, developers will provide only data : the current state of the application. We can easily update #40 to avoid some files with a specific prefix and setting the database in a specific state. It is usefull when you want to know what changes had generated a bug.

Finally, MongoDB suggests not to use mongoexport with production database. Maybe using mongodump and mongorestore could be better (http://docs.mongodb.org/v2.6/reference/program/mongoexport/).

padilo commented 9 years ago

With Mongoimport you can export only data from a particular collection. System.indexes is just another collection that can be imported too.

Mongoexport and Mongoimport only works at collection level, you can't import or export an entire database (directly): As you say at the end, Mongoexport is not to use with a production database, you are absolutely right :+1: . But the advantage of using Mongoimport is that imports data in JSON format which is very useful, because you can modify your json import data or add more data in a very easy way. As the data to import are just plain text you can use your VCS to see differences between versions, I don't think a Mongorestore would be useful because works with BSON.

40 is a more generic funcionality that is very useful, because the limit of the solution provided by @pvardanega is practically the sky :). But Mongoimport is just a more friendly way to do imports, of course you can do the same with #40, but if you only need to import some data #41 should be the way to go.

BTW #40 is using eval, which seems that is deprecated in the future release of MongoDB 3.0: http://docs.mongodb.org/manual/reference/command/eval/#dbcmd.eval But It's quite strange, because in the MongoDB Java Driver isn't https://github.com/mongodb/mongo-java-driver/blob/r3.0.0-rc0/driver/src/main/com/mongodb/DB.java#L430

I think executing a script should be done passing just the script as an argument this way: http://docs.mongodb.org/manual/tutorial/write-scripts-for-the-mongo-shell/ And I think it could be done just using MongoShellExecutable from Flapdoodle Embedded MongoDB without the need of the MongoDB Java Client. Doing this way we can be sure it uses the same version as the Maven plugin is executing.

joelittlejohn commented 9 years ago

Doing this way we can be sure it uses the same version as the Maven plugin is executing.

Very good point, I like this.

So, I think we have a few things to do here:

padilo commented 9 years ago

I'm completely agree. I'll do those change soon :+1:

I think having this two features it will enhance this plugin a lot.

pvardanega commented 9 years ago

:+1: I'll update #40 with new requirements.

padilo commented 9 years ago

To have the common methods available I create AbstractEmbeddedMongoMojo. ImportEmbeddedMongoMojo is the only one using this at the moment, I think it will be nice to do it with other goals.

I needed to add annotations support of Maven plugins, because its necessary to have the parameters inherited from a parent class.

Removed algo the dependency to the Mongod instance of the start goal and I had to also remove the wait support with mongo-import because a Mongod instance is required to waitFor the Mongod process. I think this needs to be rethink.

@joelittlejohn and @pvardanega What do you think? Did I miss something?

Hope you like my Abstract approach. BTW I tried to not modify too many things, when this PR and #32 are ready to merge I think some integration work will need to be done, but I would prefer to create a issue just to do this part.

padilo commented 9 years ago

@joelittlejohn and @pvardanega

Did you have time to have a look? Do you want me to explain in detail the solution?

I would like to have this features in a release, and there is still things to do after this PR and #32 to have it well integrated :)

joelittlejohn commented 9 years ago

@padilo sorry for the delay, I'll take a look tonight. I was thinking it might be good for you and @pvardanega to work collaboratively on the same branch. It will give you a chance to get the code-sharing between the two mojos right without having to work in isolation.

pvardanega commented 9 years ago

Sorry guys, I don't have enough time those days to work on this. @padilo, maybe you can merge both features in a branch and refactor the code to make something cleaner, as @joelittlejohn suggested ?

padilo commented 9 years ago

Ok, I'll try to get it done in a couple of weeks if I can steal some time :)

joelittlejohn commented 9 years ago

@padilo I've added you as a collaborator and pulled your import-support branch into the main repo. Feel free to hack on anything you like there and we can merge into master when you're ready. Cheers

padilo commented 9 years ago

Recovered branch from @pvardanega and merged :) I'm refactoring things, I will mention you @joelittlejohn when I feel comfortable with it.

I'm open on any suggestion or anything you have in mind.

joelittlejohn commented 9 years ago

Great, I'll keep an eye on things :D On 16 May 2015 17:03, "Pablo Díaz-López" notifications@github.com wrote:

Recovered branch from @pvardanega https://github.com/pvardanega and merged :) I'm refactoring things, I will mention you @joelittlejohn https://github.com/joelittlejohn when I feel comfortable with it.

I'm open on any suggestion or anything you have in mind.

— Reply to this email directly or view it on GitHub https://github.com/joelittlejohn/embedmongo-maven-plugin/pull/41#issuecomment-102643797 .

padilo commented 9 years ago

Take a look, I think this can be merged :-)

joelittlejohn commented 9 years ago

Cool, thanks @padilo. Could you rename the init-data goal to mongo-scripts? I think it will make it a bit clearer how exactly this goal differs from the other.

joelittlejohn commented 9 years ago

Excellent work by the way. I'll merge this and we can tweak a few things before releasing if required.

joelittlejohn commented 9 years ago

Github seems to be having some issues tonight. I've merged this.

joelittlejohn commented 9 years ago

I think we just need an example that shows usage for the mongo-scripts goal in the README.md