API Endpoint for getting contact by UUID

[jkuester]

Is your feature request related to a problem? Please describe.

Currently the only REST api options for getting contact data are:

• GET /api/v2/export/contacts which returns a little bit of data about all the contacts
• GET /api/v1/contacts-by-phone - which returns a list of fully hydrated contacts for a given phone number

A more basic API is needed for being able to retrieve contact data for just a specific contact. (e.g. you just got the data for a user via GET /api/v2/users/{{username}} and now you have their contact_id value, but you want to get all the data for that contact.)

Describe the solution you'd like

We already have POST /api/v1/places and POST /api/v1/places, so we could have GET /api/v1/places/{{id}} and GET /api/v1/people/{{id}}.

Additional context

As noted in https://github.com/medic/cht-core/issues/8889, we should implement the core data access aspects of this via a new data source library that can be shared between API and webapp with a well defined interface.

cc @m5r @mrjones-plip

[jkuester]

Okay, I figured I would throw out a bit of a technical design. This is all entirely speculative (riffing on the content/discussion in this doc), but I figured the most practical way to proceed here was to start with a specific proposal that we could discuss.

@garethbowen the main goal here is to hammer out an over-arching design. Once we have an idea of what that should look like, I can iterate on this proposal with more detail. I am happy to jump on a call to discuss this further if that would be more effective!

Tech Design

The actual api code for this issue is trivial, so this proposal is mostly focused on the structure/content of the new "Webapp API library" (that I am calling cht-datasource).

cht-datasource

It seems to make sense to put this new lib into the shared-libs folder where it can be referenced by api/sentinel/webapp.

• index.ts

  onst init = (sourceDb) => {...}
  

• doc.ts

  interface Doc {
    _id: string;
    _rev: string;
    type: string;
    [other: string]: unknown;
  }
  

• contact.ts

  interface Contact extends Doc {
    contact_type?: string;
    name: string;
    parent_id?: string;
    reported_date: Date;
  }
  
  interface ContactWithLineage extends Contact {
    parent?: ContactWithLineage;
  }
  
  const getContactWithLineage = (contact: Contact): ContactWithLineage => {...}
  

• person.ts

  interface AbstractPerson {
    date_of_birth?: Date;
    phone?: string;
    patient_id?: string;
    sex?: string;
  }
  
  interface Person extends Contact, AbstractPerson { }
  interface PersonWithLineage extends ContactWithLineage, AbstractPerson { }
  
  const getPerson = (args: { uuid?: string }): Person => {...};
  const getPersonWithLineage = (args: { uuid?: string }): PersonWithLineage => {...};
  

Usage:

• api - New controller for getting a contact by uuid
• webapp - Could be used in the lineage-model-generator.service.ts instead of the existing mix of on-off code and shared-libs/lineage
  • Ideally we could replace all usages of shared-libs/lineage...

Additional questions:

• [ ] Should we expose everything from the library in a namespace?
• [ ] Should we also implement CREATE/UPDATE person as a part of this project, or can that be done later (and just stick to READ in this issue)?
• [ ] Should we consider using a schema validation library (like Zod)? Might be useful to validate incoming data at the edges of the library (both data being read from Couch and data being provided for CREATE/UPDATE)?

[garethbowen]

@jkuester Thanks for taking the initiative and kicking this off - it's been on my todo list too long!

Should we expose everything from the library in a namespace?

I think so, mostly from the perspective of future proofing. There's a good chance it won't be necessary but if it is found to be useful in future then migrating would be potentially difficult. APIs must also be versioned/versionable and I wonder if namespaces are the way to achieve this too.

Should we also implement CREATE/UPDATE person as a part of this project, or can that be done later (and just stick to READ in this issue)?

Keep this as an MVP IMO.

Should we consider using a schema validation library?

It's very difficult to introduce validation into an existing system, as we just found out. It may work if we limit it to just the pieces that are absolutely necessary - if we know the CHT will break if you create a contact without a contact_type then we should fail early. But avoid the temptation of validating nice to haves for legacy data. I think this is another thing we could push out of the MVP.

