OrderedContainerItemRenamer assumes too much

Bug #98385 reported by Marius Gedminas
2
Affects Status Importance Assigned to Milestone
Zope 3
Won't Fix
Medium
Marius Gedminas
zope.copypastemove
Fix Committed
Medium
Justin Alan Ryan

Bug Description

Imagine that you have an ordered container with a custom name chooser.

OrderedContainerItemRenamer(container).renameItem(oldName, newName) uses IObjectMover to rename the item, and then restores up the item order. IObjectMover uses INameChooser to validate the new name. The name chooser is free to choose a name different from newName passed to it. If this happens, IObjectMover returns the real new name back to the item remover, which ignores it and tries to call container.updateOrder with a list of IDs that includes newName.

OrderedContainerItemRenamer should not assume that IObjectMover(container).moveTo(container, newName) will always return newName unchanged.

I would like to fix this bug, but it raises an API question. Is IContainerItemRenamer.renameItem(oldName, newName) allowed to rename the item at oldName to a name that is different from newName? If so, wouldn't it make sense for renameItem to return the real new name?

Options:

  1. renameItem should raise an error if the name chooser overrode newName to something else

  2. renameItem should cope with override new names and return the real new name

  3. renameItem should cope with override new names, but keep the old API (no return value) for backwards-compatibility raisins

I prefer option (2).

Related branches

Revision history for this message
Marius Gedminas (mgedmin) wrote :

Supporters added: mgedmin; removed: chrism

Uploaded: fix-ordered-container-item-renamer.diff

I'm attaching a patch that implements option 3 (because it seemed to be the safest one).

Tres Seaver (tseaver)
Changed in zope.copypastemove:
importance: Undecided → Medium
status: New → Confirmed
Changed in zope3:
status: Confirmed → Won't Fix
Changed in zope.copypastemove:
assignee: nobody → Marius Gedminas (mgedmin)
Tres Seaver (tseaver)
tags: added: bugday20100424
Revision history for this message
Justin Alan Ryan (justizin) wrote :

FWIW, I don't see how starting to return a value, when none was returned before, would create a compatibility problem.

I'm thusly in favor of #2, which is simple enough to just add a return of newName at the end.

Revision history for this message
Justin Alan Ryan (justizin) wrote :

Ah, looks like the patch implements #2, adding a return value and using it, which does not break compatibility with the old API. Without a custom name chooser, an exception appears as if it would be raised.

+1 to that.

Changed in zope.copypastemove:
assignee: Marius Gedminas (mgedmin) → Justin Ryan (justizin)
tags: added: bugday
Tres Seaver (tseaver)
Changed in zope.copypastemove:
status: Confirmed → Fix Committed
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.