Bug 221556 - [api][breaking] introduce IHostPath for paths in IFileService
Summary: [api][breaking] introduce IHostPath for paths in IFileService
Status: NEW
Alias: None
Product: Target Management
Classification: Tools
Component: RSE (show other bugs)
Version: 3.0   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: dsdp.tm.rse-inbox CLA
QA Contact: Martin Oberhuber CLA
URL:
Whiteboard:
Keywords: api
: 218947 (view as bug list)
Depends on: 162950
Blocks: 224947
  Show dependency tree
 
Reported: 2008-03-05 12:44 EST by David McKnight CLA
Modified: 2012-11-19 04:57 EST (History)
3 users (show)

See Also:


Attachments
IHostPath and HostPath (18.29 KB, patch)
2008-04-29 16:46 EDT, Rupen Mardirossian CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David McKnight CLA 2008-03-05 12:44:33 EST
For situations as those that can arise with bug 218947, it would be useful to introduce an IHostPath interface.  The IFileService layer would need to be changed so that instead of taking String for the name and the parent path, the APIs would instead use IHostPath.
Comment 1 Martin Oberhuber CLA 2008-03-06 05:31:46 EST
Adding Lothar, who filed the original API change request on bug 162950.

I agree that the current situation with using Strings everywhere is cumbersome, especially because
  * Given a remote path in native form, it's unclear how to construct the 
    parent path or child path

Now there are two ways of fixing this particular situation:
  a) Get rid of Strings everywhere and us an IHostPath object instead; or,
  b) Create a single "Converter" API where service implementers can contribute
     their converters in order to answer the questions they have like "who
     is the parent of X".

Advantage of (b) the Converter API is that much less (if any) existing APIs need to be touched. It's a model-controller approach where the model (Strings) stays the same.

Advantage of (a) replacing String everywhere is that it adds type safety checked by the compiler.

Realistically speaking, we have no chance going with (a) for RSE 3.0 since there's not enough time. So we could start going with (b) for now, and if we have a Converter API we could even gradually replace "String" by "IHostPath" as we go forward since anybody can convert one into the other if needed.

Here's one possible converter API, that would put all operations like getParent(), toString(), etc into an IHostPath object:

interface IHostPathConverter {
   public IHostPath hostPathFor(String path);
}

Here's another possibility:

interface IHostPathConverter {
   public String getParent(String aPath);
   public boolean isRoot(String aPath);
   public String concat(String aPath, String aChild);
}

I think I'd prefer going with the first approach since IHostPath objects could provide an optimized storage mechanism for the paths in the long run, so if we replace more and more Strings by IHostPath we might get better performance and type safety in the end.

Comments welcome.
Comment 2 Radoslav Gerganov CLA 2008-03-06 07:05:19 EST
Keep in mind that in order to support EFS each subsystem should provide converter for EFS paths as well. In other words we can't use the same converter for both EFS paths and native paths. OR we can extend the converter API to support EFS paths:

interface IHostPathConverter {
   public IHostPath hostPathForNativePath(String path);
   public IHostPath hostPathForEfsPath(String path);
}

and use separate APIs for contructing IRemoteFiles for the EFS provider and RSE file filters.
Comment 3 Martin Oberhuber CLA 2008-03-06 07:31:10 EST
Thanks Radoslav, that's a good point.

It basically boils down to the question what the "String" that should represent a remote path in RSE should look like. Requirements to that "String" are different depending on context:

  * For EFS, it must be possible to construct a proper URI out of the String
    entered by a user in a text field.

  * For RSE, the String should represent what the remote file path would look
    like in a remote file system, as closely as possible

In current RSE, we do not (yet) serve the second requirement well since for VMS paths we convert them into "normalized path Strings". Which also serve the EFS requirement so currently, RSE is not really capable of handling "host style paths" very well.

