Hello experts,
I made another stab at this one. Please review:
https://github.com/mpotociar/jax-rs/commit/00b33d12245849ac967cbb129daa09fcb008ddd6
Here's the change summary:
- ClientFactory merged with and renamed to ClientBuilder.
- Added new security-related setters to ClientBuilder (sslContext, keyStore, trustStore, hostnameVerifier).
- The new ClientBuilder now implements Configurable.
- Added ClientBuilder.newBuilder() static method.
- Updated examples and javadoc references to ClientFactory.
I have to say that I went as far as I could go. Clarifications, javadoc fixes, typos, method renames and similar comments and suggestions are, of course, always welcome. But, please, do not try to sneak any more features into this proposal (esp. not related to SSL), otherwise I may be inclined to go with the "not have it at all" option...
Please, send your feedback by Thursday CoB.
Thank you,
Marek
On Feb 4, 2013, at 3:01 PM, Sergey Beryozkin <sberyozkin_at_talend.com> wrote:
> More on it,
>
> Yes, I know that Apache HttpClient can be superior indeed in certain cases, but IMHO it is wise to start with the lowest common denominator and expand from there
>
> Cheers, Sergey
> On 04/02/13 13:50, Sergey Beryozkin wrote:
>> On 04/02/13 13:10, Bill Burke wrote:
>>>
>>>
>>> On 2/2/2013 12:29 PM, Marek Potociar wrote:
>>>> I believe I did provide an explanation.
>>>>
>>>> There's a lot that can be configured in SSL and we do not have enough
>>>> cycles to properly design it
>>>
>>> I disagree. At least on our end, its already being used successfully.
>>>
>>>> and implement all the concepts in RI in
>>>> time. For example, your key/trust store setters only cover the most
>>>> basic config scenario.
>>>
>>> Exactly...my setters cover the most basic config scenario that is used
>>> by 90% of applications (truststore, keystore, hostname verification,
>>> disabling trust)...an SSLContext setter covers the rest. Most users are
>>> going to be solely dealing with keystores and the default algorithms.
>>> Only insanely advanced users want to write their own trust managers and
>>> key factories. Again...for that case an SSLContext setter covers it.
>>>
>>>
>>>> In order to provide a full coverage, we would
>>>> need a lot more methods.
>>>
>>> *Why do you need full coverage?* Cover the 80-90% simple use cases, let
>>> an SSLContext setter handle the complex cases. Even so, you could add
>>> full coverage methods later on.
>>>
>>>> And rather than exposing lots of new setters in
>>>> the client builder API I'd like to see if there is a chance we could
>>>> provide a separate "SSL configuration utility" instead that would
>>>> produce the SSL context - something similar to what Grizzly provides
>>>> (see bellow). Etc. But I do not have enough cycles right now to explore
>>>> all these options and to convert them into a polished API proposal.
>>>>
>>>> As for non-SSL configuration (TTL, pool size, HTTP auth, ...), I
>>>> understand that all of it may be easy to implement with Grizzly client
>>>> or AHC, but RI has to provide default implementation based on
>>>> HttpURLConnection.
>>>
>>> Errr....why does the RI have to provide a default implementation based
>>> on HttpURLConnection? So, we're hobbling an API because of an RI
>>> constraint you put on yourselves? <sarcasm>What a great way to design a
>>> spec!</sarcasm> And, an RI that no user is ever going go use? Great!
>>>
>>
>> -1. HttpURLConnection has been successfully used by CXF WS clients (and
>> I hope by RS clients too) in all sort of productions, so we just can not
>> avoid making sure that HttpURLConnection is covered.
>>
>> I think the statement that no one will use the API (the relevant parts)
>> if it is not simply a cover above Apache HTTP Client is covered are wrong
>>
>> Sergey
>>
>>> And, isn't it possible to make a specific configuration setting
>>> optional? Making it a no-op in that scenario?
>>>
>>>> And again, we do not have enough cycles for that, not
>>>> to mention the fact that these other config settings have NOTHING to do
>>>> with security configuration and, as I have mentioned multiple times in
>>>> this forum, now is not the right time for sneaking in new features.
>>>> We're past PRD, approaching final release. If we were not able to add
>>>> these features to the client API in the last 2 years, we may need to
>>>> wait until MR is out. Sorry.
>>>>
>>>>> Finally, IMO, ClientFactory should be ditched and set property methods
>>>>> should be added to ClientBuilder. This allows implementation code to
>>>>> construct the client based on configuration properties rather than
>>>>> having to worry about concurrency or partial initialized Client
>>>>> instances that could be reconfigured if a property is set
>>>>> intermittently.
>>>>>
>>>>> Anyways, -1000 for what you've got. I'd rather not have it at all.
>>>>
>>>> That's always an option...
>>>>
>>>> We can certainly ditch the whole thing. Such decision would at least
>>>> give me more time to focus on resolving issues in RI and existing API
>>>> features. Still, I would prefer we go over the list of features that did
>>>> not get in, prioritise them and maybe try to squeeze in one or two
>>>> additional feats from the list that you and other EG members find most
>>>> important.
>>>>
>>>> In case you change your mind, here's the my contribution to the list of
>>>> features for consideration:
>>>>
>>>> - host name verification policy
>>>
>>> +1. Something I already have in Resteasy's builder class.
>>>
>>>> - ClientBuilder implements Configurable
>>>
>>> +1. I could *reluctantly* support adding the ClientBuilder as-is if you
>>> put Configurable in, and if you removed ClientFactory.newClient().
>>>
>>>> - KeyStore & TrustStore setting (is overridden if SSLContext has been
>>>> set);
>>>
>>> +1. Something I already have in Resteasy's builder class.
>>>
>>> I also have:
>>>
>>> - disabling trust entirely for testing and prototype purposes so you
>>> don't have to manage keystores and all the other stuff.
>>>
>>>
>>>> even though I'd still prefer to keep it in a separate utility
>>>> class, similar to Grizzly SSLContextConfigurator
>>>> (http://java.net/projects/grizzly/sources/git/content/modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLContextConfigurator.java)
>>>>
>>>>
>>>>
>>>
>>> That looks interesting, but I can't see anywhere else you'd want to do
>>> SSL configuration other than on a Client. You could configure SSL per
>>> WebTarget, but then WebTarget would also have to be closeable and users
>>> would have to worry about cleanup of WebTarget instances. Still, I'd
>>> compromise on anything to get something in the spec that simplified the
>>> most common use cases for SSL.
>>>
>>>
>>>
>>
>