REST API contract for resources having attributes

Development
1

Continuing the discussion from Attributes for concepts: One of the topics that came up in relation to the “Concept Attribute” feature was the contract/wire-format of a resource that has attributes. In cases like Location, Provider resources, the contract has been typically as below. I am posting an example concept with attribute

GET: /openmrs/ws/rest/v1/conept/8c92f9c4-a31a-4569-9f19-3ebbb6f8fa2b?v=full

{
  "uuid": "8c92f9c4-a31a-4569-9f19-3ebbb6f8fa2b",
  "display": "Diarrhea",
  "name": {
    "display": "Diarrhea",
    "uuid": "6cf39275-64f7-47e5-bc2d-9810fbeb8e7c",
    "name": "Diarrhea",
    "locale": "en",
    "localePreferred": true,
    "conceptNameType": "FULLY_SPECIFIED",
    .....
  },
  ..... 
  "attributes": [
    {
      "display": "offline: true",
      "uuid": "1a66b9e7-bcf3-4f78-b7da-9968e14dec4f",
      "attributeType": {
        "uuid": "e2d847ea-7620-4d12-a896-b4b395e1addf",
        "display": "offline",
        .... 
      },
      "value": true,
      "voided": false,
      .... 
    },
    ..... 
   
  ],
  ..... 
}

@Burke asserted that the intention of attributes for any resource type is to extend the data model. Going by that logic, should the contract/wire-format be as below?

{
  "uuid": "8c92f9c4-a31a-4569-9f19-3ebbb6f8fa2b",
  "display": "Diarrhea",
  "name": {
    "display": "Diarrhea",
    "uuid": "6cf39275-64f7-47e5-bc2d-9810fbeb8e7c",
    "name": "Diarrhea",
    "locale": "en",
    "localePreferred": true,
    "conceptNameType": "FULLY_SPECIFIED",
    .....
  },
  ..... 
  "offline": {
      "display": "offline: true",
      "uuid": "1a66b9e7-bcf3-4f78-b7da-9968e14dec4f",
      "attributeType": {
        "uuid": "e2d847ea-7620-4d12-a896-b4b395e1addf",
        "display": "offline",
        .... 
      },
      "value": true,
      "voided": false,
      .... 
    },
  ..... 
}

Notice how the “offline” attribute has moved up to the top level, indicating a top level field. I would like to know what the community thinks.

PS: (“Offline” meaning whether this concept is available for offline or not, never mind the example!)

2

My personal take:

  • I think we should think about the API contract. Attributes can be renamed, retired and potentially purged. Such actions would break any existing contract with a client consuming the resource.

  • To counter the above, we may say that once we define the attribute, its indelible. (can’t be renamed or purged). We might say that - ‘you decide upfront how your concept model is going to be and stick to it’. However I doubt such will be the case ever. We would probably discover attributes later, and even refine or replace existing attribute.

  • Being a top level field, suggests that you can expect the field to be always there in the wire-format. Note, in our tables, if a location/provider/concept does not capture an attribute, we don’t add a row in EAV table. Meaning, the request would have to then expose something like below (for resources which didnt capture the attribute)

    { “uuid”: “8c92f9c4-a31a-4569-9f19-3ebbb6f8fa2b”, “display”: “Diarrhea”, “name”: { “display”: “Diarrhea”, “uuid”: “6cf39275-64f7-47e5-bc2d-9810fbeb8e7c”, “name”: “Diarrhea”, “locale”: “en”, “localePreferred”: true, “conceptNameType”: “FULLY_SPECIFIED”, … }, … “offline”: null, … }

Meaning, irrespective of whether the resource has this attribute or not, we still expose in the contract.

  • What happens when we have to add a new attribute after a client has written code against the defined contract? I know we can use annotations like @JsonIgnoreProperties(ignoreUnknown = true), but that still raises question about a resource contract being ‘agreed’ upon.
  • How would we version the resource when we change or add new attribute? - we can probably find a way around, but its going to get more & more complicated.

Considering the above (specifically versioning and breaking contract), I would rather not have this. APIs shouldn’t encourage such contract.

Thoughts?

3

It seems like you need a new column in the concept table instead of a dynamic attribute that can be created or deleted at will. This is the only way to define a stable contract between the API and its consumers IMHO.

Or maybe instead of a relational database you need a NoSQL one (MongoDB, ElasticSearch …) that has no metadata and allows any kind of data to be inserted. But the API problem will still remain of course.

1 Like
4

I think it would relatively easier to discuss these options if you pointed out the motivation for these changes. In other words what are the limitation of the existing format and how changing it to the suggested one is going to solve the problem?

