Questions about resources in Allergy UI module

Moving AllergyAPI into OpenMRS 2.0 Platform - TRUNK-4747 has raised the following issues:

  • should the “/allergies” resource be a subresource of patient since allergies are always within the context of a patient (In the current allergyapi code this is not the case)
  • should “/allergy” resource also be a subresource of patient (In current webservices.rest this is not the case and /allergy/id is not implemented. In any case to implement this feature, a new method will have to be added to PatientService, getAllergyByUuid()) Any suggestions on how to proceed?
1 Like

@jdegraft, thanks for noticing this!

We discussed this a bit on the Developers Forum today but did not come up with a final answer.

Broadly, there are a few options:

  1. Change things as little as possible. Keep the /allergies resource, and also /allergies/{uuid}/allergy
  2. Put “allergyStatus” and “allergies” properties on the /patient resource
  3. Put “allergyStatus” on /patient, but have the actual allergies accessible through /allergies or /allergy

Note that the PatientService in the allergyapi module ( https://github.com/openmrs/openmrs-module-allergyapi/blob/master/api/src/main/java/org/openmrs/module/allergyapi/api/PatientService.java ) also needs to be moved to the core PatientService.

We will pick this discussion up again when @burke is back from vacation, since he had strong opinions about the design of this API.

@darius, as part of TRUNK-4747 - Move Allergy API into OpenMRS 2.0 Platform - , the allergyapi features of PatientService has already been merged into core. The only issue remaining for that ticket is how to handle the REST API features of allergyapi. allergyapi did not have the getAllergyByUuid() method though. Let’s see what views @burke has for moving this forward.

From a clinical standpoint, the resources that would be intuitive to me would be the sub resource approach:

/patient/{uuid}/allergies → [] (for NKA), [ {allergy1}, … ], or HTTP 404 if unknown /patient/{uuid}/allergies/{uuid} → { allergy }

I suppose I could be convinced of allergies as a separate resource, since FHIR has a separate AllergyIntolerance resource; however, it seems unintuitive to me to reference allergies outside the context of a patient. In my mind, the existence of allergies (or no known allergies) is a “property” of a patient.

-Burke :burke:

(btw, I’m not a fan of the term allergyStatus)

@burke, someone else suggested they should be a subresource on the design call. I pushed back by saying that visit, encounter, and lots of other things also belong to patient, but nothing else is a subresource. Why should allergy in particular be a subresource when nothing else is?

It’s a fair point. While I wouldn’t say that everything linked to a patient (encounters, orders, etc.) needs to be a subresource, it would make sense to me if those things that are attributes of the patient (allergies, identifiers, and conditions/problems) were subresources.

You can ask about an encounter and discover who the patient was for it. It would be odd to ask about an allergy and then find out who had it. :smile: (given this logic, I suppose identifier could make sense as an independent resource… but allergies & conditions/problems are properties of the patient).

Okay, I’m convinced.

I’m not sure how easy it is to implement this as a sub resource given our existing base classes in the framework and the special behavior associated with the NKA state, but we can try.

I don’t really like representing NKA as a 404. Especially because it’s not intuitive how you would set that status. I would lean towards having an object with one property for the status +/- a property for the list.

Allergies as a sub-resource of patient seems a good design choice.

No Known Allergies, NKA, may best be represented as a empty list rather than a 404, though a bit easier to implement it still may be a bit difficult as @darius points out. However NKA as an empty list would require that the API around allergyStatus be removed.

Is this the consensus and should implementation proceed?

I should clarify my shorthanded summary:

What I meant was:

Patient with No Known Allergies

This is not the same as unknown allergy information. Someone has explicitly confirmed that the patient does not have any known allergies – i.e., allergy information is known, there just aren’t any allergies. In this case, we have a known list of allergies that is an empty list.

curl -i /patient/d8f598df-6f24-4434-845b-0849f2360dc3/allergies

HTTP/1.1 202 OK

[]

Patient with allergies

Surprise, surprise. We return a list of allergy resources.

curl -i /patient/d8f598df-6f24-4434-845b-0849f2360dc3/allergies

HTTP/1.1 200 OK

[
  {
    # allergy resource #1 here
  },
  {
    # allergy resource #2 here
  }
]

Allergies unknown

This is not “no known allergies.” We do not have any information about the patient’s allergies. They might be allergic to something; they may not have any allergies. Returning an empty list would be misleading. Our clearest response is to say the allergies resource does not yet exist for this patient.

curl -i /patient/d8f598df-6f24-4434-845b-0849f2360dc3/allergies

HTTP/1.1 404 Not Found

@burke, how would you set a patient to the NKA status, in this world where getting it returns a 404?

Just post (technically, it should probably be a PUT) the empty list of allergies.

curl -X POST -d "[]" /patient/d8f598df-6f24-4434-845b-0849f2360dc3/allergies

HTTP/1.1 202 OK

[] # returns the patient's allergies

Note that NKA does not return a 404. Only missing allergy information (which is not the same thing as NKA) returns a 404.

Sorry for the confusion, I have been completely incorrect in my usage of NKA in my previous two posts.

What I mean to ask was: how do you set a patient’s allergy status to unknown?

The pattern we have followed for personattribute and patientidentifier are as follows:

  • GET person/uuid/attribute => { results: […] } // results are paged if there are many
  • POST person/uuid/attribute => creates one new personattribute
  • DELETE person/uuid/attribute => invalid operation
  • GET person/uuid/attribute/attruuid => { representation of one personattribute }
  • POST person/uuid/attribute/attruuid => updates a specific personattribute
  • DELETE person/uuid/attribute/attruuid => voids a specific personattribute

To follow this same pattern (which should be our default approach) allergy would look like:

  • GET patient/uuid/allergy => { results: […] } // results are paged if there are many
  • see below for discussion
  • POST patient/uuid/allergy => creates one new allergy
  • DELETE patient/uuid/allergy => sets allergyStatus to unknown and voids all allergies
  • this is new (e.g. there’s no equivalent mechanism to remove all personattributes)
  • GET patient/uuid/allergy/allergyuuid => { representation of one allergy }
  • POST patient/uuid/allergy/allergyuuid => updates a specific allergy
  • DELETE patient/uuid/allergy/allergyuuid => voids a specific allergy

About GET patient/uuid/allergy@burke says that should return 404 for the unknown state. I think this will be unintuitive for a client, and I propose that we do:

(if no known allergies)

{
    status: "No known allergies"
    results: []
}

(if some)

{
    status: "See list"
    results: [
        { /* ... */ }
    ],
    links: [ ... ]  // if >1 page of results, which should never happen in real life unless the client requests a small page size
}

(if unknown)

{
    status: "Unknown"
}

(these text strings are from the Java API (see here)

Are you suggesting that DELETE be used to set allergies to unknown? after a DELETE operation I would expect a 404 at that resource. I think using 404 is an interesting idea here in the context of sub resources that have never been set. Or how about 204 no content? Caveat: I am no rest expert, just voicing my opinion

Based on the REST API Tutorials documentation on HTTP Status Codes, a 404 seems more appropriate than a 204, since a 204 No Content just means that the server is sending a response without content (not necessarily indicating why the message has no content).

To me, returning a 404 for missing data seems intuitive and RESTful. But, if handling 404’s in ajax code is extra work, then Darius’ approach could make sense – i.e., you always get a response and need to inspect the response and infer either from a status string or a missing property that the resource does not exist.

From peeking at the docs for Angular’s $http service, I think that a 404 is treated as an error code, so it will probably be extra work (e.g. it’s the errorCallback for the promise, not the successCallback). Someone can explore this more in depth if they want, but I would lean towards communicating “unknown allergies” via the response content, not the HTTP status code.

In this scenario:

curl -X POST -d '{"allergen": "penicillin"}' /patient/{uuid}/allergy

HTTP/1.1 200 OK

{
  "allergen": "penicillin",
  "links": {
    "self": "/patient/{uuid}/allergy/cf60b614-02a6-4149-aa49-a433393ea93a"
  }
}
curl -X DELETE /patient/{uuid}/allergy/cf60b614-02a6-4149-aa49-a433393ea93a

HTTP/1.1 200 OK

What would be the response to this?

curl /patient/{uuid}/allergy/cf60b614-02a6-4149-aa49-a433393ea93a

Wouldn’t it be a 404? If so, couldn’t deleting /patient/{uuid}/allergy behave the same?

Couldn’t something like this:

$http.get("/patient/{uuid}/allergy").then(function(data) {
  if (data.status === "Unknown") {
    displayAllergiesUnknown();
  } else {
    displayAllergies();
  }
});

Leverage the 404 to become something like this?

$http.get("/patient/{uuid}/allergy")
  .then(displayAllergies)
  .else(displayAllergiesUknown);

We had to use the status (No known allergies|See list|Unknown) within Java because the API needed to return a single object regardless of state of the allergy list and throwing an exception for expected behavior isn’t best practice. In REST, however, I think using 404 for a missing resource is best practice, isn’t it?

Returning a 404 for a missing resource seems intuitive to me, but ultimately, we want to produce whatever is going to be easiest for a dev to understand & consume. I’ll stop painting this shed. :no_mouth:

Returning 404 for NKA is a bit unintuitive to me. I would expect a 404 for calls that i should not be making because they are wrong, mispelt, or something that am doing but i should not. I find it ok, natural and acceptable (no need for 404) to make a get for a patient who may not yet have any known allergies. :smile:

Just s reminder that we have a design forum on Resources in Allergy UI Module scheduled for Wednesday, December 23 @6-7pm UTC

If this issue has been settled, can the design approach be summary to enable development? (@darius, @burke)

This is what to implement

Summary: (based on Questions about resources in Allergy UI module and several following comments:

  1. Allergies will be a subresource of patient. Mostly this will behave like the existing personattribute and patientidentifier subresources.

  2. The patient/allergy subresource will function like:

  • GET patient/uuid/allergy => gets current allergies (see below for what is returned)
  • POST patient/uuid/allergy => creates one new allergy
  • DELETE patient/uuid/allergy => sets allergyStatus to unknown and voids all allergies
  • this is “new” (i.e. there’s no equivalent mechanism in personattribute or patientidentifier)
  • GET patient/uuid/allergy/allergyuuid => { representation of one allergy }
  • POST patient/uuid/allergy/allergyuuid => updates a specific allergy
  • DELETE patient/uuid/allergy/allergyuuid => voids a specific allergy
  1. What is returned when you fetch a patient’s complete allergy status (i.e. GET patient/uuid/allergy)

(We will take a leap of faith and follow @burke and @kristopherschmidt’s suggestion about how a REST-first API should behave, even though @dkayiwa and I think this is less intuitive.)

If the patient’s allergy status is “Unknown” => return a response status of 404

If the patient’s allergy status is “No known allergies” => return { results: [] }

If the patient’s allergy status is “See list” => return

{
    results: [
        { /* representation of a single allergy */ },
        ...
    ],
    links: [
        /* if >1 page of results, we have next/prev links, just like other resources */
    ]
}
  1. Personally I think that the patient resource should also have an “allergyStatus” field added to it (and the underlying Patient.java and Patient.hbm.xml should reflect this), but I’m not 100% certain about this. (And further discussion on this point should not block the bulk of the web service work from being done now.)