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

[limodou@…]

In models/manipulator.py'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/related.py,

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.

[Luke Plant]

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: ​http://groups.google.com/group/django-developers/browse_frm/thread/701f1981d94e150f/5fc281a6452295b5?tvc=1#5fc281a6452295b5

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

[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\related.py in _add_m2m_items, line 131

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

[Tom Tobin <korpios@…>]

fixes save method of AutomaticManipulator

[Tom Tobin <korpios@…>]

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.

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

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

[limodou@…]

I found something in ​PEP 0249, it says that:

.rowcount 
          
            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
            'insert').
            
            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.

[Adrian Holovaty]

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