5

The intent of *_attributes tables in the OpenMRS data model was to allow for local extensibility of the model. If, for example, your implementation needed a boolean concept.offline attribute that the community didn’t include in the data model, you could have it, almost as if you had convinced the community to ALTER TABLE concept ADD COLUMN offline boolean.

GET: /openmrs/ws/rest/v1/conept/8c92f9c4-a31a-4569-9f19-3ebbb6f8fa2b?v=full

{
  "uuid": "8c92f9c4-a31a-4569-9f19-3ebbb6f8fa2b",
  "display": "Diarrhea",
  "name": { "display": "Diarrhea", ... },
  ...,
  "attributes": [
    {
      "display": "offline: true",
      "uuid": "1a66b9e7-bcf3-4f78-b7da-9968e14dec4f",
      "attributeType": {
        "uuid": "e2d847ea-7620-4d12-a896-b4b395e1addf",
        "display": "offline",
        ...
      },
      "value": true,
      "voided": false,
      ...
    },
    ...
  ],
 ...
}

would become:

{
  "uuid": "8c92f9c4-a31a-4569-9f19-3ebbb6f8fa2b",
  "display": "Diarrhea",
  "name": { "display": "Diarrhea", ... },
  ...,
  "offline": true,
 ...
}

The contract would be the existence of the core (what comes out of the box) properties for concept. Local attributes (like concept.offline) would be additional properties used locally – i.e., they can only add to the core model. You don’t need the additional metadata explaining offline’s datatype (just like you don’t need additional metadata explaining concept.description is a string).

The difference between the current approach:

if (myConcept.attribute.find(function(it) {return it.attributeType.uuid == 'e2d847ea-7620-4d12-a896-b4b395e1addf';}).value) {
  // concept is offline
}

and treating attributes as attributes:

if (myConcept.offline) {
  // concept is offline
}
6

Why don’t we just follow the pattern for other resources where attributes like other child collections are treated as sub resources? I see what Burke is saying but i disagree because the reality is that attributes are a child collection

7

From the REST client point of view it’s just an attribute. I personally like Burke’s approach.

It should be transparent for the client if the offline attribute if physically a DB colum or a row in a subentity. Conceptually it’s just a concept attribute.

8

I agree with @angshuonline’s concerns, that allowing runtime changes to data to modify the top-level properties in our REST contract is a bad idea.

The simplified javascript in @burke’s example is a false benefit, because it means you’re writing javascript that expects one OpenMRS specific implementation’s database configuration.

@burke, I think you’re asking for two different things, that we should discuss separately:

  1. having attributes appear as top-level properties. (I disagree with this, for reasons stated above.)
  2. simplifying the REST representation so they appear like normal properties

I think the consumer benefit you’re describing comes from #2, not #1, though we explicitly asked the Bahmni devs to spike on #1 only, and not #2, on the last design call where we discussed this.

In this example, the simplified representation makes a lot of sense, e.g. if you have a ConceptAttributeType with datatype=boolean, minOccurs=1, and maxOccurs=1, then you could represent this as a simple boolean property, instead of an attribute with type and value.

A simple datatype with maxOccurs > 1 would also be easy to handle, by representing it as just an array of simple objects.

But it gets more complex if the CustomDatatype handler is storing the data outside of the concept_attribute.value_reference field. As a contrived example, imagine there’s a “patient handout” attribute with a PDF datatype which has patient handouts for specific diagnoses. How is this supposed to look in a simpler REST representation? (And the followup question will be: does our CustomDatatype interface in openmrs-core support this?)

9

In the context of the API definition, I would like to have attributes to something like what Burke suggested, albeit with the following considerations

  1. Attribute once defined can’t be renamed, can’t have white spaces, should have only alphanumeric characters etc.
  2. for serialization/deserialization benefits and for extensibility of a resource who’s contract/wire-format is agreed upon, instead of having the attributes at top level, have it like below

GET: /openmrs/ws/rest/v1/conept/b958665a-14d2-431d-a803-26a14210010e?v=full

{
  "uuid": "b958665a-14d2-431d-a803-26a14210010e",
  "display": "Chest X-Ray",
  "name": {
    "display": "Chest X-Ray",
    "uuid": "6cf39275-64f7-47e5-bc2d-9810fbeb8e7c",
    "name": "Chest X-Ray",
    "locale": "en",
    "localePreferred": true,
    "conceptNameType": "FULLY_SPECIFIED",
    .....
  },
  ..... 
  "attributes": [
      "offline" : true, 
      "orderable": true
  ],
  ..... 
}

