Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 182454

Summary:

ISystemViewElementAdapter.getAbsoluteName() must be clearly documented

Product:

[Tools] Target Management

Reporter:

Uwe Stieber <uwe.st>

Component:

RSE

Assignee:

Martin Oberhuber <mober.at+eclipse>

Status:

RESOLVED FIXED

QA Contact:

Martin Oberhuber <mober.at+eclipse>

Severity:

normal

Priority:

CC:

ddykstal.eclipse, dmcknigh, martin.gutschelhofer, tobias.schwarz

Version:

2.0

Target Milestone:

2.0

Hardware:

OS:

Windows XP

Whiteboard:

Attachments:

Description	Flags
Patch removing the return of the element adapters hashcode	none
Updated patch more clearly showing the critical point	none

Description Uwe Stieber

2007-04-15 09:00:04 EDT

The SystemView's IElementComparer implementation can return the same hashcode for two different elements. The default implementation does return the elements adapters hashcode for elements where getAbsoluteName() returns null. However, the same element adapter can be re-used for multiple elements. That would lead to returning the same hashcode for multiple elements. At least I would call that behavior problematic. In any cases, if the hash code of the element cannot be calculated out of the absolute name, the hashcode from the element _itself_ should be returned and not the one from the element adapter. The typical pattern in Java is to override the hashcode calculation of the element itself. It's quite harder to calculate the hashcode of an element within a different (adapter) class, especially because the hashcode calculation method does not take any element.

Please note that the cases where this happens are rare, but problematic because of two reasons:
a) For ISV's being new to the RSE framework, the importance and real purpose of getAbsoluteName() is not obvious. It's possible that in early stages of an integration, that method is simply returning null. If the implementor now is running into for beginners very hard to track down issues with NPE's thrown by the SystemView's tree viewer, the chance that the ISV is giving up is very high.
b) Finding problems if the SystemView's tree viewer fails to identify the tree item objects correctly, are difficult to track down. You need a deep knowlegde about the SystemView, JFace tree viewers in general and the purpose of the IElementComparer in particular. The implementation of that thing should be as bullet proof as ever possible. It is such central for the correct behavior of the SystemView, that willingly returning that same hashcode for multiple elements does not seem a good idea to me.

Comment 1 Uwe Stieber

2007-04-15 09:06:16 EDT

Created attachment 63833 [details]
Patch removing the return of the element adapters hashcode

Committers: Discussion is open.

Attached patch
- reformated the IElementComparer for better readablitly,
- Added (commented out) tracing from tracking down problems in here
- Removed the "return indent.hashCode()" in order to fallback to the elements own hashcode calculation method.

Comment 2 Uwe Stieber

2007-04-15 09:30:22 EDT

One manifestation of the problem can be NPE's like the following:

Caused by: java.lang.NullPointerException
	at org.eclipse.jface.viewers.TreeViewer.getParentItem(TreeViewer.java:216)
	at org.eclipse.rse.internal.ui.view.SafeTreeViewer.getParentItem(SafeTreeViewer.java:159)
	at org.eclipse.rse.internal.ui.view.SystemView.refreshAll(SystemView.java:3123)
	at org.eclipse.rse.internal.ui.view.SystemView$ResourceChangedJob.runInUIThread(SystemView.java:1942)
	at org.eclipse.ui.progress.UIJob$1.run(UIJob.java:94)
	at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35)
	at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:123)
	... 56 more

Comment 3 David Dykstal

2007-04-16 09:29:34 EDT

I have not yet read the code, but something sounds odd. Hashcodes should, in general, not be identical for different items if it can be helped. That's just good practice and a fix is warranted in this case. But hashcodes are not guaranteed to be unique. If code is written to assume that two objects are identical if they have the same hash then that code is wrong.

Comment 4 Martin Oberhuber

2007-04-16 09:48:18 EDT

Dave is right -- different elements may return the same hashCode although this leads to bad performance. The same element, however, may never return different hashCodes. From this perspective, the idea of the patch looks good since it may improve performance.

The really important point in this bug though is, that 
  ISystemViewElementAdapter.getAbsoluteName() must be clearly documented.
Thus, changed the bug summary accordingly.
(old value: The SystemView's IElementComparer may return same hashcode for different model objects)

The SystemView makes so much use of getAbsoluteName() in such important places, that it is critical for extenders to properly implement the getAbsoluteName() API. Especially, it needs to be clarified whether getAbsoluteName() is allowed to return null or not. The code in the ElementComparer's equals() method seems to indicate that this is not allowed (NullPointerException is possible if 1st element returns null absolute name).

The whole story, purpose, meaning and constraints of getAbsoluteName() need to be clearly documented. Perhaps AbstractSystemViewElementAdapter.getAbsoluteName() needs to be removed to make it abstract and force extenders to implement it.

The changes made by the patch are hard to understand due to the beautification. 
The SystemView's ElementComparer should probably be put into a separate file instead. The important change is just in one line. Under the assumption that getAbsoluteName() must not ever return null, the patch is even invalid.

Comment 5 Martin Oberhuber

2007-04-16 09:54:01 EDT

Created attachment 63902 [details]
Updated patch more clearly showing the critical point

This updated patch is reduced to the important line in order to more clearly show its intention.

Comment 6 Martin Oberhuber

2007-04-25 08:03:45 EDT

Wrote up documentation in
   IRemoteObjectIdentifier
   IRemoteObjectResolver
and made sure that all actual implementations of the methods either have no Javadoc at all or reference the original Javadoc from these two classes.

While working on the documentation, I found a need to clearly specify what kinds of IDs should be considered reserved by RSE-internal objects such as filter references - created bug 183965 to track this issue.

Note that API Docs forbid getAbsoluteName() to return null and also explains why. I do think, however, that returning null may be valid under the following preconditions:
  1. drag&drop, copy&paste are not supported by the subsystem 
     for these elements
  2. The elements never change, i.e. their hashCode() is always the same and
     can be used to uniquely identify the element (together with equals()) -
     this is important during refresh
Because I think it should basically work, I don't want to add assertions checking for null into the code right now. We might add testcases for this at some time since working without absolute names may improve performance. But for now, extenders returning null are on their own due to the API docs and will not get any support.

Committers please review my docs.

Comment 7 Martin Oberhuber

2008-08-13 13:18:38 EDT

[target cleanup] 2.0 M7 was the original target milestone for this bug