For PushBuilder we are building a http request (for the push promise frame
sent to the
client and for the inbound request to the servlet engine). In terms of RFC
7232 it is up to
the recipient of the request to determine if the conditional headers
should be ignored and
therefore to me it is out of place when building the push request.
Basically if the request
includes a conditional header it is, by definition conditional. Also
requiring the caller to make
two method calls (one to set conditional and a second to set the header)
to make a request
conditional is unnecessary.
Also it seems odd to pick two conditional headers which we provide
specifiic methods for, when
there are others which we do not. .
So my vote is:
1. Remove the eTag and LastModified methods (or renamed counterparts) in
favor of setHeader and addHeader
2. remove the conditional and isConditional methods.
Also a question, if setHeader is called for a header which is currently
included more that once in the request does
the implementation remove all previous occurrences and replace them with
the new value?
Thank you,
Martin Mulholland.
WebSphere Application Server Web Tier Architect
email: mmulholl_at_us.ibm.com
Shing Wai Chan <shing.wai.chan_at_oracle.com> wrote on 03/10/2017 02:05:07
PM:
> From: Shing Wai Chan <shing.wai.chan_at_oracle.com>
> To: jsr369-experts_at_servlet-spec.java.net
> Date: 03/10/2017 02:06 PM
> Subject: [servlet-spec users] [jsr369-experts] Re: Re: PushBuilder
> with conditional headers
>
> My goal is to adequately support RFC 7232 while keeping the spec as
> tight as possible.
> The current spec leaves some ambiguities on what to do about
> conditional headers on the
> #{add,set,get,remove}Header methods:
>
> Should conditional headers be ignored when passed to those methods?
>
> Should they only be ignored if conditional(true) has been called?
>
> What happens if conditional(true) is called, some headers are added,
> and then conditional(false) is called?
>
> There are probably other questions.
>
> We would have to explicitly address these questions in the spec for all
> of those methods. That's a lot of complexities.
>
> Instead, I suggest we remove #ifNoneMatch and #ifModifiedSince (formerly
> #eTag and #lastModified) and allow #{add,set,get,remove}Header to
> accept any kind of
> header, conditional or not. Then we make a blanket statement in the
> PushBuilder class JavaDoc:
>
> Note that any conditional headers as specified in RFC 7232 must be
> stripped out of the
> push request when push() is called, unless conditional(true) is
> called first.
>
> Any comment?
> Shing Wai Chan
>
> On Mar 9, 2017, at 11:49 AM, Martin Mulholland <mmulholl_at_us.ibm.com>
wrote:
>
> To clarify the doc I have for pushBuilder says;
>
> Push a resource based on the current state of the PushBuilder. If
> isConditional()is true and an etag or lastModified value is
> provided, then an appropriate conditional header will be generated.
> If both an etag and lastModified value are provided only an If-None-
> Match header will be generated.
>
> so treating the methods to set eTag and lastModified as the
> equivalent of setHeader methods would appear to render the setting
> of conditional irrelevant. Do we even need it conditional api's if
> we can deduce such from the header settings?
>
> Thank you,
> Martin Mulholland.
> WebSphere Application Server Web Tier Architect
> email: mmulholl_at_us.ibm.com
>
> Greg Wilkins <gregw_at_webtide.com> wrote on 03/08/2017 08:16:16 PM:
>
> > From: Greg Wilkins <gregw_at_webtide.com>
> > To: jsr369-experts_at_servlet-spec.java.net
> > Date: 03/08/2017 08:17 PM
> > Subject: [servlet-spec users] [jsr369-experts] Re: PushBuilder with
> > conditional headers
> >
> > Shing,
> >
> > I think it would make sense to have
> > pushBuilder.setHeader(“If-None-Match”, value)
> > be identical to:
> > pushBuilder.ifNoneMatch(value)
> >
> > and for
> > pushBuilder.addHeader(“If-None-Match”, value)
> > be identical to:
> > String prev = pushBuilder.getHeader(“If-None-Match”);
> > pushBuilder.ifNoneMatch(prev==null?value:(prev+", "+value);
> >
> > regards
> >
> > On 7 March 2017 at 08:50, Shing Wai Chan <shing.wai.chan_at_oracle.com>
wrote:
> > This email continues the previous discussion [1] of conditional
> > headers mentioned in RFC 2732, but not in PushBuilder.
> >
> > In PushBuilder, we have add/set header through
> > #addHeader(String name, String value)
> > #setHeader(String name, String value)
> >
> > Also, we have the following
> > i) #eTag(String eTag), #lastModified(String lastModified)
> > (Per previous discussion in EG, we will rename them as
> > #ifNoneMatch(String), #ifModifiedSince(String). We will use the old
> > name in the following discussion.)
> > ii) #conditional(boolean)
> > So if #eTag is invoked with #conditional(false), then the eTag
> > header will not be added.
> >
> > Suppose we want to add the conditional header:
> > If-None-Match: ”xyzzy”
> > Then we can do the following (A):
> > pushBuilder.eTag(“xyzzy”); (A1)
> > pushBuilder.conditional(true); (A2)
> >
> > What is the behavior of invoking (B):
> > pushBuilder.add(“If-None-Match”, “xyzzy”)
> >
> > a) (B) = (A1)
> > But this means that #addHeader will not actually add a
> > conditional header.
> > b) (B) = (A1) + (A2)
> > This is problematic here because (A2) is for all conditional
> > headers, not just “If-None-Match”.
> > c) (B) = adding If-None-Match: “xyzzy” independently of #conditional
> > This means #conditional(false) will not take effect for those
> > conditional header adding through #addHeader.
> > d) (B) is illegal, which will throw IllegalArgumentException for any
> > conditional headers.
> > If we go this way, then we need to add API for the other three
> > conditional headers.
> >
> > All the above are not quite satisfactory.
> >
> > Do we still want to keep #eTag, #lastModified, #conditional et al
> > API for conditional headers?
> >
> > Any comment?
> >
> > Shing Wai Chan
> >
> > ps.
> > [1] https://java.net/projects/servlet-spec/lists/jsr369-experts/
> > archive/2017-03/message/3
> >
>
> >
> > --
> > Greg Wilkins <gregw@webtide.com> CTO http://webtide.com