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?
> * HttpHeaders should have appropriate Link methods
IIUC, HttpHeaders are request-only and should remain so. Can request contain link 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.
>
> 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.
>
> * 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?
Marek
>
> --
> Bill Burke
> JBoss, a division of Red Hat
> http://bill.burkecentral.com