[Freeipa-devel] Command instantiation

[Jan Cholasta]

On 14.1.2013 18:50, Petr Viktorin wrote:
> On 01/14/2013 06:31 PM, Alexander Bokovoy wrote:
>> On Mon, 14 Jan 2013, Jan Cholasta wrote:
>>> On 14.1.2013 17:06, Petr Viktorin wrote:
>>>>
>>>> IPA Command objects sometimes need to pass some data between their
>>>> various methods. Currently that's done using the thread-local context.
>>>> For an example see dnsrecord_del, which sets a "del_all" flag in
>>>> pre_callback and then checks it in execute.
>>>>
>>>> While that works for now, it's far from best practice. For example, if
>>>> some Command can call another Command, we need to carefully check that
>>>> the global data isn't overwritten.
>>>>
>>>>
>>>> The other way data is passed around is arguments. The callback methods
>>>> take a bunch of arguments that are the same for a particular Command
>>>> invocation (ldap, entry_attrs, *keys, **options). By now, there's no
>>>> hope of adding a new one, since all the callbacks would need to be
>>>> rewritten. (Well, one could add an artificial option, but that's
>>>> clearly
>>>> not a good solution.)
>>>> In OOP, this problem is usually solved by putting the data in an object
>>>> and passing that around. Or better, putting it in the object the
>>>> methods
>>>> are called on.
>>>>
>>>> This got me thinking -- why do we not make a new Command instance for
>>>> every command invocation? Currently Command objects are only created
>>>> once and added to the api, so they can't be used to store per-command
>>>> data.
>>>> It seems that having `api.Command.user_add` create a new instance would
>>>> work better for us. (Of course it's not the best syntax for creating a
>>>> new object, but having to change all the calling code would be too
>>>> disruptive).
>>>> What do you think?
>>>>
>>>
>>> You could extend that to other plugin types as well (e.g. having
>>> Object instances with access to a single object's params instead of
>>> passing data around in a dict would be superb), but I'm afraid this
>>> kind of change won't be easy to do now.
>
>
> Ah, yes, you've discovered my ultimate goal: rewriting the whole
> framefork :)

It would seem we share the same ultimate goal, sir! :-)

> But, rewriting the whole framework at once is not realistic, so I'd like
> to do this in little steps. Starting with Commands. That should be
> possible.
>
>>> The framework was designed around singleton plugin objects right from
>>> the beginning. I personally think this design sucks, as every kind of
>>> entity in IPA is described by such an object (object classes are
>>> singleton objects, methods are singleton objects, etc.), instead of
>>> using appropriate Python primitives for the job (object classes should
>>> be Python classes, methods should be methods of these classes, etc.).
>> One note here. Having method classes separated from the object classes
>> allows to add new methods to existing objects without affecting those
>> objects and without redefining object classes.
>>
>> Ability to write simple plugins to extend existing objects is very
>> attractive.
>>
>> In case of methods being methods of Python object classes you'd lose this
>> feature and in order to gain it back you would need to deal with
>> some sort of method descriptors -- in many cases methods are in fact
>> collection of actual procedures working together, for example, most
>> of CRUD methods have pre/post modifier callbacks, spanned over elaborate
>> Method inheritance tree and allow to amend actual execute code.
>> Pulling this into a single Python method would require more work that
>> would later grow into an imitation of a separate method class anyway.
>>

Well, that is your idea, there are more ways around this. It would 
certainly feel more natural to work with such objects in Python than 
what we do now (feels rather C-ish to me).

>
> The difference between functions, methods, and callable objects in
> Python is surprisingly small. I'd bet allowing the implementer to use
> the best tool out out of those for her job would even involve less magic
> than the current approach. (We actually parse the class names with a
> regex!)

+1

>
>
> But as I said, this is generalizing my idea too much. I'd like to keep
> my feet on the ground so I'm currently proposing the change for Commands
> only.
>


-- 
Jan Cholasta