Bug 346127

Comment 1 Thomas Watson 2011-05-17 13:40:40 EDT

Here I think it is a mistake to build the policy into Region only to support file: or reference: protocols.  The code seems to try and convert a file: URL into a reference: URL and then fail if the URL is not a file: or reference: URL to begin with.

Personally I would remove this auto-magical conversion to reference: URL altogether and leave this policy decision in the hands of the installer.

Comment 2 Glyn Normington 2011-05-18 04:22:38 EDT

The point of this restriction was so that Virgo could prepend the region name (plus "@") to the bundle location to ensure that the same bundle installed in distinct regions didn't clash bundle locations.

The only way we found to do this was to open a stream and call:

BundleContext.installBundle(String location, InputStream input)

In order to open a stream, it appeared that we needed to require a file: or reference: URL - see the openBundleStream method.

Is there a way to lift this restriction while still preventing bundle location clashes?

Comment 3 John Ross 2011-05-18 10:50:33 EDT

Created attachment 195980 [details]
Proposed Patch

This patch removes the policy of converting all URLs to reference: URLs when using the Region.installBundle(String) method. Any URL for which there is a registered URL Handler is now supported.

This patch also adds a new test.

I also tested this against the subsystems implementation where Maven URLs are used.

Comment 4 Glyn Normington 2011-05-19 05:31:04 EDT

I reviewed the patch and I'm not sure I follow the new implementation of openBundleStream. Previously it was careful to produce a reference: URL before opening the stream. Will the new implementation work for all the URL schemes that the previous one coped with, particularly file:?

Also the exception message below is not quite right:

throw new BundleException("Location '" + location + "' resulted in an invalid bundle URI '" + location + "'", e);

There's no point inserting location twice as that will only confuse.

Comment 5 John Ross 2011-05-19 09:15:31 EDT

(In reply to comment #4)
> I reviewed the patch and I'm not sure I follow the new implementation of
> openBundleStream. Previously it was careful to produce a reference: URL before
> opening the stream. Will the new implementation work for all the URL schemes
> that the previous one coped with, particularly file:?

Perhaps I'm missing some history here because I don't understand the concern this question reflects. Is there something funky about the file: URL in conjunction with Region Digraph that required special consideration? Previously, only file: URLs were supported because everything was converted to reference: which only works with file: URLs. The patch should work with any URL for which a URL Handler exists, including reference: and file:.


> Also the exception message below is not quite right:
> throw new BundleException("Location '" + location + "' resulted in an invalid
> bundle URI '" + location + "'", e);
> There's no point inserting location twice as that will only confuse.

I'll take care of this.

Comment 6 John Ross 2011-05-19 10:41:07 EDT

(In reply to comment #4)
> I reviewed the patch and I'm not sure I follow the new implementation of
> openBundleStream. Previously it was careful to produce a reference: URL before
> opening the stream. Will the new implementation work for all the URL schemes
> that the previous one coped with, particularly file:?

Perhaps you were referring to the handling of file: URLs that represent directories? Unfortunately, the current patch will not handle this. Given a file: URL location string pointing to a directory, the patch will create a URL input stream from it and pass it along to bundleContext.installBundle(String InputStream), which is a no-no. In this case, the location string should be passed without creating a URL input stream. If an input stream must be passed, the file: URL must be converted into a reference: URL first, which is what the previous code did but with the unintended consequence that only file: URLs were supported. I think what we want to do is handle the file directory case but also support other protocols.

Comment 7 Glyn Normington 2011-05-19 12:10:01 EDT

Yes, directories need to work otherwise Virgo will fail. If you want to build this fix on a branch with the directory support, I'll happily try it out with Virgo.

Comment 8 Thomas Watson 2011-05-19 14:42:48 EDT

Created attachment 196153 [details]
patch + test

(In reply to comment #7)
> Yes, directories need to work otherwise Virgo will fail. If you want to build
> this fix on a branch with the directory support, I'll happily try it out with
> Virgo.

Glyn, if directories need to work shouldn't virgo use reference: URLs itself when installing bundles?  Using reference under the covers builds the policy to not copy the jars from disk for all file: URLs.  This is wrong.  The decision to copy the content of the bundles into the framework cache or not should be a decision left in the hands of the installer IMO.

At any rate, here is an alternative.  I would prefer to encode the region identifier into the location in such a way that the location used to install the bundle at is still a valid URL.  This patch uses an approach to place the region name as a fragment at the end of the original URL.

For example, if you install the following location into a region named "regionX":

http://server.com/bundles/thisRocks.jar

Would translate into a location of:

http://server.com/bundles/thisRocks.jar#regionX

This way we can choose to simply pass the file: URLs through to the framework with the fragment #regionName tacked onto the end.  The file: protocol handler ignores fragments and will allow the file to be opened as is.  This allows the framework to handle the directory case correctly.

Comment 9 Glyn Normington 2011-05-20 06:11:30 EDT

Hi Tom. I don't disagree with the proposed change, but since it affects the contract, we need to make a pre-req. change in Virgo so we don't cause Virgo to fail when we pick up this change. It looks as though we could make the pre-req. change early so I've raised Virgo bug 346625, which I will handle, to track this.

Comment 10 Thomas Watson 2011-05-20 15:34:05 EDT

I went ahead and released the fix.  I assume that you can delay picking up the new region impl until you have fixed Virgo.  Let me know if that is not the case and I can back it out.

Comment 11 Glyn Normington 2011-05-24 04:46:42 EDT

(In reply to comment #10)
> I went ahead and released the fix.  I assume that you can delay picking up the
> new region impl until you have fixed Virgo.  Let me know if that is not the
> case and I can back it out.

That should be fine. Thanks.