Re: cookie2 code review and suggestions

[Oleg Kalnichevski <ol...@apache.org>]

Hi Samit,

I just committed the cookie attribute handler patch. Feel free to review
changes made to the RFC2965 spec and suggest modifications you deem
necessary. In particular see if CookieSource is a good name for this
class. I am not a native English speaker and just could not think of a
better name.

On my part I would like to suggest some further changes to the
CookieAttributeHandler interface. I am not entirely sure
CookieAttributeHandler#format(StringBuffer, Cookie) method brings much.
In essence all cookie attribute handlers implement exactly the same
logic and simply delegate the job to the ParameterFormatter class. I can
hardly think of any reasons to override the #format method for any
individual attributes (custom expiry date formatting might be one, but
this attribute should no longer be used). It seems this extra level of
abstraction adds more complexity without any tangible benefits. The fact
that CookieAttributeHandler#format cannot be used polymorphically
highlights the problem with this method. I suggest we do away with it. 

What do you think?

Oleg
PS: There are more things I would like to discuss, but let us tackle
issues one at a time


On Tue, 2005-08-30 at 08:05 +0000, Samit Jain wrote:
> Hi everyone,
> 
> I would really appreciate if you could review the code for RFC 2965 
> implementation and give your valuable comments. Especially, there are some 
> TODO items in RFC2965Spec class which still need to be resolved (listed 
> below):
> 
> //TODO(jain): when do we consider the header malformed and stop processing 
> it? parse()
> Presently if there is any error in parsing cookie attributes, we ignore the 
> cookie and process other cookies in header. The response header is 
> considered malformed entirely only when the header name/value is null. Are 
> there other cases while creating the cookie or parsing cookie attributes 
> that we consider the header malformed and stop processing the header, e.g. 
> header value too long, or contains too many illegal characters, etc. The RFC 
> doesn't really expound on this besides stating the fact that unknown cookie 
> attributes should be ignored.
> 
> //TODO (jain): formatting old and new style cookies in the same header 
> formatCookies()
> We had a separate thread for this issue. We came to the conclusion that 
> cookies in the same header should be formatted similarly. Moreover, if 
> cookies to be formatted include new and old style cookies, we format as per 
> the old specification. So this is a closed issue.
> 
> //TODO (jain): how do we handle the case when domain is specified and equals 
> host? Cookie2DomainAttributeHandler.parse()
> Just want to make sure that if the domain is specified in the set-cookie2 
> header, it cannot be the same as the request host.
> 
> //TODO (jain): other versions for set-cookie2 ?
> Currently we only allow version 1 for set-cookie2 cookies. Is this correct?
> 
> cheers,
> 
> Samit


---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org

Re: cookie2 code review and suggestions

[Samit Jain <ja...@gmail.com>]

Hi Oleg,

I just committed the cookie attribute handler patch. Feel free to review
> changes made to the RFC2965 spec and suggest modifications you deem
> necessary. In particular see if CookieSource is a good name for this
> class. I am not a native English speaker and just could not think of a
> better name.



I was myself thinking of a good name but could not come up with something 
better than CookieSource. May be CookieData :)

On my part I would like to suggest some further changes to the
> CookieAttributeHandler interface. I am not entirely sure
> CookieAttributeHandler#format(StringBuffer, Cookie) method brings much.
> In essence all cookie attribute handlers implement exactly the same
> logic and simply delegate the job to the ParameterFormatter class. I can
> hardly think of any reasons to override the #format method for any
> individual attributes (custom expiry date formatting might be one, but
> this attribute should no longer be used). It seems this extra level of
> abstraction adds more complexity without any tangible benefits. The fact
> that CookieAttributeHandler#format cannot be used polymorphically
> highlights the problem with this method. I suggest we do away with it.



I think you are right. CookieAttributeHandler#format doesn't really bring 
much presently. The only reason I had it in there is for other specs that 
might require more complex cookie formatting and to contain changes in RFC 
2965 cookie formatting in one place. I will work on its removal and clean up 
some other stuff. May be we can keep this method in CookieAttributehandler 
but have an empty implementation for it in RFC 2965. I will also be adding 
cookieMatch(Cookie, Cookie) method for different specs in the same 
changelist. I should be done with it by tomorrow.

thanks for your comments.

Samit.


> 
> On Tue, 2005-08-30 at 08:05 +0000, Samit Jain wrote:
> > Hi everyone,
> >
> > I would really appreciate if you could review the code for RFC 2965
> > implementation and give your valuable comments. Especially, there are 
> some
> > TODO items in RFC2965Spec class which still need to be resolved (listed
> > below):
> >
> > //TODO(jain): when do we consider the header malformed and stop 
> processing
> > it? parse()
> > Presently if there is any error in parsing cookie attributes, we ignore 
> the
> > cookie and process other cookies in header. The response header is
> > considered malformed entirely only when the header name/value is null. 
> Are
> > there other cases while creating the cookie or parsing cookie attributes
> > that we consider the header malformed and stop processing the header, 
> e.g.
> > header value too long, or contains too many illegal characters, etc. The 
> RFC
> > doesn't really expound on this besides stating the fact that unknown 
> cookie
> > attributes should be ignored.
> >
> > //TODO (jain): formatting old and new style cookies in the same header
> > formatCookies()
> > We had a separate thread for this issue. We came to the conclusion that
> > cookies in the same header should be formatted similarly. Moreover, if
> > cookies to be formatted include new and old style cookies, we format as 
> per
> > the old specification. So this is a closed issue.
> >
> > //TODO (jain): how do we handle the case when domain is specified and 
> equals
> > host? Cookie2DomainAttributeHandler.parse()
> > Just want to make sure that if the domain is specified in the 
> set-cookie2
> > header, it cannot be the same as the request host.
> >
> > //TODO (jain): other versions for set-cookie2 ?
> > Currently we only allow version 1 for set-cookie2 cookies. Is this 
> correct?
> >
> > cheers,
> >
> > Samit
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
> 
>