Implementation of 'get_version' #189
Conversation
| @@ -1,6 +1,18 @@ | |||
| Changelog | |||
| ========= | |||
|
|
|||
| Unreleased | |||
There was a problem hiding this comment.
We are changed NEWS.rst today by adding unreleased 16.3.0 version. Could you please merge current master branch of txmongo into your branch and resolve the merge conflict?
| index = 9 | ||
| while True: | ||
| try: | ||
| doc = yield self.gfs.get_last_version('world') |
There was a problem hiding this comment.
Please, don't mix tabs and spaces in indents :)
| @@ -138,6 +141,45 @@ def get(self, file_id): | |||
| defer.returnValue(GridOut(self.__collection, doc)) | |||
|
|
|||
| @defer.inlineCallbacks | |||
| def get_version(self, filename=None, version=-1, **kwargs): | |||
There was a problem hiding this comment.
If you have decided to omit custom filtering feature, lets drop **kwargs here too in order to not make an illusion that it will work as in PyMongo
|
|
||
| try: | ||
| defer.returnValue(GridOut(self.__collection, cursor[0])) | ||
| except StopIteration: |
There was a problem hiding this comment.
yield find() will return plain list, not cursor object like in PyMongo. So IndexError should be cought here, not StopIteration. Or better just checking:
if cursor:
defer.returnValue(...)
raise NoFile(...)
Merge from master # 2
Lines starting with '#' will be ignored, and an empty message aborts the commit.
|
Sorry, I've misguided you about the way to drop database. Currently, there is no any |
|
Ok, I was a little confused by the drop issue, looks ok now. Indeed the new version of the test code exercises the StopIteration fix .. :) .. how's it look? |
| yield self.gfs.delete(doc._id) | ||
| except NoFile: | ||
| break | ||
| try: |
There was a problem hiding this comment.
Do we need this try block? If we got out of while True loop, it certainly mean that yield self.gfs.get_last_version('world') raises the NoFile. Why re-check?
I guess you have added this block to check StopIteration fix, but it doesn't actually test it because it calls get_last_version, not get_version :)
There was a problem hiding this comment.
Sure, that was from me trying to figure out why my "drop" was failing (silently), but you're right, it's not needed. (removed)
| return | ||
|
|
||
| @defer.inlineCallbacks | ||
| def test_GFSVersion_last(self): |
There was a problem hiding this comment.
This test seems to be calling get_last_version instead of get_version(..., -1) too
There was a problem hiding this comment.
I do have 1 last "last" call left, call it "belt and braces", not strictly needed but nice to make sure get_last and get_version(-1) give the same result .. :)
There was a problem hiding this comment.
Ah, ok, I just didn't noticed that this test is actually duplicated: one calls get_version and another calls get_last_version
|
I've updated my review. Just a couple of minor comments about testing code :) |
|
Ok, comment should read ".. remove get_last_version" .. should look better now ... |
| @@ -62,6 +62,9 @@ def __init__(self, database, collection="fs"): | |||
| self.__indexes_created_defer = defer.DeferredList([ | |||
| self.__files.create_index( | |||
| filter.sort(ASCENDING("filesname") + ASCENDING("uploadDate"))), | |||
| self.__files.create_index( | |||
| filter.sort(ASCENDING("filesname") + DESCENDING("uploadDate"))), | |||
There was a problem hiding this comment.
After careful reading I've noticed that second index is redundant. {filename: 1, uploadDate: 1} index will cover both find({filename: ...}).sort({uploadDate: 1}) and find({filename: ...}).sort({uploadDate: -1}).
There was a problem hiding this comment.
Ok, in my defence I was just copying other people's code with regards to index creation, and if one of those indexes is obsolete, it would imply the current method for specifying index creation is also rather redundant. I've done a little reading too now ( :) ) so here's another shot with one index, with a slightly more direct specification, and also I modified the test so it runs under both Python2 "and" Python3.
There was a problem hiding this comment.
No-no, I didn't meant you are specifying index incorrectly. Specification was good, just ASC+DESC index was redundant. Please see my comment below. Sorry for confusion, I just used mongoshell's notion in my previous comment. It doesn't work well in Python.
|
The last comment, I beleive :) |
|
Nice catch about Py3 compatibility! But please, revert back index creation code: self.__files.create_index(
filter.sort(ASCENDING("filesname") + ASCENDING("uploadDate")))
Still you are correctly removed |
| @@ -60,7 +60,7 @@ def __init__(self, database, collection="fs"): | |||
| self.__files = self.__collection.files | |||
| self.__chunks = self.__collection.chunks | |||
| self.__indexes_created_defer = defer.DeferredList([ | |||
| self.__files.create_index( | |||
| self.__files.create_index( | |||
There was a problem hiding this comment.
You are probably already tired of my grumling, but could you please fix indenting of this line? ;)
There was a problem hiding this comment.
If I'm getting tired, I should have paid more attention to the indent before committing .. ;)
|
Good point, reverted .. that's still a horribly clunky way to define an index .. would be nice to support something a little more intuitive .. maybe [{'field1':1},{'field2':-1}] .. the way the Mongo guys implemented index spec's is a little unfortunate with regards to Python dict's ... :( |
|
Ok, turns out this would be a valid index spec; create_index([('filesname':1),('uploadDate':1)], name='version_index')I would submit this is 'clearer' in terms of what it's doing ... (?) |
|
You can specify indexes like this: I suggest you to leave it as it was, with all these Sorry, but it seems that your last commit actually broke indenting of another line instead of fixing :) Both indexes are created inside |
|
Ok, how's that? |
|
:) |
|
Let's wait for results of automated testing by Travis |
|
Thanks for your contribution! |
|
Yes, thank you very much and congratulations! :D |
additional index was removed after discussion in #189
This is based on the PyMongo call available in the v3.4 API and is pretty critical for anything that makes use of Mongo's versioning feature. I've added an additional index to ensure you can scan both-ways in the version list, and I've added a test-case to exercise the new code. The PyMongo implementation also allows random (potentially/probably non-indexed) attributes to be included in the search, I have deliberately omitted this feature as it's "likely" to introduce unpredictable performance issues if it's ever used. (although it anyone needs it, adding is relatively trivial) I don't necessarily "like" the indexing scheme, but it mirrors the PyMongo implementation for compatibility.