New XML-RPC Methods (editcircle, getcircle, gettrustgroups)

[murklins]

Back in April, and again in June, catness did a fantastic job of writing new methods for the XML-RPC Protocol that replace the LJ-inherited friends methods. They provide ways of editing user relationships, both on the watch and trust edges, and ways to create/edit/delete access filters and subscription filters.

Not many people commented on the bug, though, so catness didn't get a ton of feedback on the proposed modifications, and none at all as far as I can tell from other people who actually use the API to develop client applications. I recently began moving over the old LJ Server Manual's XML-RPC chapter into the DW wiki, so I read through the patch in an effort to put together some pages for the new getcircle, editcircle and gettrustgroups methods. When I looked through the code, though, I found myself thinking of a few things that should maybe be changed before the new methods get pushed live. Once an API starts getting used by client applications, it becomes much harder to modify, and I thought it was worth having another look before the new methods got put into use.

I first detailed my thoughts in a comment on the bug, but denise suggested I open a discussion here, so I'm going to repost it all and hopefully provoke a few more people into looking over the patch and talking about whether anything would benefit from some tweaks.

It is worth noting here, as I did in my bug comment, that I am almost totally new to DW development, so maybe there's stuff going on that I am not fully grasping yet -- if that proves to be the case, please do not hesitate to set me straight!

1. The editcircle add key seems really awkward to me, as I attempt to document it. For one thing, it is called "add", but it is also the only way to remove watch and trust edges, so the add terminology is counter-intuitive. Probably "edit" is more appropriate. (Actually, I think that the trust group/content filter editing stuff should be moved out of editcircle and into a new editfilters method, then editcircle doesn't need an "add" or "edit" key at all since it will only be used for editing users' watch/trust edges, but that may be too radical a change. Ahahaha, too radical a change, she says! Like I do not proceed to go on and on about radical changes for the next hundred paragraphs.)

   Also, though, if I'm reading this right, the way to add/remove the watch and trust edges is coded such that you can't just modify one or the other. If you want to, say, start trusting a user, you can't just add the trust edge -- you first have to find out if you already watch that user, and then supply the edge key with both with the new trust value and the current watch value. But then again, if you want to change the user's trust mask, then you can do that... but you can't also change their watch edge at the same time. Maybe instead of having a single "edge" key for watch/trust, we could have an "edittrust" key and and "editwatch" key?

2. I wonder a little about the lack of symmetry in terms of how trust groups and content filters are dealt with. I know that on the backend they are very different, but I feel those differences should be hidden as much as possible by the API. Maybe that's just me? Anyway, here's where things are different, from what I can tell:

   • getfriendgroups is deprecated, and gettrustgroups was added, so why not also have getcontentfilters? catness asked about this upthread, and mark said implementation of methods could be piecemeal, but since getcircle already implements the functionality for returning content filters, it would be minimal effort.
   • The includecontentfilters key of getcircle is like the includetrustgroups key, except that is also includes the list of users in the filter. Any reason we can't match that when we get trust groups? I know that it's more calls on the backend, but that doesn't seem like a deal-breaker?

   • The way a user's trust groups are set with the add key but a user's content filters are set with separate keys is probably not going to seem intuitive for devs using the API, even though it makes sense from an implementation perspective. catness brought this up in her initial description of the addtocontentfilters key: 'Maybe it's better not to make a special keyword for it but to have an additional keyword in "add"?' So yeah, that's one option, to move it into add in some fashion. Or maybe it's not even worth trying to bring it more into line with how we edit trust groups, since those use masks and content filters don't... Or maybe we should hide the whole mask aspect of trust groups from the API level anyway, and change the add key accordingly. So then you might have (going with my replacement of the edge key with editwatch and edittrust from (1) above):

     • username - the user to edit
     • editwatch - 0 to remove watch, 1 to add watch, or not included at all to leave as is
     • edittrust - 0 to remove trust, 1 to add trust, or not included at all to leave as is
     • addtocontentfilters - list of filters to add user to
     • deletefromcontentfilters - list of filters to remove user from
     • addtotrustgroups - list of trust groups to add user to
     • deletefromtrustgroups - list of trust groups to remove user from

     Hmm, the groupmask key could also be kept around to facilitate LJ clients supporting DW. I don't know, I'm just spewing out all my thoughts now! Too many thoughts.

   • When you create a content filter, it is included in the API response, but the same is not true when you add a trust group. Presumably, this is because when you specify a non-existent id to edit_trust_groups, it creates a new one with that id so no return struct is needed to inform the client of the new group's id. In contrast, create_content_filter generates a new id for you, so you need to return that id to the client. Maybe it's not worth messing with that for symmetry's sake? But I want to float the idea of changing it so that if settrustgroups receives a non-existent id, it generates the new group's id for you and returns it in a struct, just like setcontentfilters.
3. This patch exposes terms like "edge" and "trust groups" but wouldn't it be better for the API to use terms that are closer to the front-end experience? I wouldn't expect every dev using the API to read the backend code, so they'll probably expect to see terminology more like "editaccess", "editsubscription", "setaccessfilters" and "setsubscriptionfilters". Or maybe I'm alone on this?

Comments

[lsorense]

Why not removefromcontentfilters instead of deletefromcontentfilters and removefromtrustgroups instead of deletefromtrustgroups? After all you used remove in the description you gave, and delete tends to sound a bit drastic and doesn't fit as well as a mirror for add. create and delete, or add and remove make sense as pairs (at least to me).

[azurelunatic]

All this sounds very interesting! I can't wait to see what comes out from this. :D

[pauamma]

As a general note: I believe that the whole spec for that client interface started as an attempt to mimic the field names used on the LJ web form, which was IMO a bad idea in the long term, even without the WTF split on top of it. So I'm all for changing the API spec to something more rational, for DW clients. (Also, if you want to consider DW-LJ compatibility problems and discuss them, you may want to repost this to lj_dev on LJ. You may get a better response there.)

[cesy]

Should it be "access filters" and "subscription filters" rather than "watch", "trust", or "content" filters? Those are the names given in the front end now. Or are those terms purely front-end and easily changed, and we want to use the back-end terms for the sake of clone sites?

For new developers, it would probably help to be consistent, so change the code wherever possible to use the same terms as the site. This may need to be a more general DW bug.

[catness]

Thank you so much for the detailed and thoughtful feedback!! this is exactly what I had hoped for when starting to work on it, but yes, it's still not too late :)

There's a reason to have gettrustgroups as a separate method because it's the only one required for posting, so it's needed even by a minimal client which does only posting/editing. All the rest, including getcontentfilters, is required only for circle management, which is used much less frequently than posting, and not all the clients are required to implement it - the web interface is more or less adequate for these purposes. (As for me, I had never used friends' management even in LogJam, and only added it to my client because I run out of ideas what else to add :)

Indeed, the spec is stealing part of the terminology from LJ and part from DW backend. I'm used to think of the functions in terms of "trust" and "watch", but if the consensus is to replace them with "access" and "subscription", it's fine. Never realized there's a difference between remove and delete, but now I see it (hopefully).

I had thought the list of users in includecontentfilters to be a necessary evil, because they are not limited the same way as trusted users, so it's not possible to get them all in one mask. Don't know if it makes sense to include the list of users in gettrustgroups - the corresponding LJ function does not, it works with masks. Or we should not try to keep it compatible with LJ implementation at all, as long as the functions are called differently?

*needs to remember more of the code, and to think of the other comments*

[afuna]

Hm, given all this discussion, any thoughts on my disabling this protocol method for one more code push? that gives us time to shake things out.