Re: Helpers and Notes

On 15/03/2013 6:32 a.m., Tsantilas Christos wrote:
> Hi all,
> I bring back this thread because we have a related project we need to
> close.
>
> Also I believe that the notes interface and usage is currently a little
> broken so I believe we should fix it.
> There are thinks which will never work well if we continue using notes
> like this.
>
> For example assume that a url-rewriter return a note1 note. So assume
> the following configuration:
>
> url_rewrite_program /usr/local/squid/bin/line-helper.sh OK note1=value1
> url_rewrite_children 1
> request_header_add X-Cookie "cookie=%{note1}note"
>
> This is not expected to work, because we are copying the Notes to the
> AccessLogEntry just before logging the request. The formatter gets the
> notes to print from the AccessLogEntry object.
>
> Because the %note needs to print both ICAP/eCAP and helper notes we need
> to find a common way to use Notes, and store ICAP/eCAP and helper notes
> to the same structure.
> This is will help us to access the notes any time we need them, without
> conversions from NotePairs to Notes or the reverse.
>
> In the future we may need to add more squid features which operates on
> both ICAP/eCAP and helpers notes, for example a new ACL.
>
> I am attaching a patch which stores the helper notes to a NotePairs
> class (as ICAP/eCAP does).
> This patch:
> - Converts the HttpRequest::helperNotes to a NotePairs object
> - Adds three members to NotePairs:
> 1) NotePairs::findFirst which return the first matched note for a key
> 2) NotePairs::find which return as value a comma separated list of
> values of the notes matching the key.
> 3) - NotePairs::add to add a key/value pair
> - removes the uneeded methods from Note and Notes classes.
> - Try to hide any references to the HttpHeaders
>
> I do not believe that this patch breaks anything. Moreover I believe
> that this patch makes the code a little faster and better.
>
> Maybe requires a little more testing, but if we decide that it is in the
> right direction I will do it.
>
> Next steps:
> - Remove the AccessLogEntry::notes
> - Rename HttpRequest::helperNotes to HttpRequest::notes
> - Store ICAP/eCAP notes to HttpRequest::notes
> - Probably rewrite the NotePairs to not a new classes which is not
> based on the HttpHeader
> - Rename the Note/Notes/NotePairs classes to avoid confusion in the
> future.
>
> Regards,
> Christos
>
>
> On 01/28/2013 01:51 PM, Tsantilas Christos wrote:
>> To summarize, lets try to see what we currently have and then see what
>> we need.
>>
>> Currently:
>> 1) the helpers may return multiple kv-pairs
>>
>> 2) the helper may use non valid characters for HTTP headers for key
>> names. This is work well now, but we are worrying that in the future it
>> may not work, because the HttpHeader API will be changed.
>>
>> 3) the helper currently can return a quoted value eg group="J, Smith"
>> OR a url encoded value eg group=J%2C%20Smith.
>>
>> 4) the helper may return multiple kv-pairs with the same key
>>
>> 5) In most cases(?) used only the first returned value for a key. For
>> example if multiple "tag=" kv-pairs returned by the helper only the
>> first will be used in tag ACL.
>>
>> 6) If more than one kv-pairs with the same key returned then in
>> access-log we will print them as a list. For example if the helper
>> returned 'OK user="J, Smith" group="group1,A" group=group2' the
>> formating code "%note{group}" will print:
>> "group1,A,group2".
>>
>> 7) In the above helper reply 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.
>>
>>

+1. Passes a quick viaual check so far.

The old add() function had an important comment outlining the merge
behaviour when a key and value already existed in the list. Can you
retain that please. It should be either on add() or on find() now.
Possibly on find() with a reference to use findFirst() when a unique
kv-pair is needed.

Amos
Received on Fri Mar 15 2013 - 01:26:08 MDT