Hi Daniel,
I also agree with option 3 be the best option among them. Joe's
suggestion to throw an exception if factoryId is not the base service
type is good so that any existing application depending on that behavior
will catch this change. Are you going to revise the spec - there is
statement saying that the newFactory method is the same behavior as
newInstance method which is weakly specified.
139 * No changes in behavior are defined by this replacement method relative
140 * to the deprecated method.
It'd be good to add some tests to cover these APIs if there is none.
Otherwise, the change looks good.
Mandy
On 1/7/2013 2:34 AM, Daniel Fuchs wrote:
Thanks Joe.
To make things clear I have pushed a revised webrev with solution 3.
<http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.02.3/>
The lines of interest are in FactoryFinder.java - right hand side:
267 if (type.getName().equals(factoryId)) {
268 // Try Jar Service Provider Mechanism
269 final T provider = findServiceProvider(type);
270 if (provider != null) {
271 return provider;
272 }
273 } else {
274 // We're in the case where a 'custom' factoryId was
provided,
275 // and in every case where that happens, we expect that
276 // fallbackClassName will be null.
277 assert fallbackClassName == null;
278 }
regards,
-- daniel
On 1/4/13 7:51 PM, Joe Wang wrote:
Hi Daniel,
Yes, I agree with 3. As I said before, we should return an error if
factoryId != type.getName() since it indicates a configuration error.
Scenario 4 does exist, but it's beyond the current spec. Such an impl
should not use the default API.
The StAX spec is not always clear. My interpretation of the definition
of factoryId to be the "same as a property name" is that the author
basically was saying that it's equivalent to a name as in a property,
not the "value", therefore different from the DOM/SAX API -- a bad
choice I would think.
Thanks,
Joe
On 1/4/2013 6:31 AM, Daniel Fuchs wrote:
Hi guys,
Happy new year to you all! And apologies for this long email :-)
I think we still haven't converged on this issue in
javax.xml.stream - so let me make a recap.
The issue is for the newInstance/newFactory methods
that take a factoryId parameter, in the factories for
the javax.xml.stream package:
[
<http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.01/>
FactoryFinder.java: line 267-268 right hand side. ]
recap of the issue:
-------------------
In case the provided factoryId did not correspond to any
System/JAXP/StAX property, the old code used to look
for a service in META-INF/services using the factoryId as the
name of the file for looking up the implementation class.
In case factoryId was not the same as the base service
class (or a subclass of it) it still would have worked
although it went in contradiction to the Jar Specification
mentioned in the javadoc, since the Jar Specification clearly
states that the file name should be the fully qualified name
of the base service class.
Since we're going to replace the code that looked up for
a service in META-INF with a call to ServiceLoader, we can
no longer fully preserve that old behavior, because ServiceLoader
only takes a base service class (and no arbitrary file name).
what choices do we have?
------------------------
The question is therefore how we want to change this.
I think we have 4 (exclusive) possibilities.
In the case where a factoryId is provided, but no
System/JAXP/StAX property by that name has been found,
we could choose to either:
1. Always call ServiceLoader.load(type) where type is the
service base class.
2. Never call ServiceLoader.load(type)
3. Only call ServiceLoader.load(type) when the provided
factoryId is equal to type.getName()
4. If factoryId is equal to type.getName(), call
ServiceLoader.load(type), otherwise,
attempt to load factoryId as a class - and if that
class is a subclass of 'type' then call
ServiceLoader.load for that subclass.
pros & cons:
------------
Here is a list of pros & cons for each of these options:
1. is the naive implementation I originally proposed.
Pros:
- P1.1: simple
- P1.2: no change in behavior if factoryId == type.getName()
Cons:
- C1.1: may find no provider when the old code used to
find one, (case where factoryId="foo", there's a
provider for "foo" and there's no provider for type.getName())
- C1.2: may find a provider when the old code used to
find none (case where factoryId="foo", there's no provider
for foo, but there's a provider for type.getName())
- C1.3: may find a different provider than what the old
code use to find (case where factoryId="foo", there's a
provider for "foo" and there's also a provider for
type.getName())
2. is more drastic: if you specify a property - then the property
has to be defined somewhere for newInstance/newFactory to
succeed.
Pros:
- P2.1: simple
Cons:
- C2.1: there's a change of behavior even when
factoryId == type.getName() and no property by that
name is defined.
- C2.2: in all cases where the old code used to find a
provider by looking at META-INF/services, the new code
will find nothing.
Although it's the most simple - it's probably the most risky
in terms of compatibility.
3. Same as 1. except that C1.2 and C1.3 are removed.
4. Same as 3. except that it's more complex (so P1.1 is
removed) and that C1.1 will not occur in the case
where factoryId is the name of a subclass of 'type'
In conclusion:
--------------
Since the original spec only says that factoryId is
"same as property" - it's very difficult to figure out
which of these 4 options is the closer to the behavior
intended by the spec.
I personally think that the case where factoryId="foo",
and no property "foo" is defined, but a provider exists for
"foo" and an implementation is returned, is a bug - since
it's in contradiction with the Jar Specification mentioned
in the spec which clearly states that "foo" should
have been a service class name.
So although 4. is the implementation that would offer the
greater compatibility with existing code, I am sorely
tempted to recommend that we do 3. and clarify
the spec on this point.
(3: Only call ServiceLoader.load(type) when the provided
factoryId is equal to type.getName())
Best regards,
-- daniel
On 12/19/12 10:45 AM, Daniel Fuchs wrote:
On 12/19/12 12:10 AM, Joe Wang wrote:
It's different. If 'foo.bar' is specified but not found, it indicates
a configuration error. If the factory falls back to an impl by the
default factory id, it would serve to hide the error.
Yes - I fully agree with that.
Note that newInstance/newFactory with a factoryId parameter do not
fall back to the default implementation.
Ahh! Yes I missed that. When called from newInstance with a factoryId
parameter the fallbackClassname parameter
is null...
So should we still call ServiceLoader when fallbackClassname is
null and
factoryId is type.getName()? It would be more
backward compatible - since previously it was looking in the jars and
found (valid) providers registered with that name.
On the other hand we could alter the spec to say that if no property
with the factoryId name is found - then
no fallback loading is perform (either on ServiceLoader or
system-default implementation) and an error is thrown.
-- daniel
