Comment 1 Thomas Hallgren 2009-01-20 09:27:02 EST

The bug was caused by a faulty local repository entry. Instead of typing

file:/path/to/repository

I just typed:

/path/to/repository

The entry was silently accepted which is bad. It's added to the list of known repositories which gives the impression that it is valid.

Both the dialog for 'Add repository' dialog and the "Work with:" entries need a verifier. Perhaps an entry starting with a '/' should be converted into a as file: URI?

Comment 2 Susan McCourt 2009-01-20 11:41:04 EST

Renaming bug to focus on the issue.
The UI uses the URIUtil class to decide how to interpret a string, and if it gets a valid URI it adds it to the repo manager.

If we want to interpret '/' as a file URI, that would/should be done in URIUtil, although I could see how it may be contextual whether this would always be the default.  It might require some configuration of URIUtil.  

What do you think, John?

Comment 3 Pascal Rapicault 2009-01-20 12:04:49 EST

In addition to this issue, there is still a problem were the comparator does not handle the case where the scheme is null.

Comment 4 John Arthorne 2009-01-20 16:08:49 EST

I don't think we can hard-code "file:" as the implied scheme in URIUtil. The string "/path/to/repository" is actually a valid relative URI (relative URI is defined as a URI with no scheme). There are quite a few places in p2 where we traffic in and manipulate relative URIs so this would be a potentially breaking assumption to make at that level. We have some places where we handle this with a pattern roughly like this (in a context where we know a relative URI is not possible):

URI result = URIUtil.fromString(string);
if (result.getScheme() == null)
  result = URIUtil.fromString("file:" + string);

Would it be possible to use this approach in the "add repository" dialog, or to use keystroke validation to ensure the URI has a scheme?

Comment 5 Susan McCourt 2009-01-20 16:38:19 EST

Yes, we could consider doing such a validation in the RepositoryLocationValidator (this is where other file/jar type stuff is done), and this is used by all the UI text boxes and drop targets.

I can take this bug (not for M5), but I think Pascal's comment #3 was intended to remind you of some related fix for M5, so I won't take this bug until you tell me I can...

Comment 6 John Arthorne 2009-01-20 16:45:35 EST

Created attachment 123126 [details]
Comparator fix

Comment 7 John Arthorne 2009-01-20 16:47:47 EST

Thanks, I missed Pascal's comment. I have fixed the specific NPE problem in DownloadManager. Susan, I'll leave it to you if you want to repurpose this bug for the input validation or enter a separate one.

Comment 8 Susan McCourt 2009-01-21 11:45:30 EST

Since I hijacked Thomas' bug on the UI and turned it into a URIUtil bug, I'll hijack it back and purpose it to his original suggestion.  ;-)

Comment 9 John Arthorne 2009-01-21 11:49:54 EST

Do you think this would be widespread in the UI, or just isolated to one or two places? We could look into adding a method to do this somewhere if it becomes a common pattern.

Comment 10 Susan McCourt 2009-01-21 12:06:30 EST

(In reply to comment #9)
> Do you think this would be widespread in the UI, or just isolated to one or two
> places? We could look into adding a method to do this somewhere if it becomes a
> common pattern.
> 

All string->location mapping and validation in the UI runs through RepositoryLocationValidator, so I was thinking of doing it there.  As an initial fix, we could interpret paths as file:, but this would also be a place to allow configuration/override of that decision if we needed it in the future.

Comment 11 Susan McCourt 2009-04-16 20:33:53 EDT

Fixed in HEAD >20090416.

(In reply to comment #9)
> Do you think this would be widespread in the UI, or just isolated to 
> one or two places?  We could look into adding a method to do this somewhere
> if it becomes a common pattern.

This wasn't too widespread.  It hit about 4 places in the UI, and I collapsed it into my own helper method which made use of RepositoryHelper.localRepoURIHelper


try {
	userLocation = URIUtil.fromString(locationString);
} catch (URISyntaxException e) {
	return null;
}
// If a path separator char was used, interpret as a local file URI
String uriString = URIUtil.toUnencodedString(userLocation);
if (uriString.length() > 0 && (uriString.charAt(0) == '/' || uriString.charAt(0) == File.separatorChar))
	return RepositoryHelper.localRepoURIHelper(userLocation);
return userLocation;

While I was doing this, I got rid of all the UI code that was making jar: and file: URI's and switched to use RepositoryHelper in all of those places.  I was able to punt a bunch of code.