On 8 December 2014 at 00:43, Stuart Douglas <sdouglas_at_redhat.com> wrote:
>
>
> Greg Wilkins wrote:
>
>>
>> I've been working with Push a little bit more and have an alternative
>> proposal for the API.
>> ...
>>
>
> What is the reasoning behind copying the query string? Shouldn't it be set
> from the push URL?
>
>
Query string are used in many a various ways. Sometimes they are used to
identify the resource, in which case it should not be taken from the
original request, but passed in the push URI. Other times they are used to
indicate information about context, client, language, variations etc. in
which case they should be copied from the original request (eg if the
original had ?lang=it, then all associated push requests should probably
also have ?lang=it)
So we need a flexible API that captures these two styles and probably an
infinite number of variations.
> <snip>
>
>
>> The PushBuilder itself allows most of the initialisation to be
>> overridden if need be, but hopefully it wont:
>>
>> public interface PushBuilder
>> {
>> public String getMethod();
>> public void setMethod(String method);
>>
>
> From a users point of view it would be good if all these methods returned
> the PushBuilder, so you can chain the calls together (i.e. make it a fluent
> API). On the flip side none of the rest of the API works like this, so I
> guess it is a trade off between consistency and ease of use.
>
That is a good pattern used for Builders, so I think it is OK for us to
include as there are no other Builders in the servlet API. I almost
called it a PushContext, in which case that pattern would clash... so kind
of justifies the choice of name as a Builder :)
>
>
>> public Enumeration<String> getHeaderNames();
>>
>
> What about Set<String> instead? Enumeration is a pretty yuck API, although
> once again I guess it is a trade off between being consistent with the
> existing HttpServletRequest API.
>
>
In my current version I have this as Collection<String>.... but as
duplicates are not allowed, I guess Set<String> is more correct. I think
we can move away from old style APIs in new API.
> public String getHeader(String name);
>> public void setHeader(String name,String value);
>>
>
> I this we should include all the header manipulation methods that are
> present on HttpServletRequest as well. At the very least I think we should
> support multiple header values (getHeaders() and addHeader()), and for
> completeness we should probably also have getDateHeader etc.
>
+1
>
>> public String getQueryString();
>> public void setQueryString(String query);
>>
>
> I don't know if we need this, I would expect that it would just be parsed
> from the push URI. The way this API is currently described I think it is
> not very intuitive, e.g. if I am at /story?id=3 and I push the URL
> /images?id=6 I will end up with /images?id=3, which is not really what I
> would expect.
>
> This is setting the query string to be used on all invocations of push.
In the chronic case, we may be pushing many 10s of associated resources and
they all might need lang=es to be set. They may also each need an
individual id=n to be set, but that is the part that is passed in which
each call to push.
>
> I think we should also just have a isPushSupported() method on the
> request, so code can check if push is supported without creating a builder.
>
>
I was proposing a null return.... but then I do hate having to do null
checks. The other option is to return a Noop PushBuilder, so the code is
not written conditionally, but just is a noop of push is not supported.
Eitherway, having isPushSupported() is a reasonable addition.
Maybe also have an isPush() method so we know that the request is the
result of a push (or is this a DispatcherType), as we may need something
like that to avoid push loops.
>
>>
>>
>
> For the most part I like the idea.
>
>
Great! I have it mostly working now for a PushCacheFilter approach. I'd
be interested to hear how it works for a more framework centric impl of
push (eg JSF pushing known associates rather than cached associations).
I'll incorporate the feedback received and post an updated version later
today or tomorrow.
cheers
--
Greg Wilkins <gregw_at_intalio.com> @ Webtide - *an Intalio subsidiary*
http://eclipse.org/jetty HTTP, SPDY, Websocket server and client that scales
http://www.webtide.com advice and support for jetty and cometd.