On 5/11/11 11:57 AM, Marek Potociar wrote:
>> * Why have a ClientRequest builder? Seems like an unnecessary step. Instead just:
>>
>> ClientRequest req = client.request();
>> req.method("POST")
>> .accept("text/plain")
>> .header("If-Match", "334234324");
>>
>> Less things to import, easier for vendor implementations to extend behavior (instead of having to deal with umpteen
>> different builder classes).
>
> Just to confirm - are you suggesting we remove the ClientRequest.builder() method and replace it with Client.request()
> instead? If so, I like it. Is this also something you were referring to earlier when you mentioned that the Client class
> should be the factory for all client-side objects?
>
Yes, remove any/all builders.
> One thing that I don't like here is that the above suggests that a request is fully mutable. I was hoping to shoot for
> the full immutability instead as it seems to me less error prone and removes lot of complication from the programming
> model making it more straightforward. But I don't have a strong opinion here. If EG thinks that a ClientRequest should
> be fully mutable "builder" like it is in Resteasy, I would be most likely OK with it.
>
Well, you want the ClientRequest to be mutable so that interceptors can
add/remove headers, wrap streams, etc. Even if interceptors didn't
exist, I don't see what immutibility buys you in thi scase.
>> * IMO, a WebResource should strictly be an extension of Client. A place where you can specify a base URL to create
>> requests from, add/override interceptors and filters. I don't like the fact that you specify queryParams and path
>> params on a Web Resource. These methods belong on the client request.
>>
>> So, IMO, it should be something like:
>>
>> interface WebResource implements Client {
>> ...
>> }
>>
>> Or just have a method on Client:
>>
>> public interface Client {
>>
>> Client webResource(String baseURI);
>> }
>
> I am not necessarily ready to agree here (yet). :)
>
I'm not 100% in agreeement with myself yet, but I want to explore this
route to see if we can eliminate some interfaces/classes and reduce the
hierarchy/complexity.
> Client and WebResource should remain separated IMO as they encapsulate different concepts. The idea is that the Client
> is the overall API entry point. It should be customizable& extensible& configurable (in terms of general client-side
> features) and it should provide the access to ANY web resource. That concept is somewhat different from the WebResource.
> Web resource is an accessor for a particular resource identified by it's URI that you can use to invoke HTTP methods on
> the resource or traverse down to a sub resource. We may want to see if we can come up with a better name for this
> concept, but let's try to avoid names like "Proxy" or "ResourceReference".
>
I see your point, but its really not saving much. Having two different
classes to make http invocations seems rather confusing to me.
>> Move queryParam and pathParam to the ClientRequest object.
>
> Let me have a look at that.
>
>> Move get, post, head, etc. to the Client interface:
>>
>> So, a request would look like this:
>>
>> ClientRequest req = client.request();
>> req.method("GET")
>> .accept("text/plain")
>> .queryParam("foo", "bar")
>> .pathParam("x", "y");
>>
>> ClientResponse res = client.execute(req);
>
> I don't understand the connection between moving the get, post etc. and the sample above.
>
client.post(req) would work.
> I think that current proposal is similar in that it lets you build the request instance if you want and then use the
> generic Client.handle(...) method to send it and get back the response. This is the absolute low-level use case and thus
> it is ok to have it available in the Client class.
>
> OTOH, if a user wants to focus on a particular resource (or subset of resources) and use a "fluent" interface to perform
> HTTP method invocations, then WebResource provides that (without any need to work with ClientRequest directly). That's
> also most typical use case.
>
Keep the WebResource, just have it be a request factory.
>> In resteasy, our Client object is basically a factory for requests in which you can set the base URI along with
>> configuration parameters. Execution happens through the ClientRequest:
>>
>> ClientRequest req = client.request("http://foo.com/customers/{id}");
>> req.accept("text/plain")
>> .body("text/plain", "hello world")
>> .header("custom", "header-value)
>> .pathParam("id", 12345);
>>
>> ClientResponse<String> res = req.post(String.class);
>> String str = res.getEntity();
>>
>
> Ad last two lines - why did you decide not to provider the direct access to response entity from the post method?
> String res = req.post(String.class);
We do support that too in our interface.
STring res = req.postTarget(String.class);
vs.
ClientResponse<String> res = req.post(String.class);
My own experiences consuming this API is that I I rarely use the
direct-entity access method.
>
>> With this model, it is also much simpler for the user as they only need to know about 3 interfaces: ClientRequest,
>> ClientResponse, and Client.
>
> Actually 25% simpler to be precise since we just got rid of a single class. And with that extra 25%, the last version of
> my proposal provides also the async api... :)
>
My proposal removes: The build interfaces, WebResource,
WebResourceBase, and a few of the other hierarchy objects you have.
> I could however argue that>90% of the use cases can be satisfied with Client, WebResource and ClientResponse, so the
> amount of classes visible at the user level is the same.
>
> Also the proposed version does not let me write code like this:
> ClientResponse res = client.request().method("GET").post();
>
> The issue with the example above is very obvious, but imagine you instantiate and configure a request in one place and
> then pass it to some closed-source API that performs the actual invocation.
>
That isn't a problem and is fully an implementation detail. Our
ClientRequest implementation actually delegates to a pluggable Executor
interface.
Also, there's no reason you couldn't have a closed-source API perform
the actual invocation.
> I guess the point I take here for now is that we should work on hinding the fluent builder interfaces from the eyes of
> the user as much as possible.
>
I'd say remove builders. I don't really like them, especially
considering neither REsponse nor ClientRequest should be immutable anyways.
>> For a new user, navigating your current class hierarchy is quite daunting and I don't see
>> why we need the current complexity.
>
> Is this comment targeted at the exposed "fluent builder" interfaces or are you also suggesting we should get rid of the
> handlers, filters and other stuff too?
>
Filters are ok, I don't see the difference betweeen handlers and
filters. I want to add reader/writer interceptors. You didn't comment
on that below.
>> * I'd like to stress again, that the Resteasy proxy framework (reusing JAX-RS annotations on the client side) is very
>> popular with our user base. I know it smells like a stub, but I always get asked if this feature will be standardized.
>> That being said, the above model I suggest fits in quite nice when implementing a proxy framework like Resteasy's. The
>> proxy just builds a ClientRequest by extracting @PathParam, @HeaderParam, @QueryParam etc...information from an invoked
>> interface method. Your current WebREsource/ClientRequest hierarchy would not work in this model very well.
>
> I certainly don't want to ruin your proxy framework. We do have a proxy framework in Jersey too, even though it is
> enabling more RESTful, more dynamic and less RPC-like concept of resource views compared to Resteasy proxies. So my
> natural first guess would be that if Jersey can do much more dynamic things with the API, Resteasy should be able to do
> static things with the same API even more easily.
>
> Can you point me to your proxy code so that I can have a closer look at your use case?
>
Well considering that WebREsource doesn't allow you to set headers, etc.
and CLientREquestBuilder doesn't allow to set queryparam or path param,
the proxy framework wouldn't work.
http://resteasy.svn.sourceforge.net/viewvc/resteasy/tags/RESTEASY_JAXRS_2_1_0_GA/resteasy-jaxrs/src/main/java/org/jboss/resteasy/client/core/ClientInvoker.java?revision=1283&view=markup
>> * MessageBodyReader/Writer interceptors
>>
Please review reader/writer interceptors. It is a MUST have. The other
stuff I'm pretty flexible on.
>> Most of the use cases I've found so far involving interception revolves around marshalling and unmarshalling. In
>> Resteasy we have the concept of MessageBodyReader/Writer interceptors that can be used on both the client *AND* server
>> side. It has allowed us reuse code that supports features like:
>>
>> content encoding/decoding (i.e. gzip)
>> digital signatures: signing and verifying
>> message body encryption
>> header decorators
>>
>> It has also allowed us to easily craft new annotations that trigger some of the behavior listed above that work on both
>> the client side (with our proxy framework) and on the server side.
>>
>> Finally, reader/writer interceptors fit seamlessly into an asynchronous model. All the interceptor examples I list
>> above work both synchronous and asynchronously (both on client and server) with zero extra code for the interceptor
>> developer.
>>
>> * It is very brittle and inflexible to have ClientFilters/Handlers be a linked list and have knowledge of the
>> interceptor chain.
>
> I hear you. Let me create a sub task and have a look at it.
>
I'm not sure you'd be able to have filters/interceptors/handlers as
components if they contain a linked list to one another (besides all the
lifecycle issues).
--
Bill Burke
JBoss, a division of Red Hat
http://bill.burkecentral.com