feat(#9065): add cht-datasource to support getting contact by uuid

[jkuester]

Description

Closes #9065

cht-datasource

This PR converts shared-libs/cht-script-api into shared-libs/cht-datasource as a general purpose data-access layer intended for use within cht-core both in server and the client-side code as well as within custom config code for (tasks, targets, etc).

There are two modes of functionality exported from cht-datasource. Both require you to first acquire a DataContext (either remote or local depending on if you can interact directly with the CHT api server or if offline functionality is required).

import { getRemoteDataContext, getLocalDataContext } from '@medic/cht-datasource';

const dataContext = isOnlineOnly
  ? getRemoteDataContext(...)
  : getLocalDataContext(...);

Imperative

The imperative mode provides a hierarchical factory object that you can drill down into and interact with the datasource.

import { getDatasource } from '@medic/cht-datasource';

const datasource = getDatasource(dataContext);
const myUuid = 'my-uuid';
const myPerson = await datasource.v1.person.getByUuid(myUuid);

This mode is useful for exposing functionality to custom configuration code (tasks, targets, etc) by passing the datasource object directly into the custom function.

Declarative

The declarative mode provides more flexible access to API functions. The dataContext must be curried when using the functions.

import { Person, Qualifier } from '@medic/cht-datasource';

const getPerson = Person.V1.get(dataContext);
const myUuid = 'my-uuid';
const myPerson = await getPerson(Qualifier.byUuid(uuid));

Code review checklist

• Readable: Concise, well named, follows the style guide, documented if necessary.
• Documented: Configuration and user documentation on cht-docs
• Tested: Unit and/or e2e where appropriate
• Internationalised: All user facing text
• Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

Compose URLs

If Build CI hasn't passed, these may 404:

• Core
• CouchDB Single
• CouchDB Cluster

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

[m5r]

+1 on what Gareth said about the API design it makes more sense to have the two functions implement the same interface. I wish the TS team had implemented microsoft/TypeScript#420, it would have been pretty nice in our scenario but we can surely make cht-datasource work without!

[jkuester]

👍 This has been all part of my evil plan to start with crazy amounts of jsdoc and then agree to "moderate" things back to where I actually wanted them.... 😆

But seriously, 💯 agree regarding the unnecessary redundant docs. Most of those got added late on Friday afternoon in my feverish zeal for consistency 😅, but are actually not required by current linting rules. I have deleted those. I also removed the linting requirement for documenting namespaces. I think we will eventually want to mark previous namespaces as @deprecated, but we should not really needs docs for current ones. I will push back a bit on the SettingsService type documentation. What I had originally written was not very helpful documentation, but I think that proper docs here could be actually useful vs just not having any documentation. I tried to illustrate this by adding more helpful context to the docs. Let me know what you think!

Unless we can tweak the linter to only fail when appropriate let's leave this to the humans (coder + reviewer) to catch.

I think the big question here is if folks agree with my assessment on the value of the /** @internal */ docs. I don't see any way to tweak the linting rules so we could avoid these, but as I said before, I find them valuable in their own right. (But my tolerance threshold for "spurious" content in the codebase is def higher than many other folks....) I am happy to just drop the require-jsdoc rule entirely if folks just cannot stand the extra /** @internal */ docs.

On the other hand, if we are okay with at least some /** @internal */ docs, there are a number of other levers we can pull to further tweak when docs are required. Specifically we could decide which kinds of exported things we want to require docs on:

• Functions - seems like this is a clear "yes" if we want docs anywhere...
• Types/Interfaces - this feels like a place where we might decide we should not require docs (since in many places these docs could be redundant (e.g. the SettingsService).
• Interface property signatures - (e.g. require docs for each field/method in an interface). This I left off from the start since it feels excessive, but can turn it on if folks think it would be useful...

[sugat009]

@jkuester Not to be the variable name reviewer guy 😅 but usage-wise would it be possible to have the version access name consistent?

Meaning that for the declarative one, we do:

import { getDatasource } from '@medic/cht-datasource';

const myPerson = await getDatasource().v1.person.getByUuid(myUuid);

Notice the lowercase v1 whereas, for imperative, we do:

import { Person, Qualifier } from '@medic/cht-datasource';

const getPerson = Person.V1.get(dataContext);

An uppercase V1.
Would it be possible to have both of them either as v1 or V1?

And as for the JSdoc conversation, IMO having additional/extra comments/docs is always useful than having fewer docs, and IDEs and editors these days can always fold them out if the concern is that they take up screen space. 😀

[jkuester]

Not to be the variable name reviewer guy

😆 @sugat009 the names are one of the the most important parts of a new API, so please, by all means, be the variable name reviewer guy! 💯 I agree that more consistency here would be nice, so I have updated the namespaces to be a lowercase v1. 👍

[jkuester]

Setting this in the top level so we can have nice things in places like our api unit tests....

[jkuester]

I have added this method after much pain and suffering. It is not obvious, but using this function dramatically simplifies unit testing for code that consumes the imperative-style api. You can see the unit tests for the person API controller as an example, but basically this function provides the abstraction point where sinon can be used to easily inject a stub implementation.

There are other approaches that could be used (and if you can think of a better one that allows for simple testing, I am all ears), but after many hours of tinkering this has the best balance of simplicity, flexibility, and testability of everything I have tried. (Though I am not super happy with the name get. It is short and does make semantic sense with how we intend this api to be used (e.g. to access the inner function by currying the context), but I don't love it. Very interested to hear if anyone has a better name!)

[jkuester]

You may think it would make more sense to include this class in the src/libs/data-context.ts file and you would be correct. Unfortunately, putting it there (in the same file as the DataContext and other supporting functions) causes some very weird circular dependency issues that all get magically resolved when I move this here....

[jkuester]

@lorerod my initial approach here is to end up with the minimal amount of new integration tests by combining tests for the cht-datasource with tests for the person API controller. Basically, these tests use cht-datasource with the remote data context to call the controller via the /api/v1/person/:uuid REST endpoint. Then the api controller code uses cht-datasource with the local data context (aka Pouch) to do the data interactions with Couch. So we end up being about to test the following "integration":

cht-datasource (remote) -> api Person controller -> cht-datasource (local) -> Couch

To me, this seems like a valid way to test everything at the same time without compromising the effectiveness/fidelity of the tests at all. Using the remote cht-datasource to make the REST calls is functionally not much different than using the utils.request function to make the REST calls... But, IDK, let me know what you think about all this!

[jkuester]

Also, just because we use cht-datasource for some of the tests does not mean we need to use it for all of them. With the way things are setup now, it would be tricky to use cht-datasource with a different user, so I just opted for the utils.request function here.

[jkuester]

After some tinkering and thinking, I realized we are in a pretty stable place for authentication in cht-datasource.

Webapp (online) and the admin app are using the remote adapter which makes fetch calls from the browser. These fetch calls can be made relative to the "current page" (so the URL does not need to be provided) and they inherit the cookies for the browser session (so we don't need to worry about authentication).

Sentinel, api, and Webapp (offline) are using the local adapter which is just Pouch under the hood and the injected Pouch instances are already authenticated.

---

However, our integration tests (and any future 3rd-party usage of cht-datasource) need to provide a root URL to use for calling the api server (done via the getRemoteDataContext function). Where things get tricky for the tests is the NodeJS implementation of the fetch api does not supply cookies from the Set-Cookie of previous responses when it makes new requests (this happens automatically in the browser environment). So, the choices were either:

• update the cht-datasource code to make it possible to inject the session cookie just so the tests would work
• or get a bit creative with fetch during the test runs to be sure the cookies are there when fetch is called.

I have opted for the latter option for now. It keeps the impl code the most clean. This will probably need to be re-visited in the future, either when we go to replace all our request-promise-native with fetch or when we release cht-datasource for 3rd-party consumption and we need a more proper way to do authentication. But, for now, this seems like a reasonable approach especially since we can just piggy-back on our existing rpn cookie jar....

[jkuester]

@m5r @sugat009 This is ready for another review! Actually, IMHO this code is "complete" in that I would be comfortable merging it to master. It is the complete flow for adding an endpoint to "get person without lineage" along with the necessary unit/integration tests. That being said, before we actually merge this, we want to also include:

• Get person without lineage
• Get person with lineage
• Get place without lineage
• Get place with lineage

(These other three should be much quicker to add now that the patterns/utils are stabilizing).