RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

[Joe Wang]

On 12/18/2012 10:00 AM, Daniel Fuchs wrote:
> On 12/18/12 6:29 PM, Joe Wang wrote:
>> Hi Daniel,
>>
>> The call: T provider = findServiceProvider(type) in static <T> T
>> find(Class<T> type, String factoryId, ClassLoader cl, String
>> fallbackClassName) ignored factoryId, and assumed it's the same as
>> type.getName(). I looked back I had the same bug in my original patch.
>
> I don't think that's a bug in the new code - but rather possibly a
> bug in the old code ;-).

The old code did not have the problem since it's reading 
"META-INF/services/" + factoryId directly.

> There's no way you can pass a property name to the ServiceLoader.

True. That's why it looked a bit awkward since 'factoryId' is not passed 
in as did before, as in findJarServiceProvider(factoryId).

>
> The JAR Specification says:
>
> "A service provider identifies itself by placing a 
> provider-configuration file in the resource directory 
> META-INF/services. The file's name should consist of the 
> fully-qualified name of the abstract service class."
>
> So I think the current code is correct in ignoring factoryId - because
> according to the spec the file name should be the same as the
> abstract class name.

So the StAX API implied that factoryId could be anything, but that would 
violate the JAR specification. In other words, it would only work as 
intended if the factoryId is specified as a System Property, or in 
stax.properties or jaxp.properties.

I would think we should return an error if factoryId != type.getName() 
before the call "findServiceProvider(type)".

-Joe

>
>
> -- daniel
>
>>
>> -Joe
>>
>> On 12/18/2012 8:39 AM, Daniel Fuchs wrote:
>>> Hi,
>>>
>>> Thanks for the review.
>>> I updated the webrev to keep track of your suggested change.
>>> <http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.01/> 
>>>
>>>
>>>
>>> -- daniel
>>>
>>> On 12/18/12 4:00 PM, Alan Bateman wrote:
>>>>
>>>> I looked through this installment and aside from an aside from an
>>>> alignment issue at lines 101-102 in XMLEventFactory.java then it looks
>>>> good to me.
>>>>
>>>> Also thank you again for being so careful as you work through each of
>>>> these areas.
>>>>
>>>> -Alan
>>>
>