Questions about resources in Allergy UI module

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.)

Why does PatientService have removeAllergy and voidAllergy methods? When they both do the same, i.e. remove delegates to void

@Wyclif, those methods were transferred from allergyapi module in TRUNK-4747.

@darius, DELETE patient/uuid/allergy as a way of deleting all allergies is proving difficult to implement. DelegatingSubResource does not have a method which can be overridden to implement this behavior as its getAll method can be used to implement GET patient/uuid/allergy. Even if we assume in DelegatingSubResource.delete(allergy, reason, context) that the allergy parameter is null, I can’t see a way to access the parent Patient without which the list of allergies can’t be retrieved. An alternative is to require the client to delete all allergies one by one. What is the way to go?

Can’t you override public void delete(String parentUniqueId, String uuid, String reason, RequestContext context) and if uuid is null delete all allergies? I assume that parentUniqueId = uuid of patient (maybe I’m wrong).

Great, that is the solution I am I am looking for.

That’s the same thing i mentioned on IRC about delete. As for the duplicate methods, we should get rid of remove since it is doesn’t follow our naming convention for void methods

I’m assuming:

  • DELETE /patient/{uuid}/allergy would set the allergy list to an unknown status (i.e., we don’t know anything about the patient’s allergies).

  • DELETE /patient/{uuid}/allergy/{uuid} on the last known allergy for a patient (i.e., a patient with one allergy) would have the same effect. @jteich has argued that deleting the last allergy from the list should take the patient’s allergies to an unknown status and I agree.

But how does a client set the patient’s allergies to “No known allergies” – i.e., it’s not that we don’t know about the patient’s allergies; rather, we know the patient’s allergies and it’s an empty list. I’ve reviewed Darius’ summary, but didn’t see this in the list. Setting the patient’s allergy list to “no known allergies” (i.e., an empty list) is a very commonly used operation.

Would it be PUT /patient/{uuid}/allergies with an empty list? If so, that should return a 400 error if the patient has any known allergies (we shouldn’t silently void existing allergies).

@burke if you set the allergy status to unknown, the wouldn’t imply we need to void all the patient’s allergies? Which i think @jdegraft is having trouble with

Would it be PUT /patient/{uuid}/allergies with an empty list? If so, that should return a 400 error if the patient has any known allergies (we shouldn’t silently void existing allergies).

A problem is this will not allow a patient to move from having allergies to no known allergies if we need to support this. A child might be allergic and grow out of it. The allergies may have been attached by mistake and have to be removed.

It seems we need to differentiate the initial state and the state that is empty, from an explicit void state. If we start with Unknown Status and return to Unknown Status when all Allergies are voided, then we must be able to set No Known Allergies at any time irrespective of whether there are attached allergies or not.

As @Wyclif indicates the webservices infrastructure does not support

DELETE /patient/{uuid}/allergy would set the allergy list to an unknown status (i.e., we don’t know anything about the patient’s allergies)

it may support

PUT /patient/{uuid}/allergies with an empty list

however.

@wyclif, @burke is raising a different issue (though a bit related).

In the UI there is a “No Known Allergy” button, which is only available if the patient’s allergy state is unknown and will put them in the no known allergies state. However we have never stated how you would put a patient in this state via REST.

I agree with @burke that PUT /patient/{uuid}/allergies with body [] makes the most sense from an API standpoint.

We haven’t used PUT anywhere else, because elsewhere we only support updating certain properties, rather than putting the complete state of the resource. However in this case we are talking about putting the complete state of the resource, so…

  1. PUT /patient/{uuid}/allergies with body [], for a patient in the “unknown allergies” state should move them to the “no known allergies” state by calling Allergies.confirmNoKnownAllergies(). It should return the patient’s allergies as its response.
  • PUT /patient/{uuid}/allergies with body [], for a patient already in the “no known allergies” state should have no effect. It should return the patient’s allergies as its response.
  • PUT /patient/{uuid}/allergies with body [], for a patient with 1+ allergies recorded should fail with a 4xx response.
  • PUT /patient/{uuid}/allergies with any value for its body besides the empty list should fail with a 4xx response
    • Our API doesn’t allow creating multiple allergies at a time. This might actually be nice behavior to have but it would be part of another ticket, and we’d only do this if a real consumer requests it.

Aside: for another kind of reasoning why you wouldn’t use PUT, but that doesn’t apply in our case, you can read about REST without PUT in the ThoughtWorks Tech Radar.

The (intentional) idea is that you’re not allowed to directly go from the state of “some known allergies” to “no known allergies”, but rather you (either via the UI, or via the REST API) have to delete/void all of the patient’s existing allergies, which automatically puts them in the “unknown allergies” state. Then you can do a PUT to move them to the “no known allergies” state.