Enable collection GET requests on the API to populate memberships

[struan]

This requests add /:collection/join/:join_specifier/:id syntax to the API to reduce the necessity for multiple requests when using information in the memberships of a collection item. It enables API requests with the persons or organizations of an items memberships populates, although not both at the same time.

[mhl]

Is there a corresponding documentation update pull request for this, by the way?

[chrismytton]

Looks good! :+1:

@mhl Corresponding docs are in the join-docs branch of popit.poplus.org. Would be good to get that pull requested as well.

[mhl]

Before this is merged could I ask why the join specifier is part of the resource rather than a query parameter? It'd seem more RESTful to me that way.

[struan]

Mostly as it means in the code the more common case of just getting the resource is a completely separate case from the one that included the joins rather than checking if you need to do a join. Also I guess in my head I though of it in an SQL I am getting from here, joining it to this and I want this item sort of operation so wrote the URL accordingly.

[tmtmtmtm]

Leaking database terminology into the API (especially in the resource, rather than a parameter) seems very odd and confusing to me. Ideally an API call should be as self-documenting as possible, and I'm not sure many people would guess correctly what /persons/join/organisations/org.mysociety.za/person/104 will return. I suspect, if anything, more people would think that it's something to do with querying the dates of memberships (e.g. when Fred Bloggs joined the Labour Party).

In terms of 'least surprise' and the expectations users will have from other APIs, etc, something more akin to Eve's embedded resource serialization seems like a better approach.

[mhl]

@struan Another reason to make it a query parameter is that it'll make it easier to use the joined information from REST API wrappers like Slumber

[struan]

I've changed this so it now uses and ?embed=person type structure or person.organisation for multiple levels as that feels a bit more javascripty.

[tmtmtmtm]

@struan that definitely seems like a better approach, although I'm a little confused by the need for (and slightly also the naming of) the ?basic parameter. Is there a reason why that isn't simply the default behaviour: nothing is expanded unless you ask for it with an ?embed?

[mhl]

@tmtmtmtm That would break existing scripts that use the API and expect the membership data to be inline for a person by default, for instance. We should have a TODO list of API-breaking changes we'd like to do for the next API version.

@struan Great, I think that's much better too - the commit message might need amending now, where it says "which takes the same form as django's join specifiers". I suppose it does say "form" rather than "syntax", but maybe "which are similar to" or "inspired by" might be clearer.

[mhl]

Actually, after some discussion in IRC about the naming of the basic parameter, perhaps the better solution is to have:

• embed= ⇒ no memberships included (rather than basic)
• No embed specified or embed=membership ⇒ current behaviour, just memberships included, so the API isn't broken
• embed=membership.person ⇒ expand people mentioned in memberships
• embed=membership.person.membership.organization ⇒ expand people mentioned in memberships, their memberships and any organizations mentioned in their membership (which is currently embed=person.organization, IIUC?)

Another thing that strikes me is that you should be able to have multiple embeddings specified which are comma-separated, but that should probably be a new issue and pull request, since this is already hugely useful as it is.

[struan]

My only caveat is that while it makes sense I find membership.person is quite wordy.

[tmtmtmtm]

I'm not convinced that the design should be driven very much by existing behaviour — especially as this is explicitly marked as alpha code and that the API will change. At this stage in development it's more important to get things right, than to drift down the road towards an ugly API by legacy support.

If we don't want to version the API yet (although being able to bump the API version for things like should really become something that's fairly easy to do), then there aren't really that many users so far, and we probably know all of them well enough to actually talk to them and warn them of the change — especially if they could switch to a replacement query in the meantime that works with the existing code (as a no-op) and would continue to work with the new version.

[mhl]

@tmtmtmtm The API is already versioned (although it's arguably not the ideal way, there's /api/v0.1 in each resource's URL). We've been asked several times by people considering using the API if we're going to break it without changing the version number and certainly in the last few months have said that we won't - I think this an important undertaking to encourage people to use the API in the first place. Even when we haven't actually changed the API, there was a complaint that we had.

We're telling people to regard PopIt as an alpha service, but in practice people are already using it in live sites; I don't think this is an important enough issue to make it worth a version change and contacting all our known users. (Also, I'm not convinced that we do know all the users for some PopIt instances, e.g. the South African one that's been used at hackathons there.)

[tmtmtmtm]

Sorry, I was unclear in what I was saying. I'm aware that we theoretically have a versioned API — I meant that as we've never actually changed the version, we don't really have a simple process yet for actually doing so.

My suspicion (which may be entirely incorrect), is that if we'd already done that a few times, I think we'd find it much easier (conceptually as much as technically) to do so for this.

To hopefully be a little clearer, my argument is that:

• we should design the ideal API for this feature
• if that is incompatible with the existing API, then we should change the API, not the design
• changing the API should ideally happen with a version bump
  • but even if for some reason we don't want to do a version bump, then we should still change the API anyway, though probably by giving people a little bit of notice, and a migration path that works in advance of the change.

[mhl]

@tmtmtmtm OK, your points are well made. Two additional things that are worth mentioning, though, are:

• At the moment we're not sure what would be the best way to implement multiple concurrent API versions with the current codebase - there are several options, and actually I feel that we're likely to make the wrong decision about that until we have a good list of the changes we'd want to make in the next version.
• This pull request is for a feature which we need urgently, so I'm not inclined to introduce a breaking API change to the current version that would mean waiting to let people know about the change and give them reasonable time to act on it, especially since (I think) the only difference between what I suggested last and what you want is just what's returned in the default case (i.e. if embed isn't specified).

So, yes - I agree that having made the first concurrent new version would be a good thing for the future of the project and make us less wary of making improvements to the API, but I don't think this is the right feature to trigger that.

[chrismytton]

Looks good :+1:

[struan]

This has been merged