Re: [PATCH] HttpRequest::helperNotes to NotePairs

The 5th version of the patch.
This patch handles Amos comments.

Regards,
   Christos

On 04/07/2013 04:54 PM, Tsantilas Christos wrote:
> On 04/07/2013 07:50 AM, Amos Jeffries wrote:
>> On 6/04/2013 3:27 a.m., Tsantilas Christos wrote:
>>> This patch try to fix current current Notes interface and usage.
>>>
>>> The changes done having in mind that we need:
>>> 1) to add multiple notes with the same key
>>> 2) to support 3 different note types: adaptation meta headers, helper
>>> notes and custom notes added by the system administrator
>>> 3) to log notes using the %note formatting code
>>> 4) to use the %note formating code everywhere the formating API is used.
>>> For example use the %note with the request_header_add configuration
>>> parameter.
>>> 5) to use notes with ACLs.
>>>
>>>
>>> The NotePairs class is implemented from scratch. It stores the notes in
>>> a simple key:value pairs list, which just allow multiple entries with
>>> the same key. I have an implementation which uses a
>>> "key:value1,value2,value3..." form but my sense is that the
>>> implementation I am posting here is simpler and faster.
>>>
>>> - The %{key}notes prints a coma separated list of note values which have
>>> the key as key. Eg the %{user}note will print:
>>> "J, Smith",chtsanti
>>>
>>> - The %notes prints a "\r\n" separated list of key:value pairs:
>>> user: "J, Smith"\r\nuser: chtsanti\r\n
>>>
>>> - The %notes formating code can be used now with request_header_add and
>>> other similar configuration parameters.
>>>
>>> - The Helper response may needs fixing. For example if the helper return:
>>> user="J, Smith",chtsanti
>>> The user:"J, Smith,chtsanti" note will be stored. This is because of
>>> the strwordtok function used to parse responses.
>>>
>>> - We may need to merge the AccessLogEntry::notes and
>>> AccessLogEntry::helperNotes/HttpRequest::helperNotes to the same object.
>>> The adaptation meta headers maybe can be merged to the same object to.
>>>
>>> Some details also exist in the patch
>>>
>>>
>>> Regards,
>>> Christos
>>
>> Hi Christos, thank you for this.
>>
>> in src/AccessLogEntry.h:
>>
>> * If AccessLogEntry::notes and AccessLogEntry::helperNotes are really
>> necessary to be separate, please name the *::notes on configNotes or
>> something more descriptive of what it holds.
>> - Also for my knowledge, can you explain why they are now separate and
>> in need of re-combining? they started out as a combined lis
>
> I think there is not a reason to have them separated.
> The adaptation and helpers notes was separated before this patch, and
> just merged before logged to log file.
> Maybe there are reasons for this so I let it for a future decision.
>
>
>>
>>
>> in src/ConfigParser.* :
>>
>> * ConfigParser::QuoteString const-correctness seems unrelated. Please
>> feel free to apply this change by itself immediately.
>
> OK, I will commit with a separate patch.
>
>>
>>
>> in src/HelperReply.cc:
>>
>> * operator<< line if(!r.notes.empty() > 0) - is very obscure. Please
>> alter the condition to clarify what exactly is being tested.
>> + Probably the " > 0" is not meant to be there.
>
> Oops!
> Well it works :-) as is, but of course it is a mistake
>
>
>>
>> * please avoid adding new empty line at the end of file
> OK.
>
>>
>>
>> in src/HttpRequest.cc
>> * we no longer need to check for helperNotes != NULL before setting it
>> to NULL. Just set.
>> - same on the copy()
>
> OK.
>
>>
>>
>> in src/Notes.cc
>> * Notes::add(const String &noteKey) - should be able to be
>> const_iterator still.
>
> Nope. it will not work....
>
>>
>>
>> NP: thats all I've had time to check for today. More in a few days.
>
> I will wait more comments before post a new patch
>
> Regards,
> Christos
>
>>
>> Amos
>>
>
>