On 05/11/2011 07:43 PM, Bill Burke wrote:
> On 5/11/11 11:57 AM, Marek Potociar wrote:
>> 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.
>
OK, you're right, let me try to update the proposal. I have closed the task #79 and created a new task #95.
>
>>> * 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.
WebResource is the first class, what is the second class? Client with it's Client.handle()? But that's not very
different from your counterproposal.
>
>>> Move queryParam and pathParam to the ClientRequest object.
>>
>> Let me have a look at that.
Added comment to the subtask #95 to cover the requirement above.
>>
>>> 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.
As I said already, I find it very confusing that you can set one method on a request and then use a different method to
invoke that request. Such API seems inconsistent and confusing and requires us to address the expected implementation
behavior in all such conflicting cases in the spec.
We can resolve this by a fluent API where such conflicting cases are autmatically eliminated (e.g. Google Guice Binder
EDSL:
http://google-guice.googlecode.com/svn/trunk/javadoc/com/google/inject/Binder.html). Sure, such API comes at
expense of some interface or class type proliferation, but important fact to note is that the vast majority of those
builder interfaces are virtually invisible to the end user.
>> 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.
Added as a subtask #96 - I don't have any objection to supporting a request creation on a web resource.
>
>>> 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.
>
I see. But do you have any objections to keep the overloaded versions as they are proposed now?
>> 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.
That sounds like the proposed ClientHandler interface. I don't have any strong opinion on renaming that to e.g.
RequestExecutor or something like that. Feel free to suggest a name.
> Also, there's no reason you couldn't have a closed-source API perform the actual invocation.
Not sure I am following here, maybe I lost the context. Can you elaborate?
>>> 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.
Handler is meant to be more like your executor probably. Filter would be more like a generic request/response interceptor.
> I want to add reader/writer interceptors.
> You didn't comment on that below.
>
Actually I did at the very end - I want to create a task and I need to have a closer look. But from the top of my head,
the reader/writer interceptors make sense to me. Also unified server/client side interceptor model makes a lot of sense
to me too.
I have added tasks #97 and #98 to cover these topics
>
>>> * 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.
All right, all right... :) (see #98)
>
>
>
>>> 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).
I have added another task #99 for this.
Marek