Clarify add(), create(), remove() on ManyRelatedObjectsDescriptor for ForeignKeys

[jdunck@…]

---------- Forwarded message ----------
From: Russell Keith-Magee <freakboy3742@gmail.com>

Date: Feb 26, 2006 9:30 PM
....
Hi all,

In working on an implementation for the remove() and clear() methods
for ManyRelatedObjectsDescriptor (i.e., mypolls.choices.remove() and
mypoll.choices.clear()),  I have noticed that add() on a o2m
descriptor creates a new Choice object, whereas add() on a m2m
descriptor takes a list of pre-existing objects and adds them to the
membership set.

I have had a trawl through the wiki and mailing list logs, but I
didn't find any discussion covering this issue - it looks like the
inconsistency is an accident of porting the old API to the new. o2m
field behavior follows the old add_choices() behaviour; add/remove is
a new feature for m2m, replacing the set_articles() call.

To me, the difference in add() behaviours is a potentially confusing
inconsistency. In addition, the m2m interpretation of add() makes more
sense (to me, anyway) than the o2m behaviour. To that end, I would
like to propose the following:

1) Delete Manager.add(). Poll.objects.add(kwargs) doesn't strike me as
a particularly intuitive alternative to p = Poll(kwargs); p.save() -
the only reason it seems to be there is to simplify the o2m
descriptor.

2) Rename the existing mypoll.choices.add() to mypoll.choices.create()

3) Add a mypoll.choices.add() that will take a list of already
constructed objects (to match the m2m add), rather than create a
single new object. This would make mypoll.choices.add(choice) an
analog of "choice.poll_id = mypoll.id; choice.save()"

4) Delete remove() and clear() from the o2m descriptor.

I'm not entirely happy with (4) because of the lack of symmetry
between o2m and m2m descriptors, but the only alternatives I can see
are:

- have remove(*objs) delete all the named objects, and clear() delete
all objects with the matching key. This seems potentially confusing,
since the m2m remove/clear doesn't delete anything but the
relationship. Also, there is the potential for error if an object
provided to remove() isn't part of the o2m set.

- Clear/null the ForeignKey field for all objects in the removed set.
This would only work for objects where the field has blank=True set,
so blank=False fields would have to throw an error (or not have the
method available). This also strikes me as confusing.

- rename add() for o2m descriptors to something else, to remove the
implication that there should be a corresponding remove() and clear()
method. However, I can't think of a reasonable alternative.

[jdunck@…]

From: Adrian Holovaty <holovaty@…>

Date: Feb 26, 2006 11:27 PM
...

This sounds great to me. Go for it!

Adrian

[jdunck@…]

From: Luke Plant <luke.plant@…>

Date: Feb 27, 2006 3:18 AM

Russell Keith-Magee wrote:

• rename add() for o2m descriptors to something else, to remove the

implication that there should be a corresponding remove() and clear()
method. However, I can't think of a reasonable alternative.

I like your suggestions, but like you I'm not entirely convinced about
'create()'. I don't think 'add()' necessarily implies you can do
'remove()'. I guess the problem is that the peculiarity of a DB vs
objects in that you can constrain a relationship to be non-null in a
DB, but it's pretty difficult to stop an attribute from being 'None' in
python. How hard would it be to add the 'remove()' method but only for
foreign key relationships that can be null?

[jdunck@…]

From: Russell Keith-Magee <freakboy3742@…>

Date: Feb 27, 2006 8:25 AM

On 2/27/06, Adrian Holovaty <holovaty@…> wrote:

This sounds great to me. Go for it!

Ok - I've just committed these changes (r2409).

As a result of this commit, there is one more unit test failing - in
the many_to_one test, accessing new_article2.reporter.id seems to be
getting a stale value from the cache when an article that is already a
member of one reporters article set is added to a different reporter.
If I change the test to check new_article2.reporter_id, the test
passes, and if I look at what is in the database, it is correct - it's
just the cache that is stale.

Am I missing something obvious here, or is this a larger problem with
the cache? I seem to recall a recent discussion about caching primary
key lookups...

[jdunck@…]

---

From: Russell Keith-Magee <freakboy3742@…>

Date: Feb 27, 2006 8:35 AM

On 2/27/06, Luke Plant <luke.plant@…> wrote:

How hard would it be to add the 'remove()' method but only for
foreign key relationships that can be null?

That should be possible - I'll see what I can do.

[Jacob]

(cleaned up formatting)