feat(#9238): add functionality of getting people as an AsyncGenerator in cht-datasource

[sugat009]

Description

Add functionality of getting people as an AsyncGenerator in cht-datasource.

Ticket: #9238

Usage:

const getAllIterator = Person.v1.getAll(ctx)(Qualifier.byContactType('person'));
for await (const page of getAllIterator) {
  for (const doc of page) {
    ...
  }
}

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

License

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

[sugat009]

@jkuester @m5r I've created a draft PR for discussion.
Currently, the state of the code is that the natural setup of the functionality has been done according to the pattern in cht-datasource.
The current plan is to write the iterable logic in this function.
In contrast, the remote logic has been written in this function. The thing that I'm thinking about right now is that if a new endpoint different than the one in which the getPage(remote) would be requesting for i.e. /api/v1/person is necessary like a /api/v1/personAsync or something.
What do you think about this?

[sugat009]

Additionally, the plan to implement the iterable for the local function Person.v1.getAll is to first query the view medic-client/contacts_by_type?<personType> for total row count and then, subsequently query page by page using the already existing Person.v1.getPage function until the total row count is reached.

Update:
Would it be better to continue paginating through the results until we encounter a page that returns no data? At that point, the Person.v1.getAll function would terminate its operation.

[jkuester]

So, my thinking here was that we might be able to get away without having any separate remote/local logic at all. Instead we implement getAll in src/person.ts in such a way that it just calls the getPage function in src/person.ts. I am not sure if there is some extra complexity that would prevent this, but at first glace it seems possible. With this approach, we should be able to tell when we have reached the end of the data and can stop yielding when we get back an empty page, or a page with a length < 100.

Should not need any separate REST endpoint for this functionality. In reality, all we are implementing here is a TS convenience wrapper over top of getPage.

[sugat009]

So, my thinking here was that we might be able to get away without having any separate remote/local logic at all.

That is an interesting one and I agree that the whole implementation would be simpler and is also feasible. I might be a bit out of context here but why not the remote logic?

[jkuester]

but why not the remote logic?

If the src/person.ts/getAll function is just calling the src/person.ts/getPage, then all of the remote vs local behavior will be handled by src/person.ts/getPage and can be transparent to src/person.ts/getAll. getAll is just getting arrays of Persons back. It should not even need to care if they are coming from the remote or the local adapter. At least that is my thinking anyway... 🤔

[jkuester]

If we know that we have reached the end of the results, we should not return a cursor value.

  readonly cursor?: string;

[m5r]

Good catch! I believe cursor's type should be

interface Page {
  readonly cursor: string | null;
}

That's more explicit to express "you've reached the end"

[sugat009]

I think we should be more explicit and return "-1" and not nothing to indicate the end of iteration.

[sugat009]

@m5r and I commented simultaneously, null should do as well.

[jkuester]

Solid logic here! 👍 I think we can tighten up the cursor calculation a bit (at least reduce the nesting if-statements):

const fetchAndFilter = async (
  currentLimit: number,
  currentSkip: number,
  currentPersons: Person.v1.Person[] = []
): Promise<Page<Person.v1.Person>> => {
  const docs = await getDocsByPage([personType.contactType], currentLimit, currentSkip);
  const noMoreResults = docs.length < currentLimit;
  const newPersons = docs.filter((doc): doc is Person.v1.Person => isPerson(settings, doc, doc?._id));

  // Need to slice here because we retrieve extra docs on re-fetches and may end up with too many.
  const overFetchCount = currentPersons.length + newPersons.length - limit || 0;
  const totalPersons = [...currentPersons, ...newPersons].slice(0, limit);
  if (noMoreResults) {
    return { data: totalPersons };
  }
  if (totalPersons.length === limit) {
    const nextSkip =  currentSkip + currentLimit - overFetchCount;
    return { data: totalPersons, cursor: (nextSkip).toString() };
  }

  // Re-fetch twice as many docs as we need to limit number of recursions
  const missingCount = currentLimit - newPersons.length;
  logger.debug(`Found [${missingCount}] invalid persons. Re-fetching additional records.`);
  const nextLimit = missingCount * 2;
  const nextSkip = currentSkip + currentLimit;
  return fetchAndFilter(nextLimit, nextSkip, totalPersons);
};

return fetchAndFilter(limit, skip);

[m5r]

Nothing to add to Josh's comments, I'll approve to not block merging after Josh's approval

[sugat009]

I am closing this PR, as I might have messed up somewhere during conflict resolution, creating many duplicate commits.
Reverted the commits before conflict resolution instead.