Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#1334 closed defect (wontfix)

[magic-removal][patch] `save` method of `AutomaticManipulator` broken for `ManyToManyField`s

Reported by: limodou@… Owned by: Adrian Holovaty
Component: Core (Other) Version: magic-removal
Severity: normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


In models/'s save() function, there is a bug that using the old-style manipulator method set_XXX, so as saving manytomany field, just like save group object in Admin, it'll be failed. So I made this patch.

And I also found that in fields/,

rowcount = cursor.execute("SELECT %s FROM %s WHERE %s = %%s AND %s IN (%s)" % \

(rel_col_name, join_table, this_col_name,
rel_col_name, ",".join(%s? * len(new_ids))),
[this_pk_val] + list(new_ids))

existing_ids = set([row[0] for row in cursor.fetchmany(rowcount)])

if there is no records after executing the SELECT, the rowcount will be None, so cursor.fetchmany(rowcount) will be failed. The patch also including fix this.

Attachments (2)

manipulators.patch (1.4 KB) - added by limodou@… 11 years ago.
magic_removal-manytomany_save_fix.diff (2.4 KB) - added by Tom Tobin <korpios@…> 11 years ago.
fixes save method of AutomaticManipulator

Download all attachments as: .zip

Change History (9)

Changed 11 years ago by limodou@…

Attachment: manipulators.patch added

comment:1 Changed 11 years ago by Luke Plant

priority: highestnormal

Thanks for the report.

For the first part, the patch won't work correctly: the return value of add() is currently undefined, and even if we decide it should be 'true if objects were added', the clear() call means that 'was_changed' will have false positives. I posted to django-developers about this:

For the second part, cursor.fetchmany() doesn't throw an error, it just returns an empty tuple, which is fine, so there is no need to change it. (This code is covered by tests as well).

comment:2 Changed 11 years ago by limodou@…

I'm using sqlite3 for db backend, and if I try to print cursor.fetchmany(rowcount), I'll got and exception about:

Exception Type:  	TypeError
Exception Value: 	an integer is required
Exception Location: 	c:\python24\lib\site-packages\django-0.91-py2.4.egg\django\db\models\fields\ in _add_m2m_items, line 131

So maybe the bug is concerned to sqlite3, I guess.

Changed 11 years ago by Tom Tobin <korpios@…>

fixes save method of AutomaticManipulator

comment:3 Changed 11 years ago by Tom Tobin <korpios@…>

Summary: [Patch]Fixed models/ 'set_%s' bug[magic-removal][patch] `save` method of `AutomaticManipulator` broken for `ManyToManyField`s

I worked up an improved patch for the issues in this ticket; updating many-to-many objects seems to work now.

Also, I've run into a similar problem regarding the cursor.fetchmany(rowcount) call, where rowcount was None, yet I'm running PostgreSQL. I included the fix for this in my patch as well.

comment:4 Changed 11 years ago by Luke Plant

OK, so it looks like the behaviour of fetchmany() varies from db to db, or perhaps it's cursor.execute(). With mysql, it returns 0, not 'None' if there are no records.

comment:5 Changed 11 years ago by Luke Plant

Thanks for the patch Tom. Does it work for non-integer primary keys? It looks like it assumes that they will be ints, which isn't necessarily the case.

comment:6 Changed 11 years ago by limodou@…

I found something in PEP 0249, it says that:

            This read-only attribute specifies the number of rows that
            the last executeXXX() produced (for DQL statements like
            'select') or affected (for DML statements like 'update' or
            The attribute is -1 in case no executeXXX() has been
            performed on the cursor or the rowcount of the last
            operation is not determinable by the interface. [7]

            Note: Future versions of the DB API specification could
            redefine the latter case to have the object return None
            instead of -1.

And I also found that in execute() description it says that:

Return values are not defined.

Maybe using return value of execute() is not a good idea.

comment:7 Changed 11 years ago by Adrian Holovaty

Resolution: wontfix
Status: newclosed

I don't think it's worth the time to integrate this patch, because automatic manipulators are going away.

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