On Apr 10, 2012, at 7:39 PM, Bill Burke wrote:
>
>
> 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.
Interceptors can already access the properties via context.
> 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.
I thought interceptors are here to provide a solution to the dynamic aspect of producing an entity. I'd prefer to keep entity providers static as they are now.
>
>>> * 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 checked it and you are correct. The question is if it is enough to have the helpers in the filter contexts or if we really need to extend the HttpHeaders interface too. If you are still convinced we need to have the support also in the injectable HttpHeaders API, please bring it up on a separate thread and file a new issue for that.
>
>>> * 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 am doing it for the sake of user convenience. There's nothing confusing or unfamiliar about the new methods. My goal is to provide a single mutable context object with convenience accessor methods and I believe such approach is more user friendly than creating a very special "delegating" container request context API completely different from all the other contexts just for the sake of reusing a few existing injectable interfaces on the server request side. Consider a rather common case when one implements the req/resp filter logic as part of a single class. Why should the code in the two methods that works with the same type of data (e.g. message headers) look different?
At the same time I am not forbidding anyone to inject HttpHeaders or UriInfo, even though in case of UriInfo it makes sense to inject it only in the post-matching filters. That's why I don't want to have it in the common (pre/post-matching) context interface.
>
>>>
>>> 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.
Many of the convenience header accessor methods are missing in HttpHeaders. Also as I am writing above, I don't think that we should take the ContainerRequestContext out of the common context API scheme. So -1 on that suggestion.
>
> 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.
So even though there would be a FilterableClientResponse, the client request filter would not be able to produce one?
> 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.
Firstly, we should design filters to either work with contexts or messages. So if we are to make the changes on the response side, we should also make the same changes on the request side (ContainerRequest extends Request).
And then, the conceptual difference between working with a context as opposed to working with a message in a filter is that you may only update the context, but you may completely replace the message. So message-based filter interfaces would need to look like:
public Message filter(Message); // allows complete replacement or wrapping of the message
as opposed to context based:
public void filter(Context); // context cannot be replaced or wrapped
In such case we would also need to agree on what gets injected into the resource as a Request instance - I'd say it would have to be the last instance returned by the request filter chain.
Note that I am not strictly against the above. The one disadvantage I can see as an API designer is that once we take this approach we will not be able to update the involved message interfaces anymore. Never ever. But I guess from the users perspective it doesn't really matter much - both approaches are similar and acceptable.
Marek
>
>
> --
> Bill Burke
> JBoss, a division of Red Hat
> http://bill.burkecentral.com