Pymongo3 compatibility #946
Conversation
|
|
|
I think we should extract the PyMongo major version to a constant somewhere. |
|
Ref #948 |
| @@ -7,6 +7,10 @@ | |||
|
|
|||
|
|
|||
| DEFAULT_CONNECTION_NAME = 'default' | |||
| if pymongo.version_tuple[0] >= 3: | |||
| READ_PREFERENCE = ReadPreference.SECONDARY_PREFERRED | |||
There was a problem hiding this comment.
This should be PRIMARY by default.
SECONDARY_PREFERRED as a default with no other configuration will break many applications - as it introduces an inherent race conditions for replica set systems. Imagine adding a User to the database and then on the next request trying to read from a secondary to get that User - it might not have been replicated to the secondary node.
There was a problem hiding this comment.
Fully right ! Sorry, it was late yesterday when I finished...
|
Looks great @MRigal a couple of comments / observations to help get this into merged |
|
Sorry @thedrow I just saw the use reviewable comment :( |
| @@ -491,6 +509,9 @@ class BlogPost(Document): | |||
|
|
|||
| self.assertEqual(BlogPost.objects.count(), 10) | |||
| self.assertEqual(BlogPost.objects.hint().count(), 10) | |||
| # here we seem to have find a bug in PyMongo 3. | |||
There was a problem hiding this comment.
(GitHub comment in order to remove it from code)
Seems to be a PyMongo3 bug
The cursor first makes a SON out of the list of tuples
Then later reuses it and wonders why is it not a list of tuples
|
|
|
Thanks @rozza ! I'll push a new commit with some of your suggestions ! Concerning Point 1: Everything is up with the @thedrow ? We make a global constant IS_PYMONGO_3 in mongoengine/init.py Which I set to a boolean? Or a PYMONGO_MAJOR which I set to an integer? I would still ask for support for point 2, please checkout yourself and give me your opinion on the changes there #946 (diff) Try to comment the self._created check and see yourself, digging with pdb |
|
|
|
I think that |
|
There is no significant performance difference between True != False and 3 != 2 I think it should more be based on "human-readability". I still don't know what's best... Let's wait for one or two more comments |
|
(sorry by the bug introduced by a revert introduced to the wrong place) |
|
|
|
+1 on |
|
OK for IS_PYMONGO_3. I couldn't put it into mongoengine/init.py because of circular imports. I hope the current location is appropriate, else I can move it again easily |
|
|
|
mongoengine/queryset/transform.py, line 138 [r15] (raw file): We should probably just do this: elif key in mongo_query:
if key in mongo_query and isinstance(mongo_query[key], dict):
mongo_query[key].update(value)
# $maxDistance needs to come last - convert to SON
value_dict = mongo_query[key]
if ('$maxDistance' in value_dict and '$near' in value_dict):
value_son = SON()
if isinstance(value_dict['$near'], dict):
for k, v in value_dict.iteritems():
if k == '$maxDistance':
continue
value_son[k] = v
value_son['$near'] = SON(value_son['$near'])
value_son['$near']['$maxDistance'] = value_dict['$maxDistance']
else:
for k, v in value_dict.iteritems():
if k == '$maxDistance':
continue
value_son[k] = v
value_son['$maxDistance'] = value_dict['$maxDistance']
mongo_query[key] = value_sonThat seems to work on both MongoDB 2.4.x and 2.6.x (tested locally). However, other tests do fail with 2.4 so we should really start looking at adding different MongoDB version to Travis (and discuss when do we deprecate 2.4, etc.). Here is why it is failing in Python 3 and not in Python 2: When you call get_connection().max_wire_version it returns a Database object (since getitem on MongoClient returns a Database if the request name is not an attribute). Now how does Python 2 handle integer comparisons to classes? In [2]: class Test:
...: pass
...:
In [3]: Test() < 1
Out[3]: TrueLet's try that again with Python 3: In [1]: class Test:
...: pass
...:
In [2]: Test() < 1
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
<ipython-input-2-ec09b1796211> in <module>()
----> 1 Test() < 1
TypeError: unorderable types: Test() < int()So its a combination of PyMongo 3 and Python 3 behavior causing this problem. Since we can no longer check the wire version of the server with PyMongo, if we need to support version checking in the future, we should check the version from get_server_info() or some other method. You will also have to remove the Comments from the review on Reviewable.io |
|
Excellent @seglberg ! Fast and precise! I'll do my best with it, and also give you owner access to my repository, since we are working close together on many issues, it might be better to directly commit some things on the PR of the other ;) 👍 |
|
|
|
Halleluja! Finally green! 🎱 But the price for enabling Pymongo 3.0 and Dev: :-( |
|
I let any other maintainer have a latest eye on it and (eventually) merge it |
|
I will give it one more look and pull it down locally again to test things out. Even though this will extend the Travis times, if you notice, Travis is not so "continuous" lately. It will complete the first 4-8 jobs then sit and wait for 20 minutes before it starts anymore, the entire time, the total duration keeps going up. |
|
I reviewed it as well and it looks good |
|
Had it go through a soak test over the weekend and nothing failed. 👍 🍻 |
|
|
|
Not sure if is related. This PR breaks Flask-Mongoengine Pagination edit: That's probably because snapshot was deprecated |
|
Yep, it definitely is related, snapshot was removed in PyMongo3 and is not 2015-05-11 23:50 GMT+02:00 rat notifications@github.com:
|
|
Should I release? |
|
@thedrow I wouldn't. I think we should decide what should go into the next release, also how to name it, but at least integrate #980 . |
|
Yeh I think you're right. |
|
Yes, I think we should take 0.9.0 and create 1 additional commit on top of On Wed, May 13, 2015 at 10:50 AM, Omer Katz notifications@github.com
|
|
I am perfectly fine with it. A pity it wasn't done earlier, but better now
|
|
Lovely how pymongo < 3.0 was nuked from pypi. Suggestions now? |
|
As far as I can see, it is still on PyPI: https://pypi.python.org/pypi/pymongo/2.8 I even tested with pip and it still works just fine. |
|
For what it's worth, I'd really like to see a release with this change. An 0.9.1 wouldn't help me as I've got a system which has been ported to Pymongo3 (and without compatibility shims). I saw mention of wanting to incorporate other unrelated changes. My preference would be for releases to be made often and whenever the codebase can be made stable. I see 15 open tickets on the milestone. I hate to think that all of those have to be addressed before a release can be made to address this issue. |
Hi guys,
Now everything is fixed and is fully compatible for both PyMongo 2.X and 3.X
I'm awaiting some feedback for:
The way I introduced differential behaviour based on PyMongo versionThe behaviour for save() when a document is created with a given primary key, source of 5 of remaining issues. Here particularly, I have removed the check for self._created since we used to modify this why changing/switching db on the fly. This whole behaviour has changed in PyMongo3 and IMHO doesn't make sense anymore.Your opinion if the test_hint failure could point to a bug in PyMongo3A hand on the other error on test_geo_indexes_recursionA problem with binary field used as primary index that was always there and only discovered now by a better test that became mandatory by the use of a work-around concerning a PyMongo 3.0 bug already fixed in the upcoming 3.0.1Cheers,
Matthieu