Re: [PATCH] HttpRequest::helperNotes to NotePairs

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.
>>
>> 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 (' ').

OK. I agree on this.
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?

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

In both cases the patch requires a small modification in
NotePairs::findFirst method to return the first in the list if more than
one members/values exist in an note entry.

>
>
>> 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....

>
>
>> 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

>
>
>> 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...

Regards,
   Christos

>
>
> Thank you,
>
> Alex.
>
>
Received on Tue Mar 19 2013 - 19:37:03 MDT