This is a standard approach, for allowing extension to a resource contract (some call it properties, extensions). This implies that other that the attributes/extension section, all other fields are standard, and the attributes might not be fixed - just like OpenMRS does with attributes.

  1. If we do want the above format, there are other things to consider for the attribute sub-resource. Following the above format, the direct access to the attribute sub-resource would/should not be allowed. This means that only way, you can change/update an attribute value is re-submitting the concept itself.

instead of current approach where you can actually get access the sub-resource (attribute). [IMO, this should not be allowed]

GET: /openmrs/ws/rest/v1/conept/b958665a-14d2-431d-a803-26a14210010e/attribute

{
 "results": [
   {
     "display": "Orderable: true",
     "uuid": "b958665a-14d2-431d-a803-26a14210010e",
     "attributeType": {
       "uuid": "9169118c-bd9f-4aba-9757-c99309059033",
       "display": "Orderable",
       "links": [
         {
           "rel": "self",
           "uri": "http://yourdomain.com/openmrs/ws/rest/v1/conceptattributetype/9169118c-bd9f-4aba-9757-c99309059033"
         }
       ]
     },
     "value": true,
     "voided": false,
     "links": [
       {
         "rel": "self",
         "uri": "http://yourdomain.com/openmrs/ws/rest/v1/concept/d557815a-03c9-4824-9363-ac7f05eb0175/attribute/b958665a-14d2-431d-a803-26a14210010e"
       },
       {
         "rel": "full",
         "uri": "http://yourdomain.com/openmrs/ws/rest/v1/concept/d557815a-03c9-4824-9363-ac7f05eb0175/attribute/b958665a-14d2-431d-a803-26a14210010e?v=full"
       }
     ],
     "resourceVersion": "2.0"
   },
   {
     "display": "Offline: true",
     "uuid": "abc8665a-14d2-431d-a803-26a14210010e",
     "attributeType": {
       "uuid": "abcd118c-bd9f-4aba-9757-c99309059033",
       "display": "Offline",
       "links": [
         {
           "rel": "self",
           "uri": "http://yourdomain.com/openmrs/ws/rest/v1/conceptattributetype/abcd118c-bd9f-4aba-9757-c99309059033"
         }
       ]
     },
     "value": true,
     "voided": false,
     "links": [
       {
         "rel": "self",
         "uri": "http://yourdomain.com/openmrs/ws/rest/v1/concept/d557815a-03c9-4824-9363-ac7f05eb0175/attribute/abc8665a-14d2-431d-a803-26a14210010e"
       },
       {
         "rel": "full",
         "uri": "http://yourdomain.com/openmrs/ws/rest/v1/concept/d557815a-03c9-4824-9363-ac7f05eb0175/attribute/abc8665a-14d2-431d-a803-26a14210010e?v=full"
       }
     ],
     "resourceVersion": "2.0"
   }
 ]
}
  1. I agree with @darius comments about things getting complicated with attribute where there is a custom datatype handler, although we can raise the argument that teams writing such handler should define the contract and take care of GET/POST/PUT. In such cases I would assume that the handlers can not be randomly associated with an attribute.

While I like the simpler representations of the attributes: question is are we ready for such a change as part of OpenMRS core 2.0 and when? Are we saying that this is applicable for concept and not for location, visit, provider, person … ?

Alternatively, if we do retain the existing representations, then is this irreversible in future? Surely will raise a compatibility issue for existing consuming systems.

10

Just to point out, our AttributeType interface extends OpenmrsMetadata, so it has a name field that’s intended to be human-readable, and doesn’t enforce anything like “no spaces.” So, if we’re going to go this route, we would need to change that interface (and its existing implementations for location and provider) to add a key (or something like that).

Basically, I agree that the REST API that @burke is asking for is better than what we have now. But it’s not a trivial change, and I don’t think it makes sense to introduce it for concept without also changing location and provider to behave in the same way. (And that could be a breaking change to existing REST clients.)

So, my preference is to close the ticket on concept_attributes with the work done so far, and create a new ticket about refactoring how attributes are done in the REST API. And we can decide whether it’s worth doing this work in the same OpenMRS version as the concept_attributes are done in.

11

We have raised a pull request for now with the older contract: https://github.com/openmrs/openmrs-module-webservices.rest/pull/213/ .

12

Merged. Thanks @preethi_s and team! :smile:

Whoever wants to try it out, feel free to access: http://uat01.openmrs.org:8080/openmrs/module/webservices/rest/apiDocs.htm or http://uat01.openmrs.org:8080/openmrs/module/webservices/rest/test.htm