On Sat, Jan 9, 2010 at 9:47 AM, Xia Kai(夏恺) <xia...@gmail.com> wrote:
> Hi all,
>
> IMHO this patch would still make django issue illogical signals in admin.
> frans and I have pointed out that the __set__ method in the
> ManyRelatedObjectsDescriptor is flawed[1-2], which is used by admin.
It isn't as simple as saying that it is "flawed" - it just doesn't
work quite as you perhaps expected. It don't think it's especially
illogical, either - in order to assign a new set, you have to clear
the old set, which is exactly what the current implementation does.
I'm also not clear what this has to do with the admin. There is
nothing about ManyRelatedObjectsDescriptor that is admin-specific.
> The __set__ method would save a many-to-many relationship by clearing all
> entries in a field and then reinserting them back. This would work in the
> old days when no many-to-many signal is around. However, with the addition
> of this signal, all saving through admin would issue two signals: first, a
> signal with action 'clear', and then, a signal with action 'add'. Signal
> with remove action would never be correctly issued.
The approach you describe sends two signals as well. One indicating
the objects that have been removed, and one that indicates the objects
that have been added.
I don't accept your argument that a "remove" action is "correct" -
it's just a different way of describing the same change.
> frans and I had provided some patches to work this out. However, I'm afraid
> my last solution in [2] would incorrectly issue signal with 'clear' action.
> A more appriate solution would be:
>
> manager = self.__get__(instance)
> previous=set(manager.all())
> new=set(value)
> if previous and not new: # correction against [2] here.
> manager.clear()
> else:
> added = new - previous
> removed = previous - new
> if removed:
> manager.remove(*removed)
> if added:
> manager.add(*added)
Again, issuing a 'clear' isn't *incorrect*, it's just contrary to your
expectations.
Appropriate is a different argument. The approach you are advocating
does have one advantage - the database change implemented (and thus
the signals issued) will represent the delta of the change, rather
than a complete clear/re-add. This means that in the case of::
author.books = [b1,b2,...b100]
author.books = [b1,b2,...b100, b101]
your code will perform one insertion, whereas the existing code will
do 100 removals and 101 insertions. However, this is at the cost of
retrieving 100 items, constructing a set, and doing a set difference.
This retrieval and set difference is not a free operation - there is a
cost in both CPU and database load. Whether this is an net performance
gain is not immediately obvious, although I suspect there will be a
gain if the assignment set is large, but has a small delta. I'd need
to see some concrete performance numbers to convince me. Those
performance numbers would need to cover at least 4 cases:
* large assignment, small delta
* large assignment, large delta
* small assignment, small delta
* small assignment, large delta
The clear() problem isn't the only problem with the implementation you
propose. __set__ can accept a list of ids or a list of objects. Your
implementation requires that a list of objects has been assigned.
Ironically, if you provide a list of ids, the code still works - it
just reverts to what you refer to as "incorrect" behavior; that -
removing all the old objects, then adding new objects. However, it
does this by removing each object individually, not clearing the
objects as a whole.
In summary, the current behavior isn't wrong - it's just contrary to
your expectations. The change you are proposing is a feature request
that is independent of the desire to add m2m signals. The change you
propose may well be a performance improvement, especially for large
assignments with small deltas (I can think of a situation in code I
have in production where it would be a gain). The benefit for small
assignments will not be as obvious.
If you want to pursue this change:
* Open a new ticket
* Provide an implementation that fixes the problem I have identified
* Perform some performance numbers to convince me that this is a net gain.
Yours,
Russ Magee %-)
--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en.
