On 4/10/12 6:11 AM, Marek Potociar wrote:
> Hi Bill,
> thanks for the review. Please see my comments inlined.
>
> On Apr 6, 2012, at 3:14 PM, Bill Burke wrote:
>
>> * There is no getProperties() method in Response
>
> My understanding is that properties are for communication between filters and handlers, i.e. properties are part of each single request context but not part of the request or response objects in the context. What is the use case for having the properties available in the response?
>
Well, you may need to pass information to WriterInterceptors. for
example, if you want to sign or encrypt the entity, you might want to
pass through a cryptographic key. I'm also thinking that the MBR and
MBW interfaces need a getProperties() method as you might want to pass
in dynamic configuration through to these interfaces.
>> * HttpHeaders should have appropriate Link methods
>
> IIUC, HttpHeaders are request-only and should remain so. Can request contain link headers?
>
Yes, a request can contain link headers. IIRC, Link headers are entity
headers, not request/response headers.
>> * I don't see why you need all the helper methods in ContainerRequestContext as @HttpHeaders, @UriInfo, etc. can all be injected. Or you could just reference them from within the context.
>
> Since filters are meant to often work with headers, I thought that it's a more user-friendly solution to expose the request scoped header information via the new context API directly, rather than modify the old injectable API. The way I understand it, one of the benefits of the new proposal was the minimized impact on the existing API.
>
You're just duplicating methods. Since ContainerRequestContext is
server-side only, why not use the same server-side constructs that
people are familiar with?
>>
>> i.e.
>>
>> interface ContainerRequestContext {
>>
>> UriInfo getUriInfo();
>> SecurityContext getSecurityContext();
>> HttpHeaders getHttpHeaders();
>> ...
>> }
>
> Got it. Still, IMO this would only work for the container request context and would require quite heavy modification of 1.x HttpHeaders API. From the user perspective it seems more practical to keep all the context interfaces alike. So I prefer keeping the header convenience methods directly in the contexts.
>
Why would it require modifying HttpHeaders? Just keep it the same and
add the mutators to the ContainerRequestContext.
interface ContainerRequestContext {
HttpHeaders getHttpHeaders();
UriInfo getUriInfo();
...
// mutators
MultivaluedMap<String, String> getHeaders();
void setMethod(String m);
void setPath(String path);
...
}
>>
>> * I could give up on my header java.lang.Object value insistence and ClientRequestContext could re-use HttpHeaders too.
>>
>> Here's another idea:
>>
>> - Rename ContainerResponseContext to FilterableContainerResponse
>> - Have FilterableContainerResponse extend Response and add additional mutator methods
>> - Rename ClientResponseContext to FilterableClientResponse'
>> - Have FilterableClientResponse extend Response and add additional mutator methods
>>
>> class FilterableContainerResponse extends Response
>> {
>> public OutputStream getEntityStream();
>>
>> // invoking this method DOES NOT invoke handlers or MBR
>> // must not be null
>> public void setEntityStream(final OutputStream entityStream);
>>
>> // conveniece method for setting new entity
>> public<T> void setEntity(
>> final Class<T> type,
>> final Annotation annotations[],
>> final MediaType mediaType,
>> final T entity);
>>
>> public<T> void setEntity(
>> final GenericType<T> genericType,
>> final Annotation annotations[],
>> final MediaType mediaType,
>> final T entity);
>>
>> // get the declared entity type (used by MBW)
>> public GenericType<?> getDeclaredEntityType();
>>
>> // get the entity annotations (used by MBW)
>> public Annotation[] getEntityAnnotations();
>>
>> ... and any other specific methods to a container response...
>> }
>>
>> public class FilterableClientResponse extends Response {
>>
>> public void setStatusCode(int code);
>>
>> // mutable response headers map
>> public MultivaluedMap<String, String> getHeaders();
>>
>> public boolean hasEntity();
>>
>> // invoking this method DOES NOT invoke handlers or MBR
>> // returns empty stream if no entity
>> public InputStream getEntityStream();
>>
>> // invoking this method DOES NOT invoke handlers or MBW
>> // must not be null
>> public void setEntityStream(final InputStream entityStream);
>>
>> // conveniece method for setting new input stream; DOES invoke handlers& MBW
>> // produced input stream is not buffered by default
>> public<T> void writeEntity(
>> final Class<T> type,
>> final Annotation annotations[],
>> final MediaType mediaType,
>> final T entity);
>>
>> public<T> void writeEntity(
>> final GenericType<T> genericType,
>> final Annotation annotations[],
>> final MediaType mediaType,
>> final T entity);
>>
>> ... and any other specific methods to a client response...
>> }
>>
>
> Interesting idea, but what are the real benefits of that? Firstly, I still think that those interfaces better represent a message context rather than a message object itself. Secondly, how would I produce those objects? I would need to also create specific builders for them, right? Evaluating pros and cons, seems to me the current context-based proposal is superior. It is better aligned with the reality, is purely interface-based and from the end user perspective, the ease of use is same (or perhaps even better). Or perhaps I am missing some important detail you have in mind?
>
I guess I'm just trying to remove duplication and multiple ways of
obtaining the same information. You wouldn't need a builder because the
Response subclasses would have the mutator methods needed. The
container (client or server) would be responsible for creating these
objects. A user would not be able to create them.
Or maybe I'm just not understanding your concern? I don't care that
much, just trying to see if things can be cleaned up more.
--
Bill Burke
JBoss, a division of Red Hat
http://bill.burkecentral.com