Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Endpoint for getting contact by UUID #9065

Open
jkuester opened this issue Apr 24, 2024 · 5 comments · May be fixed by #9090
Open

API Endpoint for getting contact by UUID #9065

jkuester opened this issue Apr 24, 2024 · 5 comments · May be fixed by #9090
Assignees
Labels
Type: Feature Add something new

Comments

@jkuester
Copy link
Contributor

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

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

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 #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 jkuester added the Type: Feature Add something new label Apr 24, 2024
@jkuester jkuester self-assigned this Apr 25, 2024
@jkuester
Copy link
Contributor Author

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

     const 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
Copy link
Member

@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
Copy link
Contributor Author

👍 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
Copy link
Contributor Author

jkuester commented May 2, 2024

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
Copy link
Contributor

lorerod commented May 7, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Add something new
Projects
Status: This Week's commitments
Development

Successfully merging a pull request may close this issue.

3 participants