Opened 16 years ago

Closed 13 years ago

Last modified 13 years ago

#6045 closed Bug (fixed)

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

Reported by: Dmitry Gorbunow Owned by: Rémy Hubscher
Component: Database layer (models, ORM) Version: dev
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 16 years ago.
The better version
patch.diff (1.3 KB ) - added by Gonzalo Saavedra 15 years ago.
related.py.patch (1.2 KB ) - added by Rémy Hubscher 13 years ago.
New patch for Django 1.2.3
test_for_6045_worksforme.diff (2.2 KB ) - added by Aymeric Augustin 13 years ago.
6045.solution_and_test.patch (2.6 KB ) - added by b.leskes@… 13 years ago.
solution + test

Download all attachments as: .zip

Change History (28)

comment:1 by Chris Beaven, 16 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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

by Dmitry Gorbunow, 16 years ago

Attachment: related.py.diff added

The better version

comment:2 by Dmitry Gorbunow, 16 years ago

Patch needs improvement: unset

comment:3 by Chris Beaven, 16 years ago

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.

in reply to:  3 comment:4 by Terry Ma <zjumty@…>, 16 years ago

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

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 by Karen Tracey, 15 years ago

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

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.

by Gonzalo Saavedra, 15 years ago

Attachment: patch.diff added

comment:8 by Rémy Hubscher, 15 years ago

Owner: changed from nobody to Rémy Hubscher
Status: newassigned

With the patch, it now works.
Thanks you

comment:9 by oliver.andrich@…, 15 years ago

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 by Thomas Schreiber, 15 years ago

Cc: Thomas Schreiber added

comment:11 by ramen, 14 years ago

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 by Rémy Hubscher, 13 years ago

Resolution: fixed
Status: assignedclosed

I can confirm that this bug reappeared.

Here is the patch

by Rémy Hubscher, 13 years ago

Attachment: related.py.patch added

New patch for Django 1.2.3

comment:13 by Rémy Hubscher, 13 years ago

Resolution: fixed
Status: closedreopened

comment:14 by Russell Keith-Magee, 13 years ago

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

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?

by Aymeric Augustin, 13 years ago

comment:16 by Gabriel Hurley, 13 years ago

Severity: Normal
Type: Bug

comment:17 by Jacob, 13 years ago

Easy pickings: set

comment:18 by b.leskes@…, 13 years ago

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.

by b.leskes@…, 13 years ago

solution + test

comment:19 by Aymeric Augustin, 13 years ago

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

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

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

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

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