-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
> 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.
>
>
Okay, gotcha.
> >
> > > > 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(...);
>
>
>
That sounds reasonable. I'm okay with that.
> >
> > > >
> > > >> - 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.
Can't make promises, but I'll see if I can fit that in.
- -jOrGe W.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)
iEYEARECAAYFAkkmwx0ACgkQ72r0i/n44hOmTgCgnmuxKlQEbNSp744L2Rv67DTH
dPwAn2N1bmWYbGr18o6RVNcMZaVaJuCI
=ovQ0
-----END PGP SIGNATURE-----