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

[Stanislav Laznicka]

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.

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

Standa
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160610/8abff394/attachment.htm>