Let a factory claim the URL being parsed, not only the calling class

[paulrutter]

Fixes #62.

A protocol can be shared by several factories that are distinguishable only by the URL itself, and there is currently no way to route on that. Multiple instances of the same framework in one JVM all use the same protocol, and the owner is an id in the URL host. Such a URL can also be parsed by a caller that no factory recognises from the call stack, which leaves nothing to select on at all — the first factory added is chosen, and it cannot resolve an id belonging to another instance. Background and the Apache Felix case are in #62.

Change

An optional hook consulted in findFactory before the call stack is walked:

default boolean shouldHandle(String protocol, String spec)

PlurlFactoryHolder delegates it the way it already delegates shouldHandle(Class) — without that, findFactory only ever asks the holders, which answer with the interface default.

Why a spec and not a URL. Selection happens while the URL is still being parsed: parseURL runs before the host and path are populated, and setHandler pins the chosen handler onto the URL immediately afterwards. I implemented the URL form first and it is unreachable wherever java.net is open — including this repo's own test build (--add-opens java.base/java.net) and the Felix framework bundle (Add-opens: java.base/java.net), because the handler swap then succeeds and plurl never regains control at openConnection. The spec is the one thing that exists at the moment plurl has to choose. Implementations are therefore handed only strings, and must not call back into URL handling.

Compatibility

Intended to be safe while released signatures stay in use in the same JVM:

• default returning false, so existing factories are unaffected and nothing needs recompiling. Java 8 baseline is unchanged (this compiles at release 8).
• Only java.lang types in the signature, so the reflective path finds the method on a factory compiled against a different copy of this package.
• A factory that predates the method is tolerated, not an error: the lookup returns empty and selection falls through to the call stack exactly as today.
• The reflective lookup is cached per class, because it sits on the URL parsing path — a missing method must not cost a lookup and an exception per URL.

Two-argument, so it does not overload shouldHandle(Class) and shouldHandle(null) stays unambiguous.

Tests

PlurlURLSelectionTest: two factories claiming by host, one URL each, neither claiming anything by call stack. Both cases fail without the change (expected:<second> but was:<first>) and pass with it, including a URL rebuilt from its external form. 51/51 green.

Not included

When no factory claims a URL, plurl selects the first factory added. Declining looks safer to me — a factory that cannot serve the URL returns a handler that then answers wrongly or fails confusingly — but that is a behaviour change for existing users, so I left it alone. Raised as a question in #62.

[paulrutter]

Added a second commit: 3fe3d05 — take it or leave it, the first commit stands on its own.

It comes from your point about released signatures staying in use in the same JVM. The copy that wins the install is the one that routes, and it may be older than the copy a factory brought with it. A factory then registers successfully and is silently misrouted, with no way to tell. For selection by spec that is the difference between working and quietly resolving to the wrong framework, so being able to detect it seems worth more than the API costs:

Set<String> caps = Plurl.capabilities();   // plurl://op/plurlCapabilities
caps.contains(Plurl.PLURL_CAPABILITY_SELECT_BY_SPEC);

An implementation predating the operation rejects it with IOException, so the absence of an answer is the answer — capabilities() reports that as an empty set, and callers treat a missing capability as unsupported rather than an error. That works against already-released plurl without it needing to know anything.

Two things worth flagging in that commit:

• plurlOperation now tolerates a leading slash in the path. The javadoc documents plurl://op/addURLStreamHandlerFactory, but getPath() on a parsed spec yields /addURLStreamHandlerFactory, which never matched the switch — only the URL(protocol, host, file) form the convenience methods use does. For a capability query that mattered: the documented form would have been indistinguishable from an older plurl. There's a test for both forms.
• getContent() on the operation connection widens from Consumer<Object> to Object, since a query returns a value rather than an operation. External callers go through URLConnection.getContent() and already cast, so it's source compatible; the existing lambdas just needed an explicit target type.

Rebased onto the branch after the main merge. 54 tests pass.

On the Felix side this is used to warn at startup when the installed router can't route bundle: URLs, rather than letting it surface later as a failed resource lookup: 45b65b6.

[tjwatson]

This method should be moved to org.eclipse.osgitech.plurl.PlurlStreamHandlerFactory because it is only called against URLStreamHandlerFactory registered factories.

