Re: [PATCH] HttpRequest::helperNotes to NotePairs

On 04/28/2013 01:20 PM, Tsantilas Christos wrote:
> On 04/16/2013 06:42 PM, Tsantilas Christos wrote:
>> The 5th version of the patch.
>> This patch handles Amos comments.
>
> If there is not any objection I will commit the latest patch to trunk.

applied to trunk

>
> Regards,
> Christos
>
>
>>
>> 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
>>>>
>>>
>>>
>>
>
>
Received on Mon Apr 29 2013 - 13:34:49 MDT