[Freeipa-devel] [PATCH] 0003 batch command can be used to trigger internal errors on server

[Stanislav Laznicka]

On 06/13/2016 10:15 AM, Petr Vobornik wrote:
> On 06/10/2016 06:31 PM, Stanislav Laznicka wrote:
>> On 06/08/2016 02:06 PM, Florence Blanc-Renaud wrote:
>>> On 06/08/2016 10:07 AM, Petr Spacek wrote:
>>>> On 7.6.2016 15:11, Stanislav Laznicka wrote:
>>>>> Hello,
>>>>>
>>>>> Thank you for your patch. As the thin-client patches were pushed in the
>>>>> meantime, the patch won't apply. Could you please send a rebased version?
>>>>>
>>>>> Also, I have a few comments to the patch:
>>>>>
>>>>> 1) I think that the commit message should be rather a brief conclusion to the
>>>>> changes made in the commit. This could help for faster orientation in the
>>>>> changes that were made to a certain part of code should you be searching for a
>>>>> bug introduced by a commit. Should some more info be required, it can be added
>>>>> to the ticket. Could you therefore shorten the commit message?
>>>> (My personal opinion, no golden standard.)
>>>>
>>>> Honestly I disagree with Standa. Yes, the commit message seems to be a bit
>>>> long but *tickets* are not the best place to put *technical* information into.
>>>>
>>>> Tickets are planning tool but keep in mind that Trac may/will vanish one day
>>>> and all we will have will be (Git?) repo.
>>>>
>>>> I would recommend putting the comment about expected input format into code
>>>> comments somewhere around batch command definition.
>>>>
>>>> This would reduce commit message (roughly, the text needs to be adapted) to
>>>> part starting 'The code did not check the format of ' ... which is perfectly
>>>> reasonable description of the change.
>>>>
>>>> IMHO.
>>>> Petr^2 Spacek
>>>>
>>>>
>>>>> 2) Please do not add the tickets to comments in the code. You can use git
>>>>> blame -L or git log -L to see in which commits were the changes introduced to
>>>>> a certain part of a file, these commits should include the ticket number if
>>>>> more info is needed.
>>>>>
>>>>> Standa
>>>>>
>>>>>
>>>>> On 05/27/2016 03:53 PM, Florence Blanc-Renaud wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> the following patch checks the format of parameters passed to a method
>>>>>> called through the batch command. I picked the ConversionError for invalid
>>>>>> parameters format but this choice can be discussed if you have better
>>>>>> suggestions...
>>>>>>
>>>>>> Fixes:https://fedorahosted.org/freeipa/ticket/5810
>>>>>> -- 
>>>>>> Florence Blanc-Renaud
>>>>>> Identity Management Team, Red Hat
>>>>>>
>>>>>>
>>>>>
>>> Hi,
>>>
>>> please find an updated patch version with a less verbose commit msg. I also
>>> removed the reference to ticket # in the code.
>>>
>>> Flo.
>>>
>>>
>> Hello,
>>
>> Thank you for your updated patch. I have just one small issue that maybe exceeds
>> the scope of this ticket. If the check for dictionary instance in list:
>>
>> +            if not isinstance(arg, dict):
>> +                raise errors.ConversionError(
>> +                    name='methods',
>> +                    error=_(u'must contain dict objects'))
>>
>> fails at a further member of the list, by raising an exception, you will lose
>> information about execution of all the previous commands but these were already
>> executed. This hasn't seem to be an issue until now so I wonder if it is a
>> problem or not.
> Right, this is something that we will need to address but not in scope
> of this ticket and probably not 4.4 release.
>
>> So far, batch commands have only been utilized in the WebUI so I am adding Petr
>> for opinions on how to handle this properly so that WebUI could react to it
>> should it ever happen (although AFAIK this should never happen for batch
>> commands called from the UI).
> Web UI doesn't care because it sends it correctly :) The bug is trying
> to use batch command while talking directly to API - e.g. because of
> performance reasons.
>
Thank you for the explanation. ACK, then.