[Freeipa-devel] Return values, CRUD, webUI

[Pavel Zuna]

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...
> 
> 
> 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']}
> 
> 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']}
This is the only part I don't really agree with. See this thread for my reasons:

[Freeipa-devel] [PATCH] Add --all to LDAPCreate and make LDAP commands always 
display default attributes.

But since both you and Rob seem to agree, I won't stand in the way and just make 
the change. :)

> 
> 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.
> 
> 
> Create
> ======
> 
> If successful, we should return the resulting entry in standard form.
> If any error occurs, we should raise an appropriate exception.
> 
> 
> 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.
> 
> 
> Update
> ======
> 
> (Same as Create.)
> 
> 
> Delete
> ======
> 
> (Same as Retrieve.)
>
After a delete, there is no entry to return. Unless an exception was raised, I 
think we should just report 'delete successful' to the user and be done with it.

> 
> 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.
> 
> 
> Thoughts?
> 
> -Jason
> 

It's going to take a while to implement (and test!) all these changes, but I 
should have it done by next week.

Pavel