Implementation of distinct() #120

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

dconlon commented Mar 26, 2012

Hi All,

I saw that distinct() was added (efcebec) but did not have cause to use it until today.

It seemed possible to chain distinct() with filter():

from django_mongodb_engine.contrib import MongoDBManager

class FooModel(models.Model):
    ...
    objects = MongoDBManager()

FooModel.objects.filter(filters).distinct(fields)

in that code of the above form executes without error.

However the filters are ignored as distinct() is called on the pymongo Collection, not on the pymongo Cursor from the query result.

I'm not sure what the intention was with distinct() but I lost several hours to this today and wondered whether it should either be amended to work with filter() or raise an exception when called on a QuerySet. Either could help future hapless coders like me.

I've had a go at making it work with filter() and welcome your feedback.

Dan

Member

charettes commented Mar 26, 2012

Hi @dconlon, thanks for the bug report and the provided pull request.

I'm surprised this doesn't work as expected, I agree this is a bug. Is it possible for you to provide a patch against develop branch instead of master? We use the latter for stable release thus all bug corrections and new feature should be merged into develop first.

Contributor

jonashaag commented Mar 26, 2012

Edit: Bullshit. See my next post.

Oh gosh. That bug would've been found by any static analysis tool. The issue is that we're not passing query to distinct as first parameter.

charettes closed this Mar 26, 2012

Member

charettes commented Mar 26, 2012

@dconlon I'll close this pull request since it's against master anyway. Do you mind opening a new one against develop branch with your test case and @jonashaag's suggested fix?

Contributor

jonashaag commented Mar 26, 2012

Wait that's nonsense. We actually use query in the next line. I totally missed that. So the original fix looks right. It just needs to be opened against develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment