Custom REST Resource for Obs with "value location"

Hello all…

I’m struggling a bit with coming up with the right approach to address an issue with trying to fetch a custom representation of an Encounter that have Obs with “value location” and was looking for suggestions.

The underlying issue may be that we were trying to get too “cute” and create a hacky way to support “value locations” for obs.

The History:

There’s a supported pattern where you can set the “comments” field of an obs to “org.openmrs.Location” and then set the the “valueText” field of that obs to the primary key of a location in the database.

The REST web services module supports this format and returns a Location object as a obs value in this case, see:

Current Issue:

In one of our OWAs, I’m trying to fetch an Encounter RESTfully using a custom representation. A simplifed version of the rep I’m using:

custom:(id,uuid,encounterDatetime,obs:(id,uuid,value:(id,uuid,display,names:(id,uuid,name,locale,localePreferred,voided,conceptNameType)),obsDateime))

So, when pulling back the the obs on an encounter, if one of the obs has a value of a “Location”, it fails when trying to convert a “Location” to this custom ref:

(id,uuid,display,names:(id,uuid,name,locale,localePreferred,voided,conceptNameType))

It’s not entirely clear to me what the best/correct solution for this is, as I feel we’ve exposed a limitation of have a “flexible” schema for obs value.

Note that in this case, I actually don’t care about the obs of type location, so I could create a custom endpoint that returns a subset of obs on the encounters, excluding the problematic ones. But I do feel like this is a problem that could turn up in other cases. (And also would be likely to fail silently and/or be missed by testing).

Some options:

  1. Change the base REST web conversion handler so that if a custom rep throws an error when parsing, don’t fail entirely, but log an error and return the default (or reference?) representation of that particular sub-object. This would likely be the easiest option, but not sure it’s really correct and would broadly touch the REST web services conversion error handling

  2. Do something similar to #1, but instead of adding it to the main conversion code, create a custom Location converter and only allow bad custom representation for Locations (and potentially only for those that are obs values… though I’m not sure if the parent element is available to the parser). This seems uglier than #1, but is also less intrusive.

  3. Try to define some sort of custom rep format that allows for conditional refs based on object type. This seems potentially overkill and ugly and non-trivial and risks instability in the custom rep code:

value:(Concept|(id:uuid:names)|Location(id:uuid:name:tag))

Thoughts? Any potentially elegant solutions I’m missing?

@mksd @ibacher @dkayiwa @cioan @bistenes

Take care, Mark

1 Like

So, one consideration is that if the client is requesting a particular custom format it’s likely to written assuming it gets data in that custom format. For example, if you went with an option that returns the default representation for that obs, then the front-end will also need to somehow handle that additional representation. That seems to me to be the weakness of both 1 and 2 (since in either case).

I think it would be slightly more elegant to do this:

1*. Change the base REST web conversion handler so that if a custom rep throws an error when parsing, the error is logged and we add it to an “errors” array in the main JSON document, something like:

{
    "results": [
        ...
    ],
    "errors": [{
        "data_type": "Obs",
        "uuid": "...",
        "message": "Could not convert Obs to desired representation."
     }]
}

The idea being that the results array ends up containing just objects in the requested format, but the client has enough information that it can track down, e.g., the value location if it really wants to.

Admittedly, I’m not overly familiar with the internals of the REST API, so this might be quite difficult to actually implement, plus it does have the down-sides of 1, but I do think that it strikes a balance between not hiding data and keeping things easy for the client.

2 Likes

@ibacher… thanks, my first reaction is that I really like that idea!

Thanks @mogoodrich for detailing this.

I kinda feel that the right thing to do is between your option 1 and @ibacher’s suggestion.

Perhaps the server should return the array of results, including those that couldn’t be converted to the desired representation, but without separating them as results vs errors. Rather the errored ones could be distinguishable but returned as part of the overall valid response. Something like:

{
  "results": [
    {
      "data_type": "Obs",
      "uuid": "dbdacfeb-cff8-4c93-843b-b7068478b073",
      "representation": "(id,uuid,name,locale,localePreferred,voided,conceptNameType))",
      "message": "",
      "data": {
        // here the obs that could be represented as (id,uuid,name,locale,localePreferred,voided,conceptNameType))
      }
    },
    {
      "data_type": "Obs",
      "uuid": "74fe9fad-fa9e-4a89-9b15-c28dcfeafbed",
      "representation": "default",
      "message": "Could not convert Obs to (id,uuid,name,locale,localePreferred,voided,conceptNameType)).",
      "data": {
        // here the default obs representation
      }
    }
  ]
}

In both cases there this will require some gymnastics on the client side.

1 Like

@mksd the issue with the this is that the problematic elements are nested (ie obs within an encounter… or, actually, more specifically obs values stored as locations within an encounter)…)

@ibacher’s format allows us to keep the “results” response format as-is (just with a few elements missing) while adding in a new error element.

I can’t envision how we could switch to the format you suggest without doing a overall refactor of the OpenMRS REST response format?

Sure, and also this is a great point:

@mogoodrich - can you explain a little more why things are failing in the case of the Location, and not for other contrasting data types? Why don’t obs with valueNumeric or valueDrug suffer from the same issues? If it is because these are handled differently somewhere, then I would say we should extend this different handling to the location use case…

For valueNumeric and other primitives, I \believe it just ignores any custom representation component, would need to check.

valueDrug is a good point, and I have a faint memory of there being some discussion about this previously… will take a look…

Mark

I tested @mseaton @ibacher @mksd and then same problem occurs with with Value Drug. (ie, if you attempt to fetch an encounter and request a custom rep like I posted above, and an obs in that encounter is of value drug, you’ll get " Unknown property ‘names’ on class ‘class org.openmrs.Drug’"

I do think it should be relatively straightforward to do the fix @ibacher suggested, going to try it out now…

2 Likes

Quick update… I had a chat with @cioan and @mseaton and we discussed how the underlying issue is the ability for the “value” property to morph and be either location, a concept, a drug, a numeric, etc… I think we’ve had discussion before, and in hindsight there should be separate properties on the Obs, ie “valueText”, “valueDrug”, etc, like in the underlying data model. I’ll start a further conversation about this, but the idea is that we could switch to this new model while keep a deprecated “value” around for backwards compatibility.

@mseaton and @cioan also convinced me that rather than adding a separate error object we could just special case the “value” property of Obs and just return “null” if the custom rep doesn’t match. I’m hoping this will be straightforward to do, but I’m not sure how much the converter is aware of it’s context… going to look into this today. This would be a fix for the immediate issue.

Take care, Mark

2 Likes

We’ve long wanted to better handle exceptional values for observations. Is there any reason we couldn’t handle these as exceptional values the same way we’d like to be able to handle, for example, a viral load reported as “Inadequate specimen” – e.g., obs.dataAbsentReason = “error” or “unsupported”?

@burke maybe I’m missing something, but I’m not sure how that is related?

The underlying issue here is how the “value” property of Obs is polymorphic and could be a string, numeric, Concept, Drug, Location, etc… so therefore the client can’t request a custom representation since the model of the response is TBD. I think we’ve dealt with issues around this before?

We obviously can’t get rid of the “value” property, but we discussed proposing to deprecate that property, and start to model the Obs object in REST similar to how it’s modeled in the data model, with individual “valueText”, “valueDrug”, “valueNumeric” properties… (or, if we don’t want to fully deprecate “value”, at least make the other properties options you can request).

In the short-term, we are proposing a small hack fix to get around our current error (https://issues.openmrs.org/browse/RESTWS-816) but thoughts on the above solution for the long-term?

Take care, Mark

Just to throw this out there, but the “polymorphic value” you’re mentioning is basically how FHIR handles observation values. There are some differences since more things are stored in OpenMRS Obs than the FHIR spec contemplates representing, but it’s the same basic approach.

Thanks @ibacher… interesting… so if we following the FHIR pattern, we may be okay. Is there any parallel in FHIR about how it may handle the types of issues we are running into. Is there a datatype parameter that is required?

@mogoodrich Basically the way FHIR does this is the value[x] parameter will hold the data type. So, you’d get a object with:

{
...
valueDateTime: "...",
...
}

or

{
...
valueCodeableConcept: {..}
...
}

Rather than just a blanket value. Since there isn’t a blanket value parameter, the problem you’re seeing is pretty much non-existent (i.e., every value maps into the appropriate value[x] field).

Basically, it’s quite similar to a state we’re we’ve implemented what you’ve proposed and just dropped support for value.

1 Like

I was thinking that – for a short-term hack – instead of failing, the API could return an obs with an exceptional value (e.g., “Could not be rendered”).

I can see following this pattern for primitive data types, but I don’t see how it would scale to use valueLocation and valueDrug with the expectation that an implementation running a BacherMachine module that generates observations with datatype BacherResult would return observations with a valueBacher property.

Would it be more scalable to have a valueComplex or valueObject that would contain the result as an object (type determined by the observation’s type) rather than having clients trying to deal with near random property names?

1 Like

So, the thing here is that the FHIR spec really doesn’t contemplate having drugs or locations as observations, since those would normally be captured by non-observation resources (AllergyIntolerance / MedicationStatement / etc.), but if you did want to add additional types, the way FHIR would go about it would be to represent it as a CodeableConcept and stick it in the valueCodeableConcept field.

This is similar to what I thought was being discussed, i.e., that we generally have a pretty fixed set of data than be stored in an Obs (valueCoded, valueDrug, valueGroupId, valueDatetime, valueNumeric, valueModifier, valueText) and exposing that subset of values directly via the REST API seems to be reasonable (assuming we have a reasonable format to communicate those data in).

Similarly, I think it would be a mistake to extend Obs to support a valueBacher or w/e; that should likely be either a valueCoded or, in the worst case, a valueComplex with an appropriate handler.

1 Like