Great that you are taking on this work. I have both general and specific comments.
General:
* I share other's concerns with restructuring too much. At the risk
of causing you much pain (since I'm not the one implementing!),
how about the following: Create a new interface
HttpMethodWithRedirect, defining getDoAuthentication(),
setDoAuthentication(), execute(), getFollowRedirects(), and
setFollowRedirects(), all of which are deprecated, and remove(!)
the functions from HttpMethod. Have HttpMethodBase implement this
new interface. Add a new function to HttpMethod, something like
"performRequestAndResponse()", which corresponds to your new model
for the "execute" function. Old clients, using the existing
method implementations that extend HttpMethodBase, could still get
away with calling "execute()" on the method, although this would
now be a deprecated use. HttpClient.executeMethod() would always
invoke performRequestAndResponse(). My take is that the above
changes, while theoretically breaking anything that implements
HttpMethod without extending HttpMethodBase, I don't see as
causing much of a problem, because likely nobody is successfully
implementing HttpMethod outside of HttpMethodBase, seeing as we've
had a lot of trouble getting it right.
* Move the functionality that you've moved from HttpMethodBase to
HttpClient to yet another new, package level class, perhaps with
all static functions. Your new HttpClient and HttpMethodBase
could call the shared implementation.
* Seems like you need a function on HttpMethod called something like
"canFollowRedirects()" that corresponds to what the unchanged
EntityEnclosingMethod does with setFollowRedirects() - namely says
you must not follow them.
* HttpMethod.executeMethod() now suffers from the same problem that
HttpMethodBase.execute() use to have. Some of the miscellaneous
activity in this function ought to be extracted into separate
routines. I suppose you can pass the buck on that one though.
* I worry that your new changes are not going to handle proxies
correctly, in weird cases with redirects. The "execute" method of
ConnectMethod does an execute on its "wrapped" method. Not sure
how your changes would match existing functionality, and off the
top of my head, I cannot think of any obvious ways to restructure it.Specific:
* HttpClient.getDefaultPort() - Protocol.getDefaultPort() would be
appropriate to invoke here instead, I think.
* HttpMethod.getFollowRedirects() - deprecate as well? This falls
under my other comments.As for your question about connection usage, my perspective is that your changes are for the better. Right now, on a redirect, HttpMethodBase keeps the same connection (if it is persistent), and does the redirect or authentication. It seems to me that it would be a better model to return the connection to the pool and get a new one, if only for the architectural clarity of processing a single "request/response" pair (with a possible 100 Continue thrown in) with a connection, and then "releasing" the connection.
Hope my comments help.
-Elric.
Oleg Kalnichevski wrote:
Folks
I am currently working on a patch enabling HttpClient to handle
cross-site redirects.
http://nagoya.apache.org/bugzilla/show_bug.cgi?id=16729
In order to lay foundation for this capability I needed to make quite a
few changes to HttpClient & HttpMethodBase. I have opted for a more
substantial overhaul of these classes, than was strictly necessary. I
realize not all of you may agree with my decision. So, I decided to seek
an early feedback from you to make sure I do not go completely astray.
This is what I have done:
I moved complete redirect & authenticate logic from HttpMethodBase to HttpClient. HttpMethodBase
Impact:
- Even though binary interface is unchanged, HttpClient's modus operandi
with regard to redirect & authentication changed substantially. People
like Laura Werner,who do not use standard HttpClient and have developed
their own logic around lower level classes will be affected most.
- Cleaner design. Redirect & authentication in my opinion logically do not belong to domain of the HTTP method, rather, they belong to that of the HTTP agent.
- Over-convoluted HttpMethodBase class got simpler. Under-used HttpClient class is leveraged more. This is an important architectural improvement in my humble opinion. If you disagree, please let me know
- I am seriously concerned that this redesign may have adversely affected connection pooling stuff. Mike, Eric, you are the connection pooling experts, could you please give me your opinion on that?
- About a dozen of test cases have become obsolete. They will need to be redesigned. They are all commented out for the time being
As always, any feedback, including that in a form of bad tomatoes thrown at me will be appreciated
Please note, that cross-site redirect has not been implemented yet.
Cheers
Oleg
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
