Cohort REST API changes with openmrs-core 2.1?

As part of openmrs-core 2.1.0, TRUNK-4906 enhances the core Cohort domain object so that instead of just being a Set of patientIds, it’s now a set of {patient, startDate, endDate}.

I want to update the REST module to reflect these changes, so help me figure out if I’m doing it right…

Also, do we know if anyone is using this API? (e.g. for android client, muzima, etc?)

Executive Summary

We have to make some breaking changes to the cohort resource, so let’s go further and redesign it more correctly:

  • Get rid of the cohort.memberIds property
  • when you create a cohort, you can set all its members by posting a “memberships” list of {patientUuid, startDate, endDate}s
  • fetching a cohort won’t have this property
  • replace the cohort/(uuid)/member/(patientUuid) subresource with a cohort/(uuid)/membership/(membershipUuid) subresource
  • its uuid will be the uuid of the membership, not the patient
  • it should have startDate, endDate, and patientUuid properties
  • it should give you a proper hyperlink to follow to get the patient’s details
  • I need help deciding on what should happen when you DELETE a membership. (Should it void the membership, or just end it?)


Currently I can create a cohort like this:

{"name":"Darius testing", "description": "REST", "memberIds": [515]}

Technically we could preserve this behavior. But if we’re going to break other things we should make it support UUIDs instead of integers, since REST isn’t supposed to know about patientIds.) So it would be better to support “memberships” that’s a list of patientUuid, startDate, and endDate.

The default representation of a cohort (and also what’s returned when you create one) contains a memberIds property, and I think we should remove this going forwards. It never fit well with the REST API anyway, since we’re not supposed to be exposing patientIds. Alternately we could preserve this property and have it return the current list of members. Question: remove the memberIds property or preserve it?

Currently I can add a member to a cohort like this:


We should extend this to support startDate and endDate fields also, but otherwise this behavior shouldn’t change. (The underlying openmrs-core API should do whatever validation about avoiding overlapping memberships.)

However the REST representation of cohort/member is currently exactly the REST rep of a patient (whether you get one, get all, or as the response to adding a member).

First, we need to change this to be an object with startDate, endDate, and patient properties; that will be a non-backwards-compatible change for the resource.

Second, it used to be that the sub-resource uuid was the patient’s uuid, e.g. the URI is like .../cohort/uuid-of-cohort/member/uuid-of-patient, but since now the same patient may be in a cohort more than once with different dates, and since CohortMembership in core now has its own UUID, we need to use that instead.

Third, a cohort is really supposed to just have references to its members, since the cohort may be large and fetching all the member details may be expensive. I propose that as long we’re breaking the REST API for this, we also correct things so that the cohort/member subresource has a patientUuid property (easy to use) and a link to the patient (proper hyperlink):

    "startDate": "2017-01-01",
    "endDate": null,
    "patientUuid": "uuid-of-the-patient",
    "links": [
          "rel": "patient",
          "uri": ".../patient/uuid-of-the-patient"
          "rel": "self",
          "uri": ".../cohort/uuid-of-the-cohort/member/uuid-of-the-membership"

Also, I propose we rename this subresource from cohort/member to cohort/membership, which conveys the meaning more correctly, and aligns with the new openmrs-core domain object CohortMembership.

Basically we have to make several significant backwards-incompatible changes to the cohort resource, and as long as we’re doing that I propose we just rewrite it to behave better.

The one thing I’m not sure about is what should happen when you do DELETE .../cohort/(uuid)/membership

In the underlying Java API, Cohort.removeMembership() will set the membership’s endDate to be now(). This is my default thought for what the REST API should do when you send a DELETE. But if we do that there’s no way to void a cohort membership. Thoughts?

Default is to do:

  • DELETE membership => set the membership’s endDate to now(), like Cohort.removeMembership()
  • DELETE membership?purge=true => hard-delete the membership
  • no way to void

The alternative would be for DELETE to void, and if you want to end the membership now you do a POST ... {endDate: "..."}.

Wow, there is a lot here :slight_smile: As far as your last question, my opinion is that delete should void/purge. Setting an end date on a membership should be a post. The changes you suggest in terms of UUIDs and references to patient resources make sense to me.

(I’m not 100% sure about this whole cohort membership thing in general, but that ship has clearly sailed).


Delete should behave consistently with our rest API i.e. delete should mark the membership as voided. Purge is a little trickier, I’d want purge to have the membership removed from its parent cohort and not ending it, ending a membership can be performed by posting the membership with endDate property specified, It’s just that this exposes the internal logic of the cohort api. Can’t we have a sub resource for the end membership operation? Something like below,


That seems a little weird, I think am starting to agree with Darius that may be purge should be the one to use to end a membership rather than removing it entirely from the parent.

I agree that DELETE should void to be consistent with our API. DELETE with purge=true can purge. That leaves POST to set an end date. I suppose it’s not too much trouble to ask a client to populate end date with the current time, but what about a convenience mechanism like POST ... {endDate: "now"}?

In a RESTful API, resources should be nouns (i.e., things), not operations. It’s possible to use reification to turn operations into nouns, like “EndMembershipRequest”, but those make more sense for long-running operations or operations that have important associated metadata (about the operation itself).

Searching on github, I see that at least @jdick, @nyoman, @sthaiya, and mUzima (fyi @ayeung) are using the current cohort REST API.

This REST API is going to need to change in a non-backwards-compatible way, with openmrs-core 2.1. FYI to those mentioned, and please comment in this discussion if you have thoughts.

Okay, I am convinced that we should make DELETE consistent with the REST API (vs consistent with the underlying Java API), and use POST to set an end date.

A lot has already been said regarding the proposed REST API behavior. I am a little curious regarding the underlying core API though and may be this is not the place please bear with me, why is the method that ends the membership called removeMembership(). It sounds misleading to me. Wouldn’t a name like endMembership() be more consistent with what is actually being done?

1 Like

I agree with Willa, it would be best if we added the more intuitive endMembership() to have the current logic in removeMembership() in the PR, get rid of purgeMembership() and change removeMembership() to have the current logic in purgeMembership()

For consistency in the API, I would expect the verbs “end”, “void”, and “purge” for memberships. Isn’t that more consistent with what we’ve done elsewhere?

Okay it also makes sense to make these changes for consistency in the underlying API.

@wyclif and @vshankar, are you going to address this as part of the still-open PR?

Void and purge are used in services and not on domain objects, @burke are you saying we should have voidMembership() and purgeMembership() on the Cohort class?

I would suggest doing this consistently with how it works elsewhere in the API.

So it would be

  • CohortService.voidCohortMembership(CohortMembership)
  • CohortService.purgeCohortMembership(CohortMembership)
  • Cohort.endMembership(CohortMembership), though I don’t really see what this adds beyond CohortMembership.setEndDate(…) or just CohortMembership.endNow()

That means Cohort.removeMembership(CohortMembership) is effectively the same as CohortService.purgeMembership(CohortMembership), or should we not have Cohort.removeMembership(CohortMembership)?

We would remove remove. I mean, we would purge the remove method. :wink:

I finally managed to commit some code for this. The ticket is if you’d like to follow this.

(It’s functionally complete, but might need some performance improvements before it’s fit for release.)