Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#1407 closed enhancement (fixed)

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

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

Description (last modified by jacob)

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

Change History (10)

comment:1 Changed 9 years ago by jdunck@…

  • Owner changed from Russell Keith-Magee to anonymous
  • Status changed from new to assigned

From: Adrian Holovaty <holovaty@…>

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

This sounds great to me. Go for it!

Adrian

comment:2 Changed 9 years ago by jdunck@…

  • Owner changed from anonymous to Russell
  • Status changed from assigned to new

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?

comment:3 Changed 9 years ago by jdunck@…

  • Owner changed from Russell to anonymous
  • Status changed from new to assigned

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

comment:4 Changed 9 years ago by jdunck@…

  • Owner changed from anonymous to Russell
  • Status changed from assigned to new

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.

comment:5 Changed 9 years ago by jacob

  • Description modified (diff)

(cleaned up formatting)

comment:6 Changed 9 years ago by russellm

  • Owner changed from Russell to russellm

comment:7 Changed 9 years ago by russellm

(In [2434]) magic-removal: Refs #1407 -- Added remove() and clear() methods for ForeignKeys that allow nulls.

comment:8 Changed 9 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

(In [2435]) magic-removal: Fixed #1407 -- Added a set method for the single object descriptor. This enables setting related objects using poll.choice = c, and ensures that the cache is kept accurate in the process.

comment:9 Changed 9 years ago by russellm

(In [2436]) magic-removal: Refs #1407 -- Modified descriptor set method to ask forgiveness rather than permission.

comment:10 Changed 9 years ago by adrian

  • milestone Version 0.92 deleted

Milestone Version 0.92 deleted

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