[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