Opened 19 years ago

Closed 19 years ago

Last modified 18 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: no UI/UX: no

Description

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.

Attachments (2)

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

Download all attachments as: .zip

Change History (9)

by limodou@…, 19 years ago

Attachment: manipulators.patch added

comment:1 by Luke Plant, 19 years ago

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

comment:2 by limodou@…, 19 years ago

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.

by Tom Tobin <korpios@…>, 19 years ago

fixes save method of AutomaticManipulator

comment:3 by Tom Tobin <korpios@…>, 19 years ago

Summary: [Patch]Fixed models/manipulator.py '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 by Luke Plant, 19 years ago

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 by Luke Plant, 19 years ago

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 by limodou@…, 19 years ago

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.

comment:7 by Adrian Holovaty, 19 years ago

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