Perhaps it should be called shouldHandleURL. I am thinking it should be made more specific in name and interface because we may need to do a similar enhancement to org.eclipse.osgitech.plurl.PlurlContentHandlerFactory and protocol and spec would not make sense there. There we would need something like:

    default boolean shouldHandleContent(String mimetype) { return false; }

Having different method names would make the reflection path more clear I think, if we decide we need this for content handlers.

[tjwatson]

To be clear, I am not suggesting we jump on implementing this for content handlers. I would wait and see if such functionality is really needed before doing that.

[paulrutter]

Thanks for your swift response, addressing your feedback in this commit.

Agreed and done: it is now shouldHandleURL(String protocol, String spec) on PlurlStreamHandlerFactory. The delegation moved with it, from PlurlFactoryHolder to URLStreamHandlerFactoryHolder, so the hook is only consulted on the stream path. Nothing added for content handlers, per your follow-up.

[tjwatson]

Can you also add a factory to the test method org.eclipse.osgitech.plurl.test.PlurlStreamHandlerFactoryTest.doTestURLContext(TestFactoryType, PlurlTestHandlers, PlurlTestHandlers) that takes over a protocol such that we can test that it is used over any of the other registered factories for the expected protocol it took over. That would also make sure the check by class stack path works even when there is a factory that took over a specific protocol for the other protocols.

I know these tests can be unwieldy, but they are necessary to test the different types of registrations: reflective, copy, shared classes, legacy etc.

[paulrutter]

Thanks for your swift response, addressing your feedback in this commit.

Done, and it earned its keep immediately: it found a real gap. createURLStreamHandler is asked per protocol with no URL, and selected a factory by call stack only, so a protocol served solely by a factory that claims it by URL was declined before parseURL could ever select it. All five registration types failed on the first run. The protocol is now also claimed when a factory that selects on the URL serves it.

doTestURLContext gains a fourth protocol served by two factories: one claiming it by calling class, one taking it over by URL. It asserts the taken-over protocol resolves from both contexts — including the one whose class no factory for that protocol claims, which only selection by URL can do — and that the by-class factory was never asked for a handler. Selection by call stack is still asserted for the other protocols alongside it, which was your second point.

TestFactory gained takeOver(String...) and a count of handlers handed out, so a test can tell which factory a URL was actually routed to. The reflective and copy paths needed the hook on TestNotPlurlStreamHandlerFactory and on the test.copy interface to exercise them — that is precisely the mixed-version case you raised at the start, so the unwieldiness was worth it.

55 tests pass.

[tjwatson]

If we are using a Set<String> then we just have boolean type of on/off capability types. I'm fine with that, because I really hope we don't need to add many of these and I don't anticipate we would need/want more complex capabilities than simple on/off ones.

But with that in mind would it be more simple if the spec was plurl://op/getCapability/<capability-name>.

	/**
	 * The plurl protocol operation to check a capability.
	 */
	public static final String PLURL_GET_CAPABILITY = "getCapability"; //$NON-NLS-1$
    public static final String PLURL_CAPABILITY_SELECT_BY_SPEC = "selectFactoryBySpec"; //$NON-NLS-1$

Then used like this:

Boolean isSelectFactoryBySpec = (Boolean) new URL("plurl://op/getCapability/selectFactoryBySpec").getContent());

This also gives us the freedom to specify the capability type (e.g. Boolean for on/off capabilities) in the javadoc for the capability constant and also gives us the freedom to use other types for future capabilities if we need to.

[paulrutter]

Thanks for your swift response, addressing your feedback in this commit.

Switched to plurl://op/getCapability/<capability-name>. Agreed on the reasoning — typing per capability is worth more than enumeration, which nothing needed. Plurl.capabilities() is now Plurl.getCapability(String), returning the value or null when unsupported.

One property worth keeping in view: an unknown capability is rejected with IOException the same way an unknown operation is, so the absence of an answer still tells an older plurl apart from one that answers. That is what the Felix side relies on to warn rather than silently misroute. Tests cover the known capability, an unknown one, the protocol form with its leading slash, and an unknown operation.

[tjwatson]

We lost the javadoc for the add(PlurlStreamHandlerFactory factory). The doc is up above the doc for the new method capabilities method.

[paulrutter]

Good catch, and my mistake: I anchored the insertion on the method signature, so the new method landed between add's javadoc and add itself. Restored, and the new method has its own.