On Nov 20, 2008, at 6:30 PM, Jorge L Williams wrote:
>
>
> Paul.Sandoz_at_Sun.COM wrote on 11/20/2008 08:08:05 AM:
>
> >
> > On Nov 20, 2008, at 3:42 PM, Jorge L. Williams wrote:
> >
> > > -----BEGIN PGP SIGNED MESSAGE-----
> > > Hash: SHA1
> > >
> > > Paul Sandoz wrote:
> > >> Hi Jorge,
> > >>
> > >> I have looked more closely at the patch, some comments and
> proposed
> > >> improvements:
> > >>
> > >> - Alex pointed out to me that by default interactive prompting
> for
> > >> credentials occurs. I think the default value for
> > >> PROPERTY_INTERACTIVE and PROPERTY_PREEMPTIVE_AUTHENTICATION
> should
> > >> be false, since by client API is likely to be used in
> > >> non-interactive environments.
> > >>
> > >
> > > The default for PROPERTY_PREEMPTIVE_AUTHENTICATION is already
> false.
> > >
> > > As for, PROPERTY_INTERACTIVE, I distinctly remember that
> prompting
> > > for credentials was the default behavior when using
> URLConnection and
> > > I wanted to match the behavior in case people were relying on
> it. Now
> > > that I look at the documentation, I guess I must have dreamt that
> > > behavior because I can't find a reference to it. Keep in mind
> that,
> > > if you set interactive to true, but also set the credentials in
> the
> > > Config you will *not* be prompt.
> >
> > OK.
> >
> >
>
> Just to clarify: Does this mean you're okay with the current default
> of setting interactive to true??
>
No. It means that i understood what you said.
>
> > > None the less, I really don't have
> > > a problem with setting the default to false.
> > >
> > >> - Rename: PROPERTY_INTERACTIVE ->
> > >> PROPERTY_AUTHENTICATION_INTERACTIVE
> > >> PROPERTY_PREEMPTIVE_AUTHENTICATION ->
> > >> PROPERTY_AUTHENTICATION_PREEMPTIVE
> > >>
> > > Good idea, no problem.
> > >
> > >> - Change the properties PROPERTY_PROXY_SET, PROPERTY_PROXY_HOST,
> > >> PROPERTY_PROXY_PORT to one property PROPERTY_PROXY that is a
> string
> > >> or URI, which if the port is absent then 8080 is used.
> > >>
> > > Another good idea. Here, again, I was trying to match the
> behavior
> > > of the java.net package.
> > >> - Have a method HttpClientConfig.getHttpState() thus the
> > >> credential-based methods could be re-factored to a helper class
> or
> > >> just reside on the DefaultHttpClientConfig.
> > >>
> > > Okay, here I disagree. I really don't like the developer having
> to
> > > know anything about HttpClient in the typical case. Not
> having to
> > > visit the Javadocs for the HttpClient api just makes the backend
> > > easier to use. I think setting credentials will occur quite
> frequently
> > > so I'm a little reluctant to have the user have to manage an
> HttpState
> > > or have to know anything about it to work with the ClientHandler.
> > >
> > > Of course, I'm open to the idea, but I guess I don't understand
> why
> > > you're making this suggestion(?) Is it because you feel it will
> > > address a security issue? Please explain.
> >
> > I am guessing that there may be other ways to configure the
> HttpState
> > beyond the common cases.
> >
> > A way to get or set the HttpState might be useful. A filter could
> > create/use a different HttpState and set it on the properties e.g. i
> > could have credentials set up for a particular WebResource using a
> > filter to add the HttpState to the properties.
> >
> > I was not proposing to remove the methods or for the ClientHandler
> to
> > be utilized. I think they are useful, and maybe so in a different
> > context.
> >
> >
>
> Okay. So long as we keep the simple credential setting methods on
> HttpClientConfig, then I don't have a problem exposing the HttpState
> through a property or by adding a getHttpState() method. Keep in
> mind that getHttpState() and the property PROPERTY_HTTP_STATE
> already exists in DefaultHttpClientConfig -- we'll just need to move
> these over.
>
What about instead if we have a class HttpClientState that
encapsulates HttpState and has those helper methods on it. Then we can
expose a property for HttpClientState and getters/setters. That way it
is more resuable and with a bit of JavaDoc we can easily reference how
to set credentials, for example:
HttpClientConfig.getState().setCredentials(...);
>
> > >
> > >> - I think we need to implement a RequestEntity specific to
> writing
> > >> so that we do not buffer the request.
> > >>
> > > We are in accord on this.
> > >
> > > I'm spread a bit thin these days and won't be able to address
> these
> > > issues until sometime next week.
> > >
> >
> >
> > I will try and do something tomorrow,
> > Paul.
> >
>
> Thank you. Keep me posted, and if there's a chance I'll contribute
> some next week.
>
Some unit tests would be most helpful.
Paul.