Document and fix connection code

[wojcikstefan]

Reverting/ fixing changes from #231. Relevant criticism: #231 (comment).

The main goal of this PR is to fix a buggy connection code introduced in v0.8. It also makes the code cleaner and easier to understand through a better separation of responsibilities and a better documentation.

This is unfortunately dropping support for mongomock, but many things were broken about it to begin with. We should revisit adding it in the future, though personally I don't see a point for it... In the age of Travis/CircleCI/etc., it's so easy to run a real MongoDB in your tests.

Things to test manually:

• Connecting to multiple mongos in a sharded cluster
• Connecting to a replica set
• Connecting to a server that requires authentication
• All of the above via a MongoDB URI

Fixes #266 #283 #282 #259 #258 #254 #250 #247 #267

[wojcikstefan]

@lafrech @thedrow lmk what you think + anything you'd add to "Things to test manually" (in the PR's description)?

[lafrech]

LGTM. I did a quick "review". Looked fine. At this point, we can't break it more than it already is, and the best way to get feedback could be to merge into master, maybe even release as 0.8.1, and ping reporters of all related bugs.

This is unfortunately dropping support for mongomock, but many things were broken about it to begin with. We should revisit adding it in the future, though personally I don't see a point for it... In the age of Travis/CircleCI/etc., it's so easy to run a real MongoDB in your tests.

Totally agree. Why not mongomock if it comes at no cost, but not a priority.

Great job. Thanks @wojcikstefan.

[wojcikstefan]

Thanks @lafrech !

Why not mongomock if it comes at no cost, but not a priority.

IMHO every line of code comes at a maintenance cost and if it can be avoided without much of a loss, then it should. Mongomock support is even more baffling, because making one's tests rely on a mocked version of a real database means that your project will always depend on that package staying up-to-date as the real database evolves (which can either mean you'll have to wait longer w/ a mongo upgrade or, worst case, your test will pass and your production environment will break).

[lafrech]

Sure. I don't use it myself. I just run a MongoDB instance on my dev machine, and cleanup the DB in fixtures before each test.

mongomock says that this

is obviously the best approach here, since we are testing against a real mongodb instance. However, a mongodb instance needs to be set up for this, and cleaned before/after the test. You might want to run your tests in continuous integration servers, on your laptop, or other bizarre platforms - which makes the mongodb requirement a liability.

I guess @losintikfos finds an interest in this. But I agree it sounds like very specific use cases.

[wojcikstefan]

I'd love to hear more from people who find an interest in this (including @losintikfos). The fact that MongoDB needs to be cleaned before/after each test is pretty irrelevant given how easy unit testing frameworks make it.

(...) You might want to run your tests in continuous integration servers, on your laptop, or other bizarre platforms (...)

First, we shouldn't optimize for "bizarre platforms" - there's too much of a cost with connection management to justify some obscure use cases. Second, modern CI servers all support MongoDB:

• Jenkins: https://wiki.jenkins-ci.org/display/JENKINS/MongoDB+Plugin
• Travis: https://docs.travis-ci.com/user/database-setup/#MongoDB
• Circle: https://circleci.com/docs/configuration/#services

Anyway, we should probably take that discussion away from this PR and into a dedicated issue. I'll test the remaining scenarios from this PRs description and then merge/tweak the code.

[lafrech]

Looks like this breaks the tests.

https://travis-ci.org/MongoEngine/flask-mongoengine/jobs/178375073#L529

[wojcikstefan]

Yup, fixed in the latest commit. Thanks.

[wojcikstefan]

test_host_as_list won't pass until a new mongoengine version is released (due to MongoEngine/mongoengine#1389)

[wojcikstefan]

Other than the test issue, I believe this is ready to go. Wanna give it one last glance before merging & releasing @lafrech ?

[allanlei]

Instead of deconstructing a connection string and passing the parts to mongoengine.connect(), can flask-mongoengine construct a connection string from the parts and pass to mongoengine.connect()?

I find it quite difficult to follow which library is parsing what. pymongo already does parsing of the host connection string, wouldn't it be a good idea to delegate the parsing to it?

It would also have the added benefit of seeing the connection string and just trying it out with the mongo CLI.

My original issue MongoEngine/mongoengine/issues/851 which was simply resolved by only using connection string URIs.

[lafrech]

test_host_as_list won't pass until a new mongoengine version is released (due to MongoEngine/mongoengine#1389)

@wojcikstefan, I relaunched the tests with new MongoEngine version out and they passed (except for pypy3, of course).

[lafrech]

Thanks @allanlei for your inputs. It sounds like an important rework, so considering flask-mongoengine has been broken for a while now, and this branch fixes numerous issues, I think we should release. It can always be reworked afterwards according to your proposal (and your help on this would be welcome but before coding anything, please open an issue to discuss the implementation, as it would be frustrating to get your code rejected for a design disagreement).

@wojcikstefan, I think we can merge/release. Do you want to do the manual tests before we proceed?

[wojcikstefan]

Thank you @allanlei, that's definitely worth looking into. I'm also not a fan of how much abstraction and logic both Flask-MongoEngine AND MongoEngine add on top of PyMongo, but I don't have a clear vision of the fix in my mind yet. We should discuss further in a separate issue (can you open one?).

Agreed @lafrech, this is ready to merge. I performed all the tests I mentioned in the description of this PR already. Can you merge and release? I'll be AFK for the next hour or so.

[lafrech]

Merged. I'm in the middle of something, so I don't release right now. Need to double-check changelog and bump version. I don't want to do fast and miss something.