[Freeipa-devel] more funky interface stuff

[John Dennis]

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.

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.

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.

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.

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.

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.

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.

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.

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)

Often None is used to indicate "invalid; no result" as opposed to "valid 
but empty result". That situation could be handled by returning a struct 
with a flag whose value carries the interpretation of None, elsewhere in 
the struct is the the return value. But that is awkward and it doesn't 
handle the case where None is embedded in a complex object.

Bottom line, I don't know how to deal with the None issue. Getting rid 
of it could be really hard, leaving it in could be really limiting if 
the <nil/> extension is not well supported in other XMLRPC libraries.


-- 
John Dennis <jdennis at redhat.com>