Re: [PATCH] HttpRequest::helperNotes to NotePairs

On 03/19/2013 10:31 PM, Alex Rousskov wrote:
> On 03/19/2013 01:36 PM, Tsantilas Christos wrote:
>> On 03/16/2013 01:52 AM, Alex Rousskov wrote:
>>> 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.
>
>> My question has to do with the way we are interpreting the helpers response.
>> The current code allow helper answers in the form:
>> OK user="\"J, Smith\",chtsanti" user="Alex"
>>
>> This one will store the following notes:
>> user: J, Smith
>> user: chtsanti
>> user: Alex
>>
>> Is it OK?
>
> No, there are only two user values in the above response IMO:
>
> value #1: "J, Smith",chtsanti
> value #2: Alex
>
> The quoted comma should not become a value delimiter -- it is a part of
> the first value.
>
>
>> An alternate method can be to support the following.
>> helper response:
>> OK user="J, Smith",chtsanti user=Alex
>> be stored as the following notes:
>> user: J, Smith
>> user: chtsanti
>> user: Alex
>
> Yes, the above looks correct to me.
>
>
>>>> 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).
>>
>> I did not found....
>
> ConfigParser::QuoteString().

oops.... Although I looked in this function I did not realize that also
checks if quoted needed.

>
>
>
>>>> 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.
>>
>> The %note currenlty is a little strange... We are urlencoding the result
>> before print it so any '"' or other special character will be printed
>> urlencoded.
>> This is the policy we are using for printing headers. I think is not so
>> bad, but maybe the result looks a little strange . The "%{user}note"
>> will print:
>> %22J,%20Smith%22,chtsanti,Alex
>
>
> We need to print values as close to their preferred input form as
> possible. Contexts that do not URL-decode notes, should not URL-encode
> them. Contexts that do URL-decode values, should URL-encode them (unless
> support for URL decoding is deprecated). Otherwise, folks get really
> confused when trying to troubleshoot issues. I have already seen that
> confusion happen in cases where admins were upgrading their helper scripts.
>
> I hope URL decoding of notes is limited to [old] helper responses. Can
> we deprecate that practice? If yes, we should not URL-encode notes when
> reporting, sending, or logging them (by default).

Currently if returned value from a helper is not quoted then considered
as url-encoded.

>
>
>>>> 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.
>
>> If we agree that the current helper response interpreting is OK then the
>> hasByNameListMember is helpful to be used in hasPair.
>> The hasPair currently used in ICAP/eCAP interface to avoid store
>> duplicate values for a key.
>> It can be used in an acl note too...
>
> Does hasByNameListMember() handle quoted commas correctly? If yes, I
> think we should keep using it. If not, I think we should fix it (and
> keep using its API).

Yep. It looks OK.

>
>
> $0.02,
>
> Alex.
>
>
Received on Tue Mar 19 2013 - 22:10:24 MDT