Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

andialbrecht commented Jan 11, 2012

Hi,

I've had some troubles when using a TranslationQueryset in a ModelChoiceField. When the field was rendered (a combobox) Django calls queryset.get() to retrieve the selected value and somehow this call caused the TranslationQueryset to forget about it's current language.

Sorry about this not very specific description. But currently I can't figure out the real problem behind this.

It would be nice if someone could have a look at this pull request. Cloning the TranslationQueryset instance in TranslationQueryset.get() solved the issue for me. Maybe someone with a better knowledge of the nani code can point me in the right direction.

Thanks!

Andi

@ojii ojii commented on the diff Jan 11, 2012

nani/manager.py
@@ -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()
@ojii

ojii Jan 11, 2012

Collaborator

with the changes further down, is this really needed?

@andialbrecht

andialbrecht Jan 11, 2012

Contributor

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 :)

@ojii

ojii Jan 11, 2012

Collaborator

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

@andialbrecht

andialbrecht Jan 11, 2012

Contributor

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

@andialbrecht

andialbrecht Jan 12, 2012

Contributor

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?

@ojii

ojii Jan 12, 2012

Collaborator

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...

@ojii

ojii Jan 13, 2012

Collaborator

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

Sorry for not taking too much part in the discussion here, I have been ill for a few days. What django version does this happen on?

Contributor

andialbrecht commented Jan 13, 2012

This happens with 1.4alpha.

BTW I share Jonas' opinion that the real issue is somewhere further
down - but I've no clue where to start looking.

On Fri, Jan 13, 2012 at 1:11 PM, Kristian Øllegaard
reply@reply.github.com
wrote:

Sorry for not taking too much part in the discussion here, I have been ill for a few days. What django version does this happen on?


Reply to this email directly or view it on GitHub:
#11 (comment)

Collaborator

ojii commented Jan 13, 2012

My initial attempt at a fix seems to solve some issues if i play around in the shell, but tests cannot verify that yet:

    diff --git a/nani/manager.py b/nani/manager.py
    index aa35c0e..6ea561f 100644
    --- a/nani/manager.py
    +++ b/nani/manager.py
    @@ -238,7 +238,17 @@ class TranslationQueryset(QuerySet):
             Therefore the check for _language_code must be done here.
             """
             if not self._language_code:
    -            return self.language().__getitem__(k)
    +            """
    +            Do NOT call language(). Because language calls filter which calls
    +            _filter_or_exclude which calls _clone. __getitem__ should NOT clone
    +            the query. Therefore we emulate _filter_or_exclude by adding a Q
    +            object to the query of THIS queryset (instead of cloning the
    +            queryset, and adding the Q object to the cloned queryset, which is
    +            what happens in language()).
    +            """
    +            #return self.language().__getitem__(k)
    +            self._language_code = get_language()
    +            self.query.add_q(Q(language_code=self._language_code))
             return super(TranslationQueryset, self).__getitem__(k)

         def create(self, **kwargs):
Collaborator

ojii commented Jan 13, 2012

also, calling len(...) before doing anything else seems to "fix" it. (Obviously not a real fix, but it does something that makes subsequent calls to methods on that queryset work)

Contributor

andialbrecht commented Jan 13, 2012

On Fri, Jan 13, 2012 at 2:21 PM, Jonas Obrist
reply@reply.github.com
wrote:

also, calling len(...) before doing anything else seems to "fix" it. (Obviously not a real fix, but it does something that makes subsequent calls to methods on that queryset work)

Right, that's what I've seen in the test case I've posted above.


Reply to this email directly or view it on GitHub:
#11 (comment)

The most weird thing is that in 449862a its qs.count() that fails - seems the problem might be even bigger.

Unless someone wants to pick up from this, I'm gonna close this pull request as its far behind current master.

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