Re: [PATCH] HttpRequest::helperNotes to NotePairs

On 03/15/2013 05:18 PM, Amos Jeffries wrote:
> On 16/03/2013 7:11 a.m., Tsantilas Christos wrote:
>> This patch converts the HttpRequest::helperNotes to a NotePairs object
>> This patch is similar with the one posted under the "Helper and Notes"
>> thread and just add a new method:
>> NotePairs::hasPair to check if a key/value pair is stored
>>
>>
>> We need to discuss, how to handle multiple notes with the same key.
>> Assume that you have the following answer from a helper:
>> OK user="J, Smith" group="group1,A" group=group2
>>
>> Currently (with or without this patch), formating code "%note{group}"
>> will print:
>> "group1,A,group2"
>>
>> The formating code "%note" will print:
>> "user: J, Smith\r\ngroup: group1,A\r\ngroup: group2"
>> I must note here that the two "group" kv-pair keys will be printed as
>> separate entries.
>>
>> How should we handle this case? Is it OK as is?
>>
>> We need to decide if the ',' will be used as separator or as valid
>> values character.
>
> Both?

Yes, I think both is the right approach here: Bare comma is a separator
(or an invalid character in some contexts), but quoted comma is a valid
character. This is similar to the space character (' ').

> IMO when we are printing the syntax key '=' value, it makes a lot of
> sense to use the HTTP ABNF format which uses both encoded token or
> 'bare' quoted-string ("...") for any value with reserved characters such
> as '',' or ':' in our notes.

And I think we already have code somewhere that smartly adds quotes to
values that should be quoted while not quoting simple tokens (a common
case).

> The simple way to do that is to just
> quoted-string all values on %note and print as duplicate keys in both
> formats.

It is better not to over-quote for performance and aesthetic reasons, I
think. It also helps to match what folks write in squid.conf with what
Squid shows in logs. If a token does not contain special characters,
let's not quote it. A comma can be declared a special character, of course.

> Please don't implement hasPair using hasByNameListMember(). The whole
> set of HttpHeader 'strList' manipulator functions are rather nasty in
> the way they build the headers into strings before re-parsing the
> values. They also take a side-trip through the HTTP reserved headers
> matching which is completely unnecessary for Notes.
> The simple loops you have for find() should work a lot faster despite
> the string comparision on each key.

If the existing API does what we need, should not we use it unless its
speed makes it really unacceptable? IMO, this does not sound like such a
critical code path that we should not use an existing API.

Thank you,

Alex.
Received on Fri Mar 15 2013 - 23:52:28 MDT