Some other thoughts...

• How will we use this in node services (API and sentinel) and configuration? The options are for the shared-lib to be compiled into JS which node imports, or adding a TS compile step to the node service.
• I'm not sure I'm reading your pseydo code correctly but does the getPerson implementation take an object with an optional uuid property? I don't understand what would happen if you chose not to provide it. Wouldn't this make a better API: const getPerson(uuid: string): Person => {...} ? If we need to differentiate between different ways of getting Persons then I'd go towards multiple APIs with strict parameters rather than one API with multiple params, eg: getByUuid and getByShortcode is preferable to get(uuid?, shortcode?).

I think it would be useful to put this in a Google Doc to get more feedback from the wider team - some of the decisions being made here will affect anyone working on Core for some time to come.

[jkuester]

:+1: Incorporating all your suggestions into a new iteration of this documentation that is back in your original proposal document.

If we need to differentiate between different ways of getting Persons then I'd go towards multiple APIs with strict parameters rather than one API with multiple params, eg: getByUuid and getByShortcode is preferable to get(uuid?, shortcode?).

Yes, the shortcode example was precisely what I was considering, though I agree, it will be less confusing for consumers to have a more rigid function definition instead of accepting a magical args object that we then have to validate... I am fine with getByUuid and getByShortcode. Names start getting a bit long once you factor in "withLineage" etc. I have added a third alternative to the new revision in the doc, but can always fall back to basic functions if that seems best.

[jkuester]

Updated tech design - specifically for this MVP issue

This is based on conversation and iteration in the proposal document.

• shared-libs:
  • cht-script-api: Rename to cht-datasource

    • Add tsconfig to allow for code/tests to be written in TS.

      • Save transpiled code into build/cht-datasource - this is where api/sentinel should reference for the JS lib

    • Update the index file to be TS and look something like:

      module.exports = (source: PouchDB | string) => {
        v1: {
          hasPermissions,
          hasAnyPermission,
          person: {
            get,
            getWithLineage
          }
        }
      };
      

      • Note that we will have to update all the places where cht-script-api is currently getting used to call the function (since the original export from index was just an object. We need to switch to a function to allow for injecting the DB. This needs to happen at the top level of the exports so we can call it before providing the returned object to the former consumers of cht-script-api.)

    • Add new person.ts file that exports get and getWithLineage.

      • This code should be strongly typed with everything living in a V1 namespace.
      • The returned data needs to be backwards compatible to what is provided from shared-libs/lineage.
      • Under the hood, we need to support two data access adapters (see the diagram at the bottom):
        1. If a PouchDB instance is provided, use it. This is how offline clients or services with direct access to Couch (api/sentinel) will communicate.
        2. If no DB instance is provided, we should use fetch to interact with the CHT REST apis. This is how online users and future 3rd party services will communicate.
           • TODO Still need to figure out how authentication will work here. Need to get session cookie somehow...

    • Have a CI job that builds the jsdoc for the library. We don't need to do anything with the doc yet, but want to validate that it exists and is well-formed.

  • lineage:
    • Remove this package and update all references to use cht-datasource
• api:
  • Update build scripts to transpile cht-datasource before building api to make sure we are running on fresh code
  • Add new controllers/person.js to map request to GET /api/v1/person/{{id}}
• webapp:
  • Update tsconfig to add path references for cht-datasource so that webapp can depend directly on the TS source files.
• tests:
  • integration:
    • cht-datasource:
      • person.spec.ts
        • Integration tests should use library to connect to instance of api (which in turn is using the library via Pouch). I think this should be a sufficient for testing the code for both adapters.
        • I also think this might mean we don't need a test/integration/api/controllers test for the new controller (since it will already be covered in these tests. @lorerod I would appreciate your input on this! Given the proposed flow in the diagram below, you can see that using the fetch adapter flow in cht-datasource to call the API server will also end up passing through the Pouch adapter to get to the DB...
