[Freeipa-devel] [PATCH] Fix File parameter validation when prompting.

[Pavel Zuna]

John Dennis wrote:
> On 01/28/2010 06:56 PM, Jason Gerard DeRose wrote:
>> On Wed, 2010-01-27 at 17:53 +0100, Pavel Zuna wrote:
>>> cli.prompt_interactively now loads files before validating the 
>>> parameter value.
>>> It also populates a list of already loaded files, so that 
>>> cli.load_files knows
>>> when a parameter already contains the file contents.
>>>
>>> Fix #557163
>>>
>>> Pavel
>>
>> ack.
>>
>> This looks reasonable to me, but I'd really like you to add some tests
>> for this, especially testing that it works correctly for a command with
>> multiple File params.
>>
>> Rob and John, do you see any problems with this approach?  Does this
>> address the needs of the cert plugins?
> 
> nck
> 
> Maybe I'm reading the code wrong but it seems like there are cases where 
> the already_loaded_files list is not updated. If load_files() is called 
> then the file is loaded but already_loaded_files is not updated to 
> reflect that. Shouldn't the function load_file() update the 
> already_loaded_files list to assure already_loaded_files is always in 
> sync? Also, wouldn't already_loaded_files be better implemented as a set 
> rather than a list?
The already_loaded_files list is only updated when the file was loaded, its 
contents were validated and stored in kw[param.name] (this is later passed to 
the command being executed).

It isn't updated in load_file, because this method can be called several times 
for each File parameter (for example when a user has really fat fingers and 
can't type properly :)).

If we did update the list in load_file, then we would have to make 
already_loaded_files a set, but since we don't and we iterate over parameters in 
a way that guarantees that no parameter is iterated over twice, we're fine with 
using the list type.

> Aside from the above state consistency issue I think it fixes the 
> problem, but I'll be honest I'm not in love with it because it's kind of 
> a hack to get around deficiencies in the command parameter framework. I 
> always get nervous when I see special case exceptions surgically 
> inserted into select pieces of code rather than conforming to a regular 
> and consistent framework.
I'm not in love with it either. There's this girl at school I like and... :D

Seriously, you could call it a hack, but as you said yourself in your second 
post at https://bugzilla.redhat.com/show_bug.cgi?id=557163:

"I just don't see anyway to support the concept of File parameters using the 
existing parameter mechanism without a lot of special case hacks spread through 
the code."

This "hack", while not the most elegant (I'm all about code elegance), only 
lives in one place, is pretty short and understandable. Also, I think the place 
this lives in is actually the right one. Loading files is going to be different 
in the webUI and CLI, so it makes sense to implement it in UI specific classes.

I'll elaborate more about the command parameter framework in a response to your 
second email.

> Also this only works because the cli object is short lived and single 
> use. If that assumption is ever violated the persistent state in the 
> object will present a consistency problem.
Yes, IF. But it's never going happen unless we rewrite pretty much everything - 
that's just how the cli class was designed. Also, I don't know what persistent 
state you're talking about and how is it related to the already_loaded_files 
list - it is a short lived local variable.

> Bottom line, I'll ack it if the first issue is resolved or Pavel 
> explains why load_files() can't produce an inconsistent state.
> 

Pavel