[Freeipa-devel] more funky interface stuff

[Rob Crittenden]

John Dennis wrote:
> Rob Crittenden wrote:
>> Rob Crittenden wrote:
>>> I've looked into some more questions raised about the interfaces.
>>>
>>> One is why rpcclient.py and ipaclient.py?
>>>
>>> ipaclient.py was created because of the ticket forwarding issue we 
>>> had early on. Since we didn't have a ticket for the UI we wouldn't be 
>>> able to use the XML-RPC interface directly, so instead we wrote a 
>>> thin wrapper which called into the XML-RPC backend functions directly 
>>> (instead of over XML-RPC which required a ticket)
>>>
>>> This is also why ipaclient.py has to do calls to toDict() but doesn't 
>>> have to unwrap binary data. Conversions that are done in XML-RPC 
>>> interface are not done when talking directly to the backend, hence 
>>> the need to, or not, do them in ipaclient.py.
>>>
>>> Now that we do have ticket forwarding working in TurboGears it may be 
>>> possible to switch to rpcclient.py. This would have the added benefit 
>>> of being able to move the UI code onto a separate web server at some 
>>> point. The downside is that it would likely slow down the UI a bit 
>>> and it would hit the KDC a lot harder.
>>>
>>> I can investigate this further if desired but it might take a day or 
>>> two to work out all the details (and time is already short).
>>>
>>> rpcclient.py is there to remove code complexity from the admin tools. 
>>> I needed an RPC client to make calls, it seemed to make sense to 
>>> mirror the XML-RPC interface in it. It also does the None -> __NONE__ 
>>> conversion for us and handles doing the data conversions (unwrapping 
>>> binary data). The functions all look more or less the same, and there 
>>> may be a way to consolidate it down, this was the most expedient way 
>>> to do it. I didn't want to abstract out the XML-RPC interface, just 
>>> make calling it easier.
>>>
>>> If there are any specific things to look at just let me know. Or we 
>>> can do this as part of the API review.
>>>
>>> rob
>>
>> I should add that ipaclient.py is really the abstraction layer that 
>> determines how a request is made. If it is a "local" request it 
>> imports funcs.py (the XML-RPC layer) and does direct calls. If it is a 
>> "remote" request it uses XML-RPC and the functions in rpcclient.py.
> 
> First let me say my comments below do not address API design per se, but 
> are more of discussion of the current implementation of the RPC API. 
> Questions of API itself (e.g. which functions are exported what data 
> they operate on is another topic).
> 
> I think the vast majority of the code duplication in both ipaclient.py 
> and rpcclient.py can be eliminated with a single decorator, that would 
> be a huge step in simplification and consistency.

Being new to python, I still really don't know what a decorator does (I 
haven't read the link you provided earlier yet). I've used it in 
TurboGears but much of that is still voodoo to me.

> If we still want to preserve the local vs. RPC calling convention that 
> too could be folded into the decorator. Although I'm not sure it's 
> necessary for the following two reasons.
> 
> 1) Working ticket forwarding might make the point moot.

The nice thing about the local calls is it saves a round-trip per call.

> 2) I'm not sure why the distinction exists in the first place. If a 
> module is going to be making local calls it should import the local 
> interface, otherwise it should import the remote interface (but perhaps 
> I'm missing some larger issue such as needing to switch between local 
> and remote at run time). With decorators the decorator function could 
> key off of a flag set before the import and return the proper function 
> pointer (local vs. remote) thus not requiring a different import.

We expected that the issue would resolve itself at some point thus 
making the local vs remote issue moot. We designed this so that if that 
happened few to no changes would be required in either client. By 
forcing everything to use ipaclient.py we help ensure that the 
capabilities remain the same between our two clients. It would be too 
easy to let the UI make calls that the RPC layer couldn't and then our 
UI and cli get out-of-sync.

> Questions/Issues:
> 
> The wrapped functions in ipaclient.py sometimes modify the input 
> parameters and sometimes modify the results. This just makes using the 
> API we've defined harder because if you're not using our library and 
> instead are trying to use the RPC API we've defined you may need to 
> aware of the various exceptions and replicate the special handling. In 
> fairness the majority of the special handling is the coercing of XML RPC 
> structs (e.g. dicts) into Python object classes. That would an 
> appropriate operation for a decorator to perform but it runs afoul of 
> one issue, if you want consolidate code and avoid duplication you'll 
> want to be using just one decorator, but the decorator won't know if it 
> needs to coerce the result, and if so then into what class? There are 3 
> ways one can address this:
> 
> 1) Be honest about the fact you're calling an RPC function which has no 
> knowledge of Python. You limit the interface to what's available in XML 
> RPC. The advantage is simplicity, but you lose friendliness.

It is our client, we can do whatever we want with the input (or output 
for that matter). Someone else writing an RPC client would likely do 
something similar, to convert the raw RPC data into a local object type. 
As far as I can tell the RPC server is returning data in a standard 
format, it is up to the client to decode it, right?

