Bug 338510 - [api] "Copy Connection" operation deletes the registered property set in the original connection
Summary: [api] "Copy Connection" operation deletes the registered property set in the ...
Status: RESOLVED FIXED
Alias: None
Product: Target Management
Classification: Tools
Component: RSE (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.3 M6   Edit
Assignee: David McKnight CLA
QA Contact: Martin Oberhuber CLA
URL:
Whiteboard:
Keywords: api
Depends on:
Blocks: 338665 355530
  Show dependency tree
 
Reported: 2011-03-01 04:00 EST by Noriaki Takatsu CLA
Modified: 2011-08-30 17:02 EDT (History)
1 user (show)

See Also:


Attachments
potential patch (14.41 KB, patch)
2011-03-01 15:37 EST, David McKnight CLA
no flags Details | Diff
patch to clone the subsystem property sets (3.73 KB, patch)
2011-03-01 16:38 EST, David McKnight CLA
no flags Details | Diff
alternate patch with API change (14.77 KB, patch)
2011-03-01 16:49 EST, David McKnight CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noriaki Takatsu CLA 2011-03-01 04:00:02 EST
A property set can be created/added in a subsystem, just like:
  IPropertySet set = (-subsystem-).createPropertySet(------);
  set.addProperty(key, value);
But this created property is saved normally but copying connection will cause
the created property set to be deleted from the original connection profile.
Comment 1 Martin Oberhuber CLA 2011-03-01 04:24:43 EST
What version is this with? Could this be a duplicate of but 301075 ?
Comment 2 Martin Oberhuber CLA 2011-03-01 04:25:08 EST
dup of bug 301075 ?
Comment 3 David McKnight CLA 2011-03-01 15:37:40 EST
Created attachment 190087 [details]
potential patch

I don't have a test case for this at the moment but I suspect this isn't quite the same bug as bug 301075.  In fact, it's likely that there are several other places where we aren't cloning the property sets.  As such, I'm thinking it would make sense to add API to IPropertySetContainer for property set as I've done in the patch.

Martin, do you have any thoughts on this?
Comment 4 David McKnight CLA 2011-03-01 16:07:33 EST
Comment on attachment 190087 [details]
potential patch

Actually, the patch here might not help with this - I see that we already do copy property sets in SubSystemConfiguration.cloneSubSystem().
Comment 5 David McKnight CLA 2011-03-01 16:38:47 EST
Created attachment 190094 [details]
patch to clone the subsystem property sets

The problem is that we pass the original subsystem's property set directly into the new subsystem, which results in this call to removePropertySet:

	public boolean addPropertySet(IPropertySet set) {
		IPropertySetContainer old = set.getContainer();
		if (old != null) {
			old.removePropertySet(set.getName());

The solution in the patch uses the clonePropertySets() method that was also used in bug 301075 rather than calling addPropertySet() on the original property set.  The closePropertySets() method has the potential to get copied a lot so it might be best to create a common API for this, as I put in the original patch.
Comment 6 David McKnight CLA 2011-03-01 16:49:52 EST
Created attachment 190098 [details]
alternate patch with API change

Here's an alternative patch that involves the API change made in the obsolete patch rather.  This one is probably cleaner and more reusable but it does involve new API.
Comment 7 Martin Oberhuber CLA 2011-03-01 17:47:08 EST
Introducing API for this seems right. I'm not sure why there has to be a #clonePropertySets() on several concrete classes like Host, ConnectorService, Subsystem. Why not do e.g.

   y.addPropertySets(PropertySetUtil.clone(x.getPropertySets())

On the other hand, with boilerplate code like this it's easy to forget the clone, having a direct #clonePropertySets() makes it clearer what needs to be done. I'll leave it to your imagination what's the cleaner fix.
Comment 8 Noriaki Takatsu CLA 2011-03-01 21:08:53 EST
This bug is not a duplicate of bug 301075.
In the 301075, some property or property set is not cloned to the NEW one.
But in this problem the property set that was added to the original connection
was removed / deleted from the ORIGINAL connection definition after the copy connection operation is done.
For example:
- first create a new connection
- add some property to the connection
  for example, in the Shells subsystem. Environment Variables can be defined 
  as property. I confirmed that the following data was added in the node
  properties for the connection. 
06-child.00000.06-child.00003.00-name=Shells
06-child.00000.06-child.00003.01-type=SubSystem
06-child.00000.06-child.00003.03-attr.hidden=false
06-child.00000.06-child.00003.03-attr.type=dstore.shells
06-child.00000.06-child.00003.06-child.00000.00-name=EnvironmentVariables
06-child.00000.06-child.00003.06-child.00000.01-type=PropertySet
06-child.00000.06-child.00003.06-child.00000.03-attr.description=null
06-child.00000.06-child.00003.06-child.00000.06-child.00000.00-name=XXXXXXX
06-child.00000.06-child.00003.06-child.00000.06-child.00000.01-type=Property
06-child.00000.06-child.00003.06-child.00000.06-child.00000.03-attr.type=java.lang.String
06-child.00000.06-child.00003.06-child.00000.06-child.00000.03-attr.value=VVVVVVVV

- Then Copy the defined connection.  In copy connection, you have to select
  "Rename" checkbox in "Duplicate Name Collision" and specify the new name
  for the copy.

- After that, the above EnvironmentVariables data were deleted from 
  the ORIGINAL connection node properties file.

I tested the level of 3.2.2.
Comment 9 David McKnight CLA 2011-03-01 21:11:30 EST
Noriaki, could you try this with the 2nd patch?
Comment 10 Noriaki Takatsu CLA 2011-03-01 22:09:54 EST
I've confirmed that the 2nd patch could solve this problem.
Thank you.
And how can we get the new jars for our product ?
Comment 11 Noriaki Takatsu CLA 2011-03-01 23:02:12 EST
We need a backport support for 3.0.x.
Comment 12 David McKnight CLA 2011-03-02 07:52:26 EST
(In reply to comment #11)
> We need a backport support for 3.0.x.

For a backport, we'll need to do something like the first patch, since the second patch introduces new API.
Comment 13 David McKnight CLA 2011-03-02 08:55:09 EST
I've committed the 2nd patch to cvs and opened bug 338665 for the backport.
Comment 14 John W Snyder CLA 2011-08-23 11:36:49 EDT
We need a backport to 3.2.2+. I opened bug355530 for the backport.