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