• package.json:
  • Add cht-datasource-dev script to build/watch cht-datasource
  • Update dev-* scripts to watch relevant directories for changes to the cht-datasource code

Data flow with pouch vs fetch adapters

@garethbowen, does this diagram match what you were thinking as far as the usage of separate adapters in webapp?

flowchart TD

subgraph webapp
  subgraph webapp/cht-datasource[cht-datasource]
    subgraph webapp/cht-datasource/online[Online User]
        webapp/cht-datasource/online/fetch[fetch]
    end
    subgraph webapp/cht-datasource/offline[Offline User]
        webapp/cht-datasource/offline/pouch[PouchDB]
    end
  end
end

subgraph api
  subgraph api/cht-datasource[cht-datasource]
    api/cht-datasource/pouch[PouchDB]
  end
end
couchdb[(CouchDB)]

api/cht-datasource/pouch ---> couchdb
webapp/cht-datasource/offline/pouch ---> couchdb
webapp/cht-datasource/online/fetch --->|GET api/v1/person| api

[lorerod]

@jkuester, we don't need an exhaustive set of tests for the controller, but a simple test to validate that everything is working properly should be okay. I may need to see more progress on this to understand it clearly. Also, if we already have unit tests on cht-script-api, we should refactor those with the changes.

[jkuester]

@garethbowen @m5r after doing all this work to stand up api/v1/person and api/v1/place endpoints I have run into some difficulties as I look towards updating some webapp code to consume cht-datasource. Namely, in webapp we often track the contact that we have in context by its _id value (that gets read from the route). We do not know the type of that contact until after reading the contact from the DB (which is the functionality that cht-datasource is supposed to be replacing). However, we have not added any generic api/v1/contact endpoint that we could call with an ID to just get generic contact data. Seems like our options include:

• Rework the state managed in webapp to somehow also track the type of the contact in context (or at least track if it is a person or place)
• Add a contact endpoint for getting data for a generic contact.
• Add an endpoint just for checking the type of a contact for a particular id (e.g.api/v1/entity/${id}/type) - not great since this still would have to load the doc from the DB to get the type, but not the worst since we are just getting by id.

Any thoughts here? Am I missing something?

[jkuester]

Splitting off the get-place functionality into https://github.com/medic/cht-core/issues/9194 so that we can close this issue and ship on 4.9.

[jkuester]

Closed by https://github.com/medic/cht-core/pull/9090

[latin-panda]

Thanks a lot @jkuester!!

[jkuester]

Met with @garethbowen and @m5r today to discuss https://github.com/medic/cht-core/issues/9065#issuecomment-2166842427. The primary conclusions included:

• Our current authorization check with the can_view_contacts permission is insufficient. An offline user with the can_view_contacts permission should only be able to view the contacts they are allowed to replicate. However, our current implementation of the person endpoint is not checking if the user has replication access to the contact they are trying to view.
  • For the sake of simplicity right now (and to avoid unintentionally leaving any other security loopholes) we should block offline users from accessing the new endpoint. Only online/admin users should be able to call it. This should not result in a reduction of the intended functionality since offline users in webapp should always be using the local data context anyways and not trying to call though via the remote context.
  • Also, just want to acknowledge that this is conceptually related to limited online user access. We are not going to try to solve that problem now.
• With the current CHT data model, we often have a uuid of a contact, but do not know if that contact is a person or a place. Because of this, we need to support a contact endpoint that can be used for reading a generic contact. We can also continue to have person and place endpoints that can be used for getting guaranteed specific types of contacts.
• Finally, we concluded that we ultimately should also support getting a generic entity ("Doc") by uuid. While this may be undesirable from an encapsulation perspective, it will be incredibly useful as a stop-gap solution for a number of workflows. (Better to make a compromise so that our API is actually usable vs having a conceptually pure API that only covers 80% of the needed workflows...)

Of these conclusions, I think that the only one that needs to be address in this issue is the first.