[9] Review request JDK-8131334: SAAJ Plugability Layer: using java.util.ServiceLoader

Wed Sep 30 15:08:29 UTC 2015

On 30/09/15 16:20, Deva Sagar wrote:
> Hi Georgiy,
>
> I think both Miran and I are referring to the mention of 
> SAAJMetaFactory in the bullet list about the public newInstance() 
> method. The Javadoc of the SAAJMetaFactory class still describes it as 
> an SPI and it is accessible from package-info. This is no different 
> from the current public Javadocs.
>
> Thanks,
> Deva
>
>
>> On Sep 30, 2015, at 9:16 AM, Georgiy Rakov <georgiy.rakov at oracle.com 
>> <mailto:georgiy.rakov at oracle.com>> wrote:
>>
>>
>>
>> On 29.09.2015 17:43, Deva Sagar wrote:
>>> Hi Miran,
>>>
>>> Sorry for the delay in responding to this - I was busy with WLS 
>>> release close down, and now I have moved to a different job in the 
>>> cloud organization outside of WLS and web services.
>>>
>>> Regarding your question in #3 - I think Georgiy made a good 
>>> observation about SAAJMetaFactory - making a public newInstance 
>>> method as you suggest would make it agree with the package-info, but 
>>> does not correspond to the API specification. The package-info in 
>>> Java SE 8 API docs says "SAAJMetaFactory is a service provider 
>>> interface. There are no public methods on this class.” If we put in 
>>> a public newInstance() method, it does not agree with this statement 
>>> in API Javadoc. It might be better in my opinion to 
>>> remove SAAJMetaFactory mention from package-info.java unless there 
>>> is a good reason to keep it.
>>>
>> Hello,
>>
>> according to my understanding the idea of SAAJMetaFactory is for SPI 
>> developer to subclass it and thus to create a custom SAAJMetaFactory 
>> implementation.
Yes, it's current state - the only description of lookup/instantiation 
user SAAJMetaFactory is statement that it is service provider interaface 
(stays unchanged): 
https://docs.oracle.com/javase/8/docs/api/javax/xml/soap/SAAJMetaFactory.html
Service loader is the only specification here.
>> Afterwards user should configure SAAJ API in order it could use that 
>> custom implementation.
User configuration controls discovery of implementation when used 
newInstance method on other factories (MessageFactory, SOAPFactory, 
SOAPConnectionFactory). If no implementation found, the fallback is user 
(or default) SAAJMetaFactory.newXXXFactory()
>> But if SAAJMetaFactory mention from package-info.java is removed 
>> there will be actually no spec defining how to configure SAAJ API for 
>> using custom SAAJMetaFactory, or do I miss spec defining it?
No, there is still statement about service provider interface.
>> BTW: from this perspective referring to SAAJMetaFactory newInstance 
>> method in spec is not relevant since it's SAAJ API implementation 
>> internal machinery.
Yes, it is internal.

Actually I thing SAAJ API is pretty complicated already, so not adding 
another spec for SAAJMetaFactory discovery is good idea ...

Thanks
Miran

