Opened 8 years ago

Closed 4 years ago

Last modified 4 years ago

#6045 closed Bug (fixed)

Many2Many manager and “TypeError: filter() keywords must be strings”

Reported by: proxor Owned by: Natim
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: many2many, related, unicode easy-pickings
Cc: gonz, rizumu Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

When using Many2Many fields with unicode data, it raises “TypeError: filter() keywords must be strings” on <model>.<m2m>.all(). The solution is same as in #4315.

Attachments (5)

related.py.diff (1.1 KB) - added by proxor 8 years ago.
The better version
patch.diff (1.3 KB) - added by gonz 7 years ago.
related.py.patch (1.2 KB) - added by Natim 5 years ago.
New patch for Django 1.2.3
test_for_6045_worksforme.diff (2.2 KB) - added by aaugustin 5 years ago.
6045.solution_and_test.patch (2.6 KB) - added by b.leskes@… 4 years ago.
solution + test

Download all attachments as: .zip

Change History (28)

comment:1 Changed 8 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

It's not quite identical to the fix there... if not isinstance(key, str):

Changed 8 years ago by proxor

The better version

comment:2 Changed 8 years ago by proxor

  • Patch needs improvement unset

comment:3 follow-up: Changed 8 years ago by SmileyChris

  • Keywords easy-pickings added
  • Needs tests set
  • Patch needs improvement set

Thanks for the new patch. Taking another look at this, I'm thinking that smart_str(value) isn't necessary (even though it got through in the other patch) -- only the keywords need to be strings.

Also, this may as well have some tests too.

comment:4 in reply to: ↑ 3 Changed 7 years ago by Terry Ma <zjumty@…>

Replying to SmileyChris:

Thanks for the new patch. Taking another look at this, I'm thinking that smart_str(value) isn't necessary (even though it got through in the other patch) -- only the keywords need to be strings.

Also, this may as well have some tests too.

It seems that the problem have not been fixed yet.
I checked out django today(2008/07/28), and found the problem still exist.

and I think the code

            for key, value in core_filters.items(): 
                if not isinstance(key, str): 
                    del core_filters[key]  
                    core_filters[smart_str(key)] = value

should be in the init() of the class ManyRelatedManager.

when can this problem be fixed. I am using django in my project now.

comment:5 Changed 7 years ago by joaoolavo

  • Has patch unset
  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from SVN to 1.0

This same problem stills happening in version 1.0. If I use an Unicode string in the parameter "related_name" in ManyToManyField, I got the prior error.

comment:6 Changed 7 years ago by kmtracey

  • Has patch set
  • Needs tests set

State of the ticket is still new, never fixed, so yes it's quite likely this problem still exists. Restoring prior flags since I don't see any reason they were deleted. The ticket has a patch but no test -- a test (one that fails with current code and passes with the fix) would be useful in moving this ticket along in the process, if anyone encountering this problem would care to provide one.

comment:7 Changed 7 years ago by gonz

  • Cc gonz added
  • Version changed from 1.0 to SVN

This bug exists in svn, so I'm changing version.

If you use a unicode string in a ManyToManyField realated_name attribute, manager's count(), all(), etc... will fail. Hope this get fixed soon but meanwhile you can workaround this by not using unicode strings on this specific atribute.

Tested the patch by proxor and its working but I'm attaching a new patch with Terry Ma and SmileyChris suggestions.

Changed 7 years ago by gonz

comment:8 Changed 7 years ago by Natim

  • Owner changed from nobody to Natim
  • Status changed from new to assigned

With the patch, it now works.
Thanks you

comment:9 Changed 6 years ago by oliver.andrich@…

Adding "from future import unicode_literals" to your models.py when using Python 2.6 also causes this problem. It's obvious, as afterwards all strings are unicode strings. But I wanted to point out the relationship to Python 2.6 in case someone else encounters such a problem.

comment:10 Changed 6 years ago by rizumu

  • Cc rizumu added

comment:11 Changed 5 years ago by ramen

There seems to have been a regression with Django 1.2, as working code started breaking after the update. If the first parameter ("to") to ManyToManyField constructor is a Unicode string, the admin is now throwing an exception on save:

