[Freeipa-devel] Return values, CRUD, webUI

[Jason Gerard DeRose]

On Wed, 2009-11-18 at 15:15 +0100, Pavel Zuna wrote:
> 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. :)

Well, you've made some good points, and this is something I don't feel
super strongly about either way (although I lean toward not putting
scalar values in a list).

Rob, has this swayed you either way?

> > 
> > 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

We need to coordinate closely on these changes so we don't duplicate.  I
have some proof of concept changes I made just while exploring the idea,
but these were just scratch really.