[Freeipa-devel] Return values, CRUD, webUI

Rob Crittenden rcritten at redhat.com
Tue Nov 17 16:08:03 UTC 2009


Jason Gerard DeRose wrote:
> The vast majority of our Command plugins subclass from one of the CRUD
> base classes, so in terms of return value consistency and API style, we
> need to focus most on them (and then adapt their style to the few
> non-CRUD commands).
> 
> While hooking up the webUI there have been many, many small problems in
> the core library and plugins that have caused unexpected setbacks for
> me.  Some features that I needed got changed without me noticing, some
> of my half-baked designs needed more baking, some features were missing,
> and some new code I was just unfamiliar with.  Point is, I've spent a
> lot of time battling little gotchas and thinking about how best to clean
> these things up.  Here are the guidelines I propose we follow:
> 
> 
> A return value dict
> ===================
> 
> As much as possible, I want to keep our return values very simple and
> regular.  This 1) makes our API easy to learn and use, and 2) makes it
> easy to use the return values to drive UI logic on both the CLI and
> webUI.
> 
> One current source of irregularity is the need to pass the "this isn't
> all the entries" flag from LDAP when we do searches.  For example,
> `user_find` returns an (entry_list, more_remains) tuple.  The problem is
> that most of the code paths don't care about the `more_remains` flag...
> they just need to know whether a list of entries was returned (result is
> a list) or whether a single entry was returned (result is a dict).
> 
> At the same time, we obviously need a way to pass extra data like the
> `more_remains` flag and it would be nice to be able to extend a return
> value with additional special data without breaking code or causing an
> explosion of special cases.  So I propose that our return values always
> be a dict containing at least a 'result', leaving us the option to
> extend the return value without breaking code that just looks at
> ret['result'].
> 
> So in the case of a search, instead of:
> 
>     ([{'uid': 'foo'}, {'uid': 'bar'}, ...], True)
> 
> We should return:
> 
>     {
>         'result': [{'uid': 'foo'}, {'uid': 'bar'}, ...],
>         'more_remains': True,
>         ...
>         'extend': 'as-needed',
>     }
> 
> The following all assume we are returning {'result': blah} even though
> they don't show it...

Well, I think that we'll always need some sort of special value to 
indicate when a limit has been exceeded. In v1 we decided to return what 
we got instead of failing the entire search. This proposal works for me.

> 
> 
> Entries
> =======
> 
> 95% of our return values are LDAP entries.  Currently we're returning
> pretty much the raw value from python-ldap (although we are decoding
> UTF-8 into `unicode` objects for use in the Python pipeline and encoding
> back to UTF-8 on the way out, which is good).  But the data structure
> returned from python-ldap is pretty awkward to work with.
> 
> First, at the top it's typically a (dn, entry) tuple.  Assuming the 'dn'
> key doesn't conflict with any sane LDAP attribute names, I think we
> should return a single dict with the dn stored under the 'dn' key.
> 
> So instead of:
> 
>     ('uid=jdoe,cn=users,cn=accounts,dc=example,dc=com', {'sn': ['Doe']})
> 
> We should return:
> 
>     {'dn': 'uid=jdoe,cn=users,cn=accounts,dc=example,dc=com', 'sn': ['Doe']}

I don't know, it is kinda handy to have the dn separate. But if it makes 
things more uniform I can deal with it. The advantage of having it 
separate is it draws a clear line between what you can change and what 
you can't. We'd have to strip out dn from updates and that might be 
confusing for some users. I suppose what we could do is have the LDAP 
module return a specific error if dn is included in an update.

> 
> Second, currently we return all attribute values inside a list whether
> or not they're multi-value.  This leads to lots of special cases
> throughout the code that would be better dealt with in a single place,
> in LDAP Backend adapter, IHMO.
> 
> So instead of:
> 
>     {'uid': ['jdoe'], 'group': ['foo', 'bar']}
> 
> We should return:
> 
>     {'uid': 'jdoe', 'group': ['foo', 'bar']}

Yes, I agree. What do you propose for a multi-valued attribute with a 
single value, returned as a list or a scalar? In v1 we returned any 
single value as a scalar and any multi-valued attr as a list.

> 
> 
> Lists of Entries
> ================
> 
> When a command returns multiple entries, the entries should be in the
> same form as they are from commands that return only one entry.  For
> example, currently user-find returns each entry as a (uid, entry) tuple.
> I think this should again be replaced with a single dict without the uid
> being duplicated.

Same but just within a list, right? So multiple entries are a list of 
dicts that represent the result.


> 
> Create
> ======
> 
> If successful, we should return the resulting entry in standard form.
> If any error occurs, we should raise an appropriate exception.

I think that in early versions of the framework we would return the 
results of Retrieve if the creation was successful. I think we should do 
that again in order to provide uniformity.

> 
> Retrieve
> ========
> 
> If successful, we should return the entry in standard form.  If no such
> entry exists we should raise a NotFound exception.  If any other error
> occurs, we should raise an appropriate exception.

+1

> 
> 
> Update
> ======
> 
> (Same as Create.)

+1, with a Retrieve.

> 
> Delete
> ======
> 
> (Same as Retrieve.)

Well, delete is a bit different. It either succeeds or fails. I'm not 
sure what we should return here, perhaps a boolean True.

> 
> Search
> ======
> 
> If one or more entries matches the search criteria, we should return a
> list of entries, where the each entry is in standard form.  If no
> entries match, we should return an empty list.  If an error occurs, we
> should raise an appropriate exception.

Sure. I'm ok with either return an empty list or an exception. In any 
case the caller is going to have to handle this case.

I don't think this represents a lot of work but will go a long way to 
helping our API.

rob
-------------- 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/20091117/6a3c6696/attachment.bin>


More information about the Freeipa-devel mailing list