> 2) Add a decorator which defines the function signature, each arg in the 
> decorator defines the type of the arg which is stored with the function. 
> When the decorator executes it looks up the each argument type and 
> decides if it needs to coerce it. In the past I had written Python RPC 
> code and this is how I solved this issue when I ran into exactly this 
> problem. Here is an example:
> 
> @rpc_method('SETroubleshootDatabase')
> @rpc_arg_type('SETroubleshootDatabase', SEFaultSignature, str, int, str)
> def set_filter(self, sig, username, filter_type, data):
> 
> The @rpc_method decorator does the magic of turning the function into an 
> RPC call, the 'SETroubleshootDatabase' parameter is the interface the 
> method belongs to. I'm guessing we're never going to export more than 
> one interface so we could simplify things by eliminating the use of 
> interfaces.
> 
> The @rpc_arg_type decorator specifies the signature. In this instance 
> it's a instance of a SEFaultSignature class object followed by a string, 
> an int and a string. For our use with XML RPC we only need to specify 
> the type when it's a class instance so this could be simplified, but 
> hopefully you get the idea.

Hmm. I think I need to read more about decorators.

> 3) Define the signature in a table and have the decorator look the 
> signature up in a table. This is just a variant on (2) but avoids having 
>  the extra decorator used to specify the signature. I don't recommend 
> this as my feeling is the decorator approach is much cleaner, keeps the 
> definitions in one obvious place (right where the function is defined) 
> which is one of the design goals of Python decorators.
> 
> The rest of the munging might be evidence of a problem in the API 
> design. Here are a few items to consider:
> 
> 1) Some functions which return a list return a array with a "counter" 
> stuffed into the first position of the array which defines the array 
> length. Why? the length is implicit in length of the array and this 
> makes the returned array non-homogeneous (while both Python and XMLRPC 
> support non-homogeneous arrays it's best to avoid this construct because 
> it makes it difficult to interpret without a priori special knowledge of 
> the array contents). The reason the counter is stuffed into the front of 
> the array is to carry a special flag value indicating if the returned 
> array was truncated. Wouldn't it be better if the result were not an 
> array but rather a struct which contains the truncation flag and the 
> array? That means the arrays do not require special interpretation, the 
> flag is much more explicit and if need be more information could be 
> returned about the state of the search.

Either way you have a special flag. I have no particular feelings either 
way about this. We could easily convert this into a dict (struct) and 
return that.

> 2) The functions which modify an object class perform special handling 
> of the before and after values so that the implementation on the server 
> side can compute the differences. If somebody else wants to call the RPC 
> API that's going to be confusing, some functions take one parameter (an 
> Entity class with the before and after values embedded in the class 
> instance) and other function signatures take two parameters passing the 
> before and after dictionaries explicitly. I would rather see a 
> consistent function signature with a pair of before and after 
> dictionaries to expose the logic of modification. This issue somewhat 
> falls into the above issue, attempting to hide the actual RPC API. I'm 
> not sure that's a good thing for two reasons, one, we would like to call 
> functions both locally and remotely, it's way easier if they look and 
> behave the same, two, if we really want to expose the RPC API for third 
> party development we too should be able to call it without wrapping it 
> with modifications.

Yes, all of the update functions are poorly handled. I have it on my 
todo list to convert it to take a single dict as input.

> 3) At least one of the RPC wrappers removes an attribute from a struct 
> it's passing, apparently because of private knowledge about the 
> receiving end's requirements.

Ah yes, in add_user(), add_group(), that is bad.

> 4) None Type: XMLRPC does not support the None type but it is used 
> extensively in our code and is extremely useful. To make our XML RPC API 
> interface useful and appealing to third party users we should avoid 
> non-standard XML RPC extensions such as <nil/> (which is how None is 
> mapped in XML RPC) The appeal of XML RPC is that it's a language neutral 
> portable RPC mechanism. Using the special <nil/> extension would blow 
> that out of the water. I have no clue how that would get mapped for a 
> client written in C for instance. Using None (e.g. <nil/>) is so useful 
> it would be hard to get rid of it, plus we use None in so many places it 
> might be hard not to let it "escape" through the XML RPC interface 
> unintentionally. Yet, on the other hand I don't think we want to make a 
> statement like "We have this wonderful RPC interface for you to use, 
> except you can't code in C or C++, or use any XML RPC library which 
> doesn't support the extension)

I believe I've removed all of the None arguments. opts is special since 
it is added after the RPC call is made, so you can ignore that. It is 
used to pass stuff from Apache (like the principal name) into our functions.

I definitely don't want to turn on None handling.

I left my funky handling of it in for now though I don't think it is 
actually used anymore (the __NONE__ stuff). The only place perhaps is 
the *_container but one can pass an empty value instead and it should 
work fine.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3245 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20071130/b685f41d/attachment.bin>