|
> - define an interface for processing requests and responses
> - turn AbstractHttpProcessor into DefaultHttpProcessor, make processing methods public > - move default implementation to an implementation package > - HttpRequestExecutor and HttpService can no longer derive from DefaultHttpProcessor, > since it is in an implementation package Makes sense. Wouldn 't HttpInterceptorChain be a better name for this interface, though? > Observation: HttpService is in an API package but imports and instantiates two implementation classes. > Maybe we should reconsider the whole protocol package with respect to API vs. implementation. You are right. This needs to be fixed Oleg Hi Oleg,
> Wouldn 't HttpInterceptorChain be a better name for this interface, though? The interface in the patch makes no reference to interceptors except in the JavaDoc. If we include methods to add/remove interceptors to the interface, I agree. Right now, those methods are only in the implementation class. I think we'll have more of them in the future (like index-based ones), and I am reluctant to add the full selection to the interface. Read-only is enough for both request executor and service. cheers, Roland Fair enough. Please go ahead and commit the patch
Oleg take 1 committed with minor modifications to JavaDocs
I also fixed HttpAsync so that it compiles again. The async fix is minimal and ugly. Things will have to be straightened out later by eliminating AsyncHttpProcessor altogether. The remaining functionality used from HttpRequestExecutor can be provided in AbstractHttpDispatcher instead. cheers, Roland Roland,
I still somehow can't help feeling that HttpProcessor is not a very appropriate name for this interface. Would the following changes be okay with you? Rename: HttpProcessor -> HttpProtocolHandler Rename: DefaultHttpProcessor -> o.a.http.protocol.BasicHttpProtocolHandler Add: class o.a.http.protocol.HttpClientProtocolHandler extends o.a.http.protocol.BasicHttpProtocolHandler { public HttpClientProtocolHandler() { super(); addInterceptor(new RequestContent()); addInterceptor(new RequestTargetHost()); addInterceptor(new RequestConnControl()); addInterceptor(new RequestUserAgent()); addInterceptor(new RequestExpectContinue()); } } Add: class o.a.http.protocol.HttpServerProtocolHandler extends o.a.http.protocol.BasicHttpProtocolHandler { public HttpClientProtocolHandler() { super(); addInterceptor(new ResponseDate()); addInterceptor(new ResponseServer()); addInterceptor(new ResponseContent()); addInterceptor(new ResponseConnControl()); } } Hi Oleg,
I kind of feel that HttpProtocolHandler does not give the slightest indication what the class is supposed to do. I liked the HttpInterceptorChain suggestion better. When I worked on the patch I also considered HttpMessageProcessor. HTTP protocol handling is much more than just setting up or interpreting a few headers. Of course, processing is also a very generic term. Some clicks on WordNet (http://wordnet.princeton.edu/perl/webwn) lead me to "shape" or "mould" as alternatives to indicate that the purpose is to bring a message into a particular form suitable for the following step. HttpMessageShaper? HttpMessageSculptor? ;-) HttpMessageFilter is probably misleading. cheers, Roland All right. As always we agree to disagree. Can we at the very least get rid of the o.a.http.impl.protocol package? In my opinion it makes sense to keep the BasicHttpProcessor class as a part of the public API.
Oleg Hi Oleg,
no problem. You can also change HttpRequestExecutor and HttpService back to subclassing, if you prefer that. I only changed them to use a reference because I moved the implementation to an impl package. cheers, Roland > no problem.
Done. > You can also change HttpRequestExecutor and HttpService back to subclassing, if you prefer that. I actually like it the way it is now. We now have an option to pass a customized (thread-safe, for instance) version of HttpProcessor to HttpRequestExecutor and HttpService. This can certainly come handy. Oleg Hi Oleg,
here are a few names I'd feel comfortable with, in alphabetical order: HttpHeaderHandler HttpHeaderProcessor HttpInterceptHandler HttpInterceptionHandler HttpPrePostProcessor cheers, Roland > HttpHeaderHandler
> HttpHeaderProcessor Interceptors can also re-write entity conent, so these names would be somewhat misleading > HttpInterceptHandler > HttpInterceptionHandler > HttpPrePostProcessor HttpInterceptorChain would be my favourite. But what if we just dropped pre- and post- from method names and kept the interface name? Oleg Hi Oleg,
> HttpInterceptorChain would be my favourite. But what if we just dropped > pre- and post- from method names and kept the interface name? I'm fine with that, too. We could also go one step further and align the method names with those in Http{Request|Response}Interceptor. Then we'd have hierarchical sets of interceptors :-) cheers, Roland > I'm fine with that, too. We could also go one step further and align the method names
> with those in Http{Request|Response}Interceptor. Then we'd have hierarchical sets > of interceptors :-) Sounds like a plan. Oleg > HttpService is in an API package but imports and instantiates two implementation classes.
Fixed. Oleg Is there still anything that needs work? Can this issue be closed?
Oleg |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Minor
AbstractHttpProcessor is not really abstract
- define an interface for processing requests and responses
- turn AbstractHttpProcessor into DefaultHttpProcessor, make processing methods public
- move default implementation to an implementation package
- HttpRequestExecutor and HttpService can no longer derive from DefaultHttpProcessor,
since it is in an implementation package
Observation: HttpService is in an API package but imports and instantiates two implementation classes.
Maybe we should reconsider the whole protocol package with respect to API vs. implementation.
Please let me know what you think.
cheers,
Roland