Traceback (most recent call last):

  File "/usr/lib/pymodules/python2.5/django/core/handlers/base.py", line 100, in get_response
    response = callback(request, *callback_args, **callback_kwargs)

  File "/usr/lib/pymodules/python2.5/django/contrib/admin/options.py", line 239, in wrapper
    return self.admin_site.admin_view(view)(*args, **kwargs)

  File "/usr/lib/pymodules/python2.5/django/utils/decorators.py", line 76, in _wrapped_view
    response = view_func(request, *args, **kwargs)

  File "/usr/lib/pymodules/python2.5/django/views/decorators/cache.py", line 69, in _wrapped_view_func
    response = view_func(request, *args, **kwargs)

  File "/usr/lib/pymodules/python2.5/django/contrib/admin/sites.py", line 190, in inner
    return view(request, *args, **kwargs)

  File "/usr/lib/pymodules/python2.5/django/utils/decorators.py", line 21, in _wrapper
    return decorator(bound_func)(*args, **kwargs)

  File "/usr/lib/pymodules/python2.5/django/utils/decorators.py", line 76, in _wrapped_view
    response = view_func(request, *args, **kwargs)

  File "/usr/lib/pymodules/python2.5/django/utils/decorators.py", line 17, in bound_func
    return func(self, *args2, **kwargs2)

  File "/usr/lib/pymodules/python2.5/django/db/transaction.py", line 299, in _commit_on_success
    res = func(*args, **kw)

  File "/usr/lib/pymodules/python2.5/django/contrib/admin/options.py", line 896, in change_view
    form.save_m2m()

  File "/usr/lib/pymodules/python2.5/django/forms/models.py", line 83, in save_m2m
    f.save_form_data(instance, cleaned_data[f.name])

  File "/usr/lib/pymodules/python2.5/django/db/models/fields/related.py", line 1140, in save_form_data
    setattr(instance, self.attname, data)

  File "/usr/lib/pymodules/python2.5/django/db/models/fields/related.py", line 731, in __set__
    manager.add(*value)

  File "/usr/lib/pymodules/python2.5/django/db/models/fields/related.py", line 490, in add
    self._add_items(self.source_field_name, self.target_field_name, *objs)

  File "/usr/lib/pymodules/python2.5/django/db/models/fields/related.py", line 560, in _add_items
    '%s__in' % target_field_name: new_ids,

TypeError: filter() keywords must be strings

comment:12 Changed 5 years ago by Natim

  • Resolution set to fixed
  • Status changed from assigned to closed

I can confirm that this bug reappeared.

Here is the patch

Changed 5 years ago by Natim

New patch for Django 1.2.3

comment:13 Changed 5 years ago by Natim

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:14 Changed 5 years ago by russellm

Despite being three years old, this ticket is still missing the most important part -- a clear, reproducible test case. This is doubly important if you want a patch to be applied to trunk.

comment:15 Changed 5 years ago by aaugustin

I wrote a test but failed to reproduce the error. I use a unicode model name and a unicode related_name. Both "<object>.save" and "<object>.<m2m>.all()" succeed.

A simple app with this models works fine in the admin.

In short, I can not reproduce any of the problems described above with current trunk. I suggest closing the ticket as WORKSFORME (and maybe committing the regression tests since there might have been an issue at some point in the past).

Natim, could you modify my patch to demonstrate the error?

Changed 5 years ago by aaugustin

comment:16 Changed 4 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Bug

comment:17 Changed 4 years ago by jacob

  • Easy pickings set

comment:18 Changed 4 years ago by b.leskes@…

  • Needs tests unset
  • Triage Stage changed from Accepted to Ready for checkin
  • UI/UX unset

I've looked into and indeed the issue appears at trunk. I've added a patch with a test to demonstrate the problem mention by ramen and a solution. My solution is different than the previous one by that it converts the to param of M2M field to an ascii string. This encoding is the only one accepted by getattr.

Changed 4 years ago by b.leskes@…

solution + test

comment:19 Changed 4 years ago by aaugustin

  • Triage Stage changed from Ready for checkin to Accepted

You shouldn't mark your own patch as "ready for checkin". It must be reviewed by someone else (see the contributing guidelines).

comment:20 Changed 4 years ago by frog32

  • Triage Stage changed from Accepted to Ready for checkin

I run the unittest from b.leskes and reviewed the code. Test passes and the code seems to do the task.

comment:21 Changed 4 years ago by mtredinnick

In [16679]:

Added another test to confirm fix in r16663.

This is the test case from #6045, which was fixed by the above commit.
Refs #6045, #16299

comment:22 Changed 4 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from reopened to closed

This was effectively fixed by r16663, in a slightly different way. I added the test from the latest patch here as a safety net.

comment:23 Changed 4 years ago by mtredinnick

In [16682]:

Make ManyToManyField model references more robust.

In r16679 a test was added to verify something had been fixed when a
unicode string type was passed in as a model name. The name has to be
ASCII convertible, but in Python 2.6 and earlier, it must also have str
type.

This commit fixes the problem for earlier Python versions and is almost
identical to a patch from b.leskes in #6045.

Fixes #16689. Refs #6045.

Note: See TracTickets for help on using tickets.
Back to Top