That may change with introducing the HostPathConverter in the future, but may be a whole lot of work since currently, I think there are lots of places where RSE expects the "String" paths to be normalized in some fashion (at least, using / or \ as path separator character).
Comment 4 David McKnight CLA 2008-04-02 13:58:16 EDT
*** Bug 218947 has been marked as a duplicate of this bug. ***
Comment 5 Rupen Mardirossian CLA 2008-04-16 16:23:36 EDT
Please take a look at comment #2 on bug 224947.  Using IPath may be useful for this defect. 
Comment 6 Martin Oberhuber CLA 2008-04-16 16:42:23 EDT
The problem with the Eclipse org.eclipse.core.runtime.Path implementation of the IPath interface is that some of its functionality depends on the client system that's running Eclipse.

In our case, what we need is an implementation of a subset of IPath with functionality specific to the Host operating system. If you try to use org.eclipse.core.runtime.Path on a Windows system, you cannot manipulate UNIX paths that contain characters such as ;<>? which are valid in UNIX filenames but invalid on Windows.

If you manipulate paths of a remote Windows system on a local UNIX box, the org.eclipse.core.runtime.Path implementation would likely not split at the "\" characters since these are a valid part of a UNIX filename.

That's why I think that we cannot re-use this class without modifications. Though I certainly like some of the functionality that it provides.
Comment 7 Rupen Mardirossian CLA 2008-04-29 16:46:28 EDT
Created attachment 98073 [details]
IHostPath and HostPath

Please give me any feedback on these two files, including information on the comments and javadocs.  I have been using them for my solution for bug 224947, and everything seems to be working well.  Please review.
Comment 8 Martin Oberhuber CLA 2008-04-29 16:56:59 EDT
Rupen, the files look cloned from Eclipse IPath / Path. You must give the original authors credit in the file's copyright header when you use their copyrighted work. We typically do this by copying the original copyright header, and adding yourself as last line on "Contributors" saying e.g.

Rupen Mardirossian (IBM) - adapted from org.eclipse.core.runtime.IPath 
Comment 9 Martin Oberhuber CLA 2008-04-29 16:58:01 EDT
Also, please explain in what respect your implementation differs from the Eclipse IPath / Path implementation. Could you also have "RSEPath" as an implementation of the original IPath interface?
Comment 10 Martin Oberhuber CLA 2008-04-29 17:01:06 EDT
Your first new code using PathUtility.getSeparator() in the HostPath constructor looks ok, but then this:
  		if (WINDOWS) {
			//extract device
is certainly not ok because it references the CLIENT machine but we are dealing with HOST (remote) paths. This would fail if somebody had the odd idea of creating a UNIX folder like this:

mkdir -p /c\:/rupen

Comment 11 Rupen Mardirossian CLA 2008-04-30 10:00:33 EDT
Thanks for the comments Martin,  

1)I will change copyright information accordingly.

2)Main differences from IPath is that it ignores certain things like trailing separators, and determines separators using the PathUtility

3)I think having an implementation of IPath would be bothersome as it holds many unnecessary methods.  What would the difference be from IHostPath that I am implementing now?

4)Do you think we should remove the device part and identify the device as a segment instead?.. so c:\rupen would have two segments instead of having one segment and a device part.
Comment 12 Martin Oberhuber CLA 2008-05-02 13:14:44 EDT
(In reply to comment #11)
> 2)Main differences from IPath is that it ignores certain things like trailing
> separators, and determines separators using the PathUtility

You talk about implementation differences, but I asked about interface (API) differences between IPath and IHostPath.

> 3)I think having an implementation of IPath would be bothersome as it holds
> many unnecessary methods.

Which ones do you think are unnecessary?

> What would the difference be from IHostPath that I am implementing now?

If we implement IPath rather than defining our own IHostPath, we can directly
push that IPath object into any other (3rd party) code that wants an IPath.
For instance, remote make. Or models of the remote system. Or resource
change listeners.

Not that I think this is necessary, but I'd like to understand the compromise
we are making by doing our own thing here.

> 4)Do you think we should remove the device part and identify the device as a
> segment instead?.. so c:\rupen would have two segments instead of having one
> segment and a device part.

No, definitely not! The "device" part must be separate because this is the portion of the path that you cannot get rid of by doing p.removeLastSegments(1). There is much code around, which recursively navigates up a tree by always looking at the parent of a path. Such code must not ever get rid of the C: because the result would be an invalid path. Therefore we need the "device". It is the counterpart of what we call a "Root" in RSE.