I prefer b)
I think the replace semantic is used elsewhere, but in this case the caller
has full control over both parts and the only difference between then is
lifecycle, so they have full control if they really want replace.
cheers
On 20 August 2015 at 10:40, Shing Wai Chan <shing.wai.chan_at_oracle.com>
wrote:
> On 8/19/15, 1:59 PM, Greg Wilkins wrote:
>
> Ed,
>
> Should we also be explicit about how query string included in setPath and
> one in setQueryString are handled?
>
> I'm assuming that they should be merged and the key difference is that
> setPath is reset each cycle, but the setQueryString is not.
>
> There are two ways of merging for parameters with the same name. (Let us
> take a simple example: path with a=1, query with a=2.)
> a) replace a=2
> b) aggregate, a=1, a=2
>
> It should be (b).
> Any comment?
>
> Shing Wai Chan
>
>
> cheers
>
>
> On 20 August 2015 at 02:52, Edward Burns <edward.burns_at_oracle.com> wrote:
>
>> Thanks for the review and discussion on this. Let's keep the momentum
>> going!
>>
>> >>>>> On Sun, 16 Aug 2015 09:33:19 -0700, Shing Wai Chan <
>> shing.wai.chan_at_oracle.com> said:
>>
>> Spec> "The instance is initialized as follows:
>> Spec> ...
>> Spec> * The query string from HttpServletRequest.getQueryString()
>>
>> Spec> PushBuilder queryString(String queryString) Set the query string
>> Spec> to be used for the push. Will be appended to any query String
>> Spec> included in a call to path(String).
>>
>> SWC> So, this means we can only add more query String, but cannot modify
>> SWC> or remove the query string from the httpServletRequest. Is it an
>> SWC> issue here?
>>
>> Yes, I do think this in an issue.
>>
>> SWC> Do we need APIs to remove or clean up or replace the query string?
>> SWC> (Just consider the case of push a jpg file which may/do not need
>> SWC> query string. Or in some case, we need to "replace" the query
>> SWC> string.) (The same may be for other headers.)
>>
>> >>>>> On Sun, 16 Aug 2015 21:01:39 -0400 (EDT), Stuart Douglas <
>> sdouglas_at_redhat.com> said:
>>
>> SD> This is the query string from the .path() call, not the query string
>> SD> from the original request. I think in this case it makes sense, if
>> SD> you don't want a query string just don't include one in the call to
>> SD> path().
>>
>> No, Shing-wai is correct with the way it is currently worded in the vJetty
>>
>> >>>>> On Mon, 17 Aug 2015 09:48:42 -0700, Shing Wai Chan <
>> shing.wai.chan_at_oracle.com> said:
>>
>> SWC> However, from the javadoc, the queryString is "initialized as
>> follows":
>> SWC> * The query string from HttpServletRequest.getQueryString().
>>
>> SWC> So, this implies that the path() call inherits the query string
>> SWC> from the original request. That is why I ask the question.
>>
>> SWC> To fix the issue, we can do one of the following:
>> SWC> a) remove the initialized for queryString from above
>> SWC> b) add remove/replace queryString API
>>
>> SWC> For the queryString issue, (a) is better. But then we also need to
>> SWC> consider similar issues for headers.
>>
>> The proposal provides for headers:
>>
>> Spec> The existing headers of the current HttpServletRequest are added
>> Spec> to the builder, except for:
>>
>> Spec> Conditional headers (eg. If-Modified-Since)
>> Spec> Range headers
>> Spec> Expect headers
>> Spec> Authorization headers
>> Spec> Referrer headers
>>
>> >>>>> On Mon, 17 Aug 2015 18:52:02 -0400 (EDT), Stuart Douglas <
>> sdouglas_at_redhat.com> said:
>>
>> SD> I don't think that it makes any sense to initialise the query string
>> SD> to be the same as the original request. I think in the majority of
>> SD> cases the new request will have a different query string, so I think
>> SD> your option a) is the way to go.
>>
>> I agree.
>>
>> >>>>> On Tue, 18 Aug 2015 14:57:16 -0700, Shing Wai Chan <
>> shing.wai.chan_at_oracle.com> said:
>>
>> SWC> Besides queryString, other http headers also have a similar issue.
>> SWC> So, we can do one of the following (as in queryString):
>> SWC> a) remove the initialized for headers
>> SWC> b) add removeHeader API
>>
>> I like b) here.
>>
>> >>>>> On Tue, 18 Aug 2015 18:04:59 -0400 (EDT), Stuart Douglas <
>> sdouglas_at_redhat.com> said:
>>
>> SD> I think removeHeader makes sense. Unlike the query string the
>> SD> automatically added headers are likely to be wanted in the pushed
>> SD> response.
>>
>> >>>>> On Wed, 19 Aug 2015 08:52:45 +1000, Greg Wilkins <gregw_at_webtide.com>
>> said:
>>
>> GW> I agree that removing the auto initialization of the query string
>> makes
>> GW> this API clearer.
>>
>> GW> I know there are some apps/frameworks that will have key values in the
>> GW> query that need to be added to every associated push request, but
>> they are
>> GW> likely to be very specific and not generally all the query - this it
>> is
>> GW> best to handle explicitly.
>>
>> I have removed "The query string from
>> HttpServletRequest.getQueryString()".
>>
>> I have added removeHeader().
>>
>> Also, I don't see the point of having both setHeader() and addHeader()
>> so I have removed addHeader() and modified setHeader() to clarify that
>> if there is an existing header with that name, it is overwritten.
>>
>> The vJetty2 revision of the proposal is at [1]. Please review and
>> comment.
>>
>> Thanks,
>>
>> Ed
>>
>> --
>> | edward.burns_at_oracle.com | office: +1 407 458 0017
>> | 54 Business days til JavaOne 2015
>> | 69 Business days til DOAG 2015
>>
>> [1]
>> https://maven.java.net/service/local/repositories/snapshots/archive/javax/servlet/javax.servlet-api/4.0.0-b01-SERVLET_SPEC-134-SNAPSHOT/javax.servlet-api-4.0.0-b01-SERVLET_SPEC-134-20150819.164748-2-javadoc.jar/!/javax/servlet/http/PushBuilder.html
>>
>>
>
>
> --
> Greg Wilkins <gregw@webtide.com> CTO http://webtide.com
>
>
--
Greg Wilkins <gregw@webtide.com> CTO http://webtide.com