On Fri 17 Feb 2012 05:22:29 PM CET, Bill Burke wrote:
> So, getRequest/ResponseBuilder would not return a copy anymore, but a fresh empty one?
C2. remove the getRequest/ResponseBuilder() methods from
FilterContext. => the methods would be removed.
>
> Should copy() be clone() instead?
clone() is defined on Object and has a specific meaning (e.g. it should
return an instance of the same Java type).
Marek
>
> On 2/17/12 8:35 AM, Marek Potociar wrote:
>> Fellow experts,
>>
>> currently the latest version of FilterContext API provides following methods for working with Req/Resp:
>>
>> Request getRequest();
>> Response getResponse();
>>
>> void setRequest(Request req);
>> void setResponse(Response res);
>>
>> Request.RequestBuilder getRequestBuilder();
>> Response.ResponseBuilder getResponseBuilder();
>>
>> Response.ResponseBuilder createResponse();
>>
>> Personally I find the combination of getter+setter and also builder getter confusing. It may lead to assumptions, that
>> following code updates the request headers in the context if value of request Content-length is greater than 1024:
>>
>> if (context.getRequest().getHeaders().getLength()> 1024) {
>> context.getRequestBuilder().header("foo", "bar").build();
>> }
>>
>> ...but it doesn't. The correct version of the code above is:
>>
>> if (context.getRequest().getHeaders().getLength()> 1024) {
>> final RequestBuilder rb = context.getRequestBuilder();
>> rb.header("foo", "bar");
>> context.setRequest(rb.build());
>> }
>>
>> Thus I would propose following changes:
>>
>> C1. introduce a new method in the Request/Response:
>>
>> public Reqest/ResponseBuilder copy();
>>
>> C2. remove the getRequest/ResponseBuilder() methods from FilterContext.
>>
>> The updated version of the correct code from above would then look like:
>>
>> if (context.getRequest().getHeaders().getLength()> 1024) {
>> final RequestBuilder rb = context.getRequest().copy();
>> rb.header("foo", "bar");
>> context.setRequest(rb.build());
>> }
>>
>> Note that the amount of code remains exactly the same, but the code itself much more clearly conveys the fact that we
>> are working with a new copy of the request, not some internal mutable version of the request associated with the request
>> context (although such optimization is, of course, still possible but remains hidden as an internal implementation
>> detail).
>>
>> Additionally, seems to me that createResponse() method is redundant, as static Response factory methods could (and
>> perhaps should) be used instead. Thus I'd like to propose another change:
>>
>> C3. remove the createResponse() method from the FilterContext.
>>
>> Also, I feel we miss a more generic header replacement method in both, Request& Response builders:
>>
>> C4. add replaceAll(MultivaluedMap<String, String> headers) method to Request& Response builder API
>>
>> I'd like to proceed with the proposed changes next week. Let me know if you have any objections or comments by Tuesday
>> Feb 21, CoB.
>>
>> Thanks,
>> Marek
>