ObjectCopier passes the original rather than the copy to INamesChooser

Bug #98311 reported by Egon Frerich on 2006-03-03
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
zope.copypastemove
Critical
Justin Alan Ryan

Bug Description

In my application one of the contenttype datafields should be the
objectname in the container. With a subclass of NameChooser I managed to
add content objects into the container with an objectname which is
identical with a name in a datafield.

In the container view it is possible to select id(s) for copying and then to paste the selected objects into the same or into another container.

If the objectname of a pasted object is already in the container the UserError "The given name is already being used" is triggered. Therefore I programmed in the NameChooser:

    def chooseName(self, name, object):
        name = object.RaumName
        try:
            self.checkName(name, object)
        except UserError, e:
            if str(e) == "The given name is already being used":
                object.RaumName = u"Copy" + object.RaumName
                self.chooseName('', object)
                name = object.RaumName
                return name
        return name

So we get unambiguous objectnames for the container (maybe "CopyCopyObject").

And the datafield in the object (here: RaumName) is synchronized with the objectname.

Since the ObjectCopier doesn't create a real copy but references only the object the datafield is changed in two objects: in the *original* object and the copied object. And the content of datafield in the original object (something with "Copy") is different from the objectname (without "Copy").

Revision history for this message
Stephan Richter (srichter) wrote :

Changes: submitter email, importance (medium => critical)

Revision history for this message
Martijn Faassen (faassen) wrote :

Status: Pending => Rejected

I'm not sure I understand the report fully. The claim is that no copy is made by the ObjectCopier but references are made only. This is not true for the ObjectCopier - it does make copies (though in some cases it will make references if there is a reference in the object to an object in another location).

It's possible something special happens in case of the use of the object name. More details are needed here.

For the time being, I'm going to reject this issue, but please
give us a more detailed description about what's going on first.

Revision history for this message
Egon Frerich (e-frerich) wrote :

Uploaded: room.zip

I put together some minimal files so everyone can see what happens. Install the files in a instance and start Zope 3.

In ZMI choose Room Folder and give it a name. for example "rooms".
Then go to this container. Then you can only add objects from type "Room". For example you add the room "kitchen". If you look at this object you will get informations from the Object Introspector.

Now select the room "kitchen" in the container view and click the copy button. Then click the paste button. Afterwards the container view contains the kitchen object but also the object "Copykitchen".

If you click Copykitchen the Object Introspector shows the roomname as Copykitchen which is correct. But if you look at the roomname of kitchen then the roomname has changed to Copykitchen which is wrong.

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

I can confirm the symptoms.

However a simple look with bin/debugzope shows that there are two different Room objects in the database. They have different __name__ attributes, but the same roomname.

If you take a look at ObjectCopier.copyTo, you will see that it invokes the name chooser on the original object before performing the actual copy. Therefore your custom name chooser overwrites the attribute of the original object.

Someone who has achieved the Zen of Zope 3 should now Pronounce wheter it is OK for name choosers to modify the object. If so, then ObjectCopier is buggy and should be fixed. If not, then your name chooser is buggy.

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

Changes: edited transcript, revised title

Revision history for this message
Philipp von Weitershausen (philikon) wrote :

I think INameChoosers should not modify objects. They should just choose names and treat objects passively.

I also think that the copy/paste machinery should invoke the namechooser on the cloned object, not the original one.

So, Egon should consider a different strategy for choosing names and zope.copypastemove should be fixed. But that's just MHO.

Revision history for this message
Philipp von Weitershausen (philikon) wrote :

A bit more constructive criticism on Egon's INameChooser: all of its logic geared towards the case when objects are cloned. THe name chooser is the wrong place. As said, the namechooser should just get an object and decide its name.

What Egon really wants is a custom IObjectCopier adapter for rooms. This custom copier could modify the cloned room any way it wants.

We still need to fix zope.copypastemove. It would be great if Egon could provide a patch (incl. unittest). The unittest would involve:

1. a stub namechooser that "marks" objects somehow
2. a test of copying an objects and making sure that the original is not marked but the cloned one

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

I believe this is, via INameChooser, related to 98385 and am working on demo code to exploit both.

affects: zope3 → zope.copypastemove
Changed in zope.copypastemove:
assignee: nobody → Justin Alan Ryan (justizin)
status: Invalid → Incomplete
Revision history for this message
Justin Alan Ryan (justizin) wrote :

Also considering Philipp and Martijn's feedback here, of course.

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

I pushed a branch with a failing test in one revision, and what I believe is a fairly clear implementation of the aforementioned proposed and agreed upon change in behaviour. I would like to discuss this, as the test currently fails, among other concerns I'll express further..

Two major concerns:

  (a) Though it seems worthwhile to pass the newly copied object to INameChooser from ObjectCopier, rather than the original, the test has to 'mark' the object, a modification, which I agree shouldn't be the work done by INameChooser.

  (b) The test still fails with the proposed change included. My test assigns an arbitrary attribute, and this seems as if it should be fine, though not a recommended way of storing values, simply to mark this object. If I'm wrong, I apologize for the oversight. Perhaps directlyProvides for a marker would be a better way?

My general concern is, in order for this test to verify the behaviour is changed, it must modify the newly copied object in a name chooser, which is recommended against, and a test may suggest that it is 'supported' behaviour.

That said, I still believe it is sensible for this order of operations to change. No existing tests fail, but my test doesn't pass, and I'm not sure it's a test which, other than showing this change enacted a change, tests functionality which should be supported.

My greater question after working a bit with this problem is, other than past API compatibility, is there a compelling reason for a NameChooser to have a copy of either object? Its' job seems to be to check if the requested name exists in the current container, and to return an alternative if so, or to handle formatting issues like turning spaces into dashes or underscores.

I can see why, from a holistic perspective, it may have made sense early in Z3/ZTK design to include the object here, I just wonder if any practical uses of this have emerged.

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

Assigning a marker interface via directlyProvides and checking providedBy against the copied object still fails.

I'm stumped, either I'm overlooking something obvious, or something really doesn't make sense. I lean toward the former, but I've been staring at this thing a while.

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

It turns out the test marker interface needs to be assigned to the target folder, not the parent. This makes sense, I simply thought for some unbeknownst reason that it would behave like a utility and come from the parent.

So, the test passes now, but I am still concerned that this shouldn't be a tested functionality.

And I still feel that the namechooser either:

 (A) should process in this discussed order for pure logic sake
 (B) may entirely not need to be passed either the source or destination object, as it's job is to determine if a particular name is available in the container.

I'll push the working test soon, for discussion sake.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Bug attachments