>>
>> So I believe if SAAJMetaFactory mention from package-info.java is 
>> removed the process of SAAJMetaFactory instantiation should be 
>> described somehow in another place and without mentioning newInstance 
>> method. Maybe it's worth rather modifying than removing this 
>> mentioning in package-info.java?
>>
>> Thanks,
>> Georgiy.
>>
>>> The rest of your changes look ok to me. Also cc’ing Chen for any 
>>> additional comments
>>>
>>> Deva
>>>
>>>
>>>
>>>> On Sep 29, 2015, at 9:45 AM, Miroslav Kos <miroslav.kos at oracle.com> 
>>>> wrote:
>>>>
>>>> On 25/09/15 20:24, Georgiy Rakov wrote:
>>>>> Hello Miroslav,
>>>>>
>>>>> sorry for delay, was busy due to deadlines.
>>>>>
>>>>> I think it's just spec I am to review. Some comments for the 
>>>>> moment please see below:
>>>>>
>>>>> 1. package-info.java:
>>>>>
>>>>>       *  <li>Checks if a system property with the same name as the factory class is set (/_*i.e.*_/
>>>>>       *  {@code javax.xml.soap.SOAPFactory}).
>>>>>
>>>>> I guess "e.g." is meant, that is "for example"?
>>>> Sure, thanks.
>>>>>
>>>>> 2. SAAJMetaFactory.java:
>>>>>
>>>>> Following is specified normatively:
>>>>>    62      * <p>Property {@code javax.xml.soap.MetaFactory} used in previous
>>>>>    63      * specifications (up to 1.3) is still supported, but it is strongly recommended to migrate to new property
>>>>>    64      * {@code javax.xml.soap.SAAJMetaFactory}.
>>>>> while in package-info.java: the same idea is specified as API note 
>>>>> that is non-normatively:
>>>>>   * @apiNote
>>>>>   * <p>For {@link javax.xml.soap.SAAJMetaFactory}, property {@code javax.xml.soap.MetaFactory} used in previous
>>>>>   * specifications (up to 1.3) is still supported, but it is strongly recommended to migrate to new property
>>>>>   * {@code javax.xml.soap.SAAJMetaFactory}.
>>>>>   */
>>>>> I believe this idea should be specified everywhere the same way 
>>>>> that is either normatively or non-normatively. If it's to be 
>>>>> defined normatively more details should be specified I believe.
>>>> Made non-normative.
>>>>>
>>>>> 3. SAAJMetaFactory.getInstance method is package private. So:
>>>>> - This method is not supposed to be used by users, is it?
>>>>> - The javadoc of this method isn't actually a public 
>>>>> specification. Thus Oracle claims nothing by it and external Java 
>>>>> SE implementers are not required to follow this JavaDoc, that is 
>>>>> this JavaDoc is intended to be used internally only, isn't it?
>>>>> I understand that this was before, but It looks very strange. Is 
>>>>> this really not a mistake? BTW: this method and spec are not 
>>>>> visible in API JavaDoc 
>>>>> <https://docs.oracle.com/javase/8/docs/api/javax/xml/soap/SAAJMetaFactory.html>.
>>>>>
>>>>> It looks like a mistake moreover because public specification 
>>>>> defined in package-info.java being reviewed refers to it:
>>>>>   * There are several factories defined in the SAAJ API to discover and load specific implementation:
>>>>>   *
>>>>>   * <ul>
>>>>>   *     <li>{@link javax.xml.soap.SOAPFactory}
>>>>>   *     <li>{@link javax.xml.soap.MessageFactory}
>>>>>   *     <li>{@link javax.xml.soap.SOAPConnectionFactory}
>>>>>   /_** <li>{@link javax.xml.soap.SAAJMetaFactory}*_/
>>>>>   * </ul>
>>>>>   *
>>>>>   * These factories define {@code/_*newInstance()*_/} method which uses a common lookup procedure to determine
>>>>>   * the implementation class:
>>>> It's good catch - it looks like error. An easy fix would be change 
>>>> the method to newInstance and make it public - Deva, would you agree?
>>>> http://cr.openjdk.java.net/~mkos/8131334/specdiff.04/index.html
>>>>
>>>> Anyway those are changes to API - should I withdraw 
>>>> http://ccc.us.oracle.com/8131334 and create new one?
>>>>
>>>> Thanks
>>>> Miran
>>>>
>>>>
>>>>
>>>>> Thank you,
>>>>> Goergiy.
>>>>>
>>>>> On 25.09.2015 16:34, Miroslav Kos wrote:
>>>>>> Ping ...
>>>>>>
>>>>>> On 11/09/15 17:57, Miroslav Kos wrote:
>>>>>>> Hi again,
>>>>>>> would somebody find a time to review?
>>>>>>>
>>>>>>> Thanks
>>>>>>> Miran
>>>>>>>
>>>>>>> On 20/08/15 16:17, Miroslav Kos wrote:
>>>>>>>> Hi everybody,
>>>>>>>>
>>>>>>>> I am sending changes for review for
>>>>>>>> JDK-8131334: SAAJ Plugability Layer: using java.util.ServiceLoader
>>>>>>>>
>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8131334
>>>>>>>> webrev: http://cr.openjdk.java.net/~mkos/8131334/jaxws.01/
>>>>>>>>
>>>>>>>> It's about migrating to standard java.util.ServiceLoader. This 
>>>>>>>> part of service discovery was implemented previously "own" way. 
>>>>>>>> There are some changes in javadoc and implementation has been 
>>>>>>>> refactored in order to use same code as in JAX-WS and JAX-B.
>>>>>>>>
>>>>>>>> Testing - I run JAX-WS unit tests (JAX-WS standalone repo), 
>>>>>>>> JCK9 + new tests specificaly developed for testing service 
>>>>>>>> discovery in SAAJ.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Miran
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>