Skip to content

Allow for mongomock:// URI #303

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

Merged
merged 1 commit into from
May 14, 2017
Merged

Conversation

alysivji
Copy link
Contributor

@alysivji alysivji commented Mar 4, 2017

I was trying to implement unit tests using the mongomock library mentioned in the mongoengine docs and got the following error:

pymongo.errors.InvalidURI: Invalid URI scheme: URI must begin with 'mongodb://'

Mongomock URIs start with mongomock://. Mongoengine itself allows this format, we just need to make sure it can get through to the create_connections function.

I added a unit test and then fixed the issue. Also ran tests on travis-ci
https://travis-ci.org/alysivji/flask-mongoengine

Please let me know if you have any questions.

Thanks,
Aly

BTW. This is my first pull request to a Github project. I dug around online before submitting. So please excuse any errors in my workflow or description.

@touilleMan
Copy link
Member

Sorry to say that, but your PR is broken :'-(
The fix you've done only convert a mongomock:// url into a mongodb:// one, hence you never make use of mongomock !

I've created a new PR starting from your work to fix this 👍

@alysivji
Copy link
Contributor Author

alysivji commented Mar 14, 2017

I only convert it to a mongodb:// URI for that part of the code that requires a valid mongodb:// URI to pull out the database name (a call to PyMongo's parse_uri() function). If you follow the code, I do not change the actual host so the URI we pass into mongoengine is a mongomock:// URI.

I've been using my fixed version for unit testing with mongomock. I don't have an instance of MongoDB running and all my tests are passing.

The other fix is to not use the PyMongo function to get the database name. I'm sure there are better ways of doing it, but my fix also works.

Thanks for your help in getting this issue fixed! Really appreciate it!

@wojcikstefan
Copy link
Member

@alysivji @touilleMan sorry for the hold up, I'll look at both PRs soon!

@touilleMan
Copy link
Member

touilleMan commented Mar 14, 2017

@alysivji My bad, I read too fast your PR !

I've corrected my PR to keep your way of doing (it's better for the user to see the url as he defined instead of having it prefix changed)

(My PR is still valid given it also aims at fixing the unittests ^^)

@touilleMan touilleMan merged commit d4318e9 into MongoEngine:master May 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants