Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Work on cloned query in TranslationQueryset.get(). #11

Closed
wants to merge 2 commits into from
Closed

Work on cloned query in TranslationQueryset.get(). #11

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27f1c15
Work on cloned query in TranslationQueryset.get().
andialbrecht Jan 11, 2012
a51e2e8
Merge remote-tracking branch 'upstream/master'
andialbrecht Jan 19, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions nani/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,12 +262,12 @@ def get(self, *args, **kwargs):
# Enforce 'select related' onto 'master'
# Get the translated instance
found = False
qs = self
qs = self._clone()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the changes further down, is this really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm seeing the wrong behavior when reverting this line. Could it be that the language() method causes this? (Sorry, I'm really not familiar with the magic that happens here :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is very weird, as language calls filter which calls _filter_or_exclude which calls _clone. So the queryset should always be cloned anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, Jan 11, 2012 at 3:26 PM, Jonas Obrist
reply@reply.github.com
wrote:

@@ -262,12 +262,12 @@ def get(self, _args, *_kwargs):
         # Enforce 'select related' onto 'master'
         # Get the translated instance
         found = False

  •        qs = self
  •        qs = self._clone()

that is very weird, as language calls filter which calls _filter_or_exclude which calls _clone. So the queryset should always be cloned anyway.

I'll try to make a test to reproduce it.

Reply to this email directly or view it on GitHub:
https://github.com/KristianOellegaard/django-hvad/pull/11/files#r343599

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a test case that fails:

class Test(NaniTestCase):
    def test_get_doesnt_change_query(self):
        # Create the instances
        SHARED = 'shared'
        TRANS_EN = 'English'
        TRANS_JA = u'日本語'
        en = Normal.objects.language('en').create(
            shared_field=SHARED,
            translated_field=TRANS_EN,
        )
        ja = en
        ja.translate('ja')
        ja.translated_field = TRANS_JA
        ja.save()

        with LanguageOverride('en'):
            # using translations is needed to order on translated field
            query = Normal.objects.using_translations().all()
            query = query.order_by('translated_field').distinct()
            # calling .get() before iterating over query makes this
            # test fail. calling get after list(query) works fine.
            item = query.get(pk=en.pk)
            self.assertEqual(len(list(query)), 1)

This is what actually happens when such a query is used as a queryset for a ModelChoiceField. get() is called by the widget to find the selected entry.

Is it wrong API usage?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'm still convinced that there's a more serious subtle bug somewhere in there and _clone is just a workaround. but if it works...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay found one of the issues: __getitem__ clones the query. working on a fix



if 'language_code' in newkwargs:
language_code = newkwargs.pop('language_code')
qs = self.language(language_code)
qs = qs.language(language_code)
found = True
elif args:
language_code = None
Expand All @@ -278,7 +278,7 @@ def get(self, *args, **kwargs):
if language_code:
break
if language_code:
qs = self.language(language_code)
qs = qs.language(language_code)
found = True
else:
for where in qs.query.where.children:
Expand All @@ -290,7 +290,7 @@ def get(self, *args, **kwargs):
if found:
break
if not found:
qs = self.language()
qs = qs.language()
# self.iterator already combines! Isn't that nice?
return QuerySet.get(qs, *newargs, **newkwargs)

Expand Down