Opened 9 years ago

Closed 5 years ago

Last modified 5 years ago

#6045 closed Bug (fixed)

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

Reported by: Dmitry Gorbunow Owned by: Natim
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: many2many, related, unicode easy-pickings
Cc: Gonzalo Saavedra, Thomas Schreiber 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 Dmitry Gorbunow 9 years ago.
The better version
patch.diff (1.3 KB) - added by Gonzalo Saavedra 8 years ago.
related.py.patch (1.2 KB) - added by Natim 6 years ago.
New patch for Django 1.2.3
test_for_6045_worksforme.diff (2.2 KB) - added by Aymeric Augustin 6 years ago.
6045.solution_and_test.patch (2.6 KB) - added by b.leskes@… 6 years ago.
solution + test

Download all attachments as: .zip

Change History (28)

comment:1 Changed 9 years ago by Chris Beaven

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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

Changed 9 years ago by Dmitry Gorbunow

Attachment: related.py.diff added

The better version

comment:2 Changed 9 years ago by Dmitry Gorbunow

Patch needs improvement: unset

comment:3 Changed 9 years ago by Chris Beaven

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 8 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 8 years ago by joaoolavo

Has patch: unset
Needs tests: unset
Patch needs improvement: unset
Version: SVN1.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 8 years ago by Karen Tracey

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 8 years ago by Gonzalo Saavedra

Cc: Gonzalo Saavedra added
Version: 1.0SVN

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 8 years ago by Gonzalo Saavedra

Attachment: patch.diff added

comment:8 Changed 8 years ago by Natim

Owner: changed from nobody to Natim
Status: newassigned

With the patch, it now works.
Thanks you

comment:9 Changed 8 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 7 years ago by Thomas Schreiber

Cc: Thomas Schreiber added

comment:11 Changed 6 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 6 years ago by Natim

Resolution: fixed
Status: assignedclosed

I can confirm that this bug reappeared.

Here is the patch

Changed 6 years ago by Natim

Attachment: related.py.patch added

New patch for Django 1.2.3

comment:13 Changed 6 years ago by Natim

Resolution: fixed
Status: closedreopened

comment:14 Changed 6 years ago by Russell Keith-Magee

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 6 years ago by Aymeric Augustin

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 6 years ago by Aymeric Augustin

comment:16 Changed 6 years ago by Gabriel Hurley

Severity: Normal
Type: Bug

comment:17 Changed 6 years ago by Jacob

Easy pickings: set

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

Needs tests: unset
Triage Stage: AcceptedReady 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 6 years ago by b.leskes@…

solution + test

comment:19 Changed 6 years ago by Aymeric Augustin

Triage Stage: Ready for checkinAccepted

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 6 years ago by Marc Egli

Triage Stage: AcceptedReady 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 5 years ago by Malcolm Tredinnick

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 5 years ago by Malcolm Tredinnick

Resolution: fixed
Status: reopenedclosed

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 5 years ago by Malcolm Tredinnick

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