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

feat(#9193): add functionality of getting places as pages or async iterables in cht-datasource #9368

Merged
merged 12 commits into from
Sep 2, 2024
24 changes: 20 additions & 4 deletions api/src/controllers/place.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,35 @@ const getPlace = ({ with_lineage }) => ctx.bind(
: Place.v1.get
);

const getPageByType = () => ctx.bind(Place.v1.getPage);

const checkUserPermissions = async (req) => {
const userCtx = await auth.getUserCtx(req);
if (!auth.isOnlineOnly(userCtx) || !auth.hasAllPermissions(userCtx, 'can_view_contacts')) {
return Promise.reject({ code: 403, message: 'Insufficient privileges' });
}
};

module.exports = {
v1: {
get: serverUtils.doOrError(async (req, res) => {
const userCtx = await auth.getUserCtx(req);
if (!auth.isOnlineOnly(userCtx) || !auth.hasAllPermissions(userCtx, 'can_view_contacts')) {
return Promise.reject({ code: 403, message: 'Insufficient privileges' });
}
await checkUserPermissions(req);
const { uuid } = req.params;
const place = await getPlace(req.query)(Qualifier.byUuid(uuid));
if (!place) {
return serverUtils.error({ status: 404, message: 'Place not found' }, req, res);
}
return res.json(place);
}),
getAll: serverUtils.doOrError(async (req, res) => {
await checkUserPermissions(req);

const placeType = Qualifier.byContactType(req.query.placeType);
sugat009 marked this conversation as resolved.
Show resolved Hide resolved
const limit = req.query.limit ? Number(req.query.limit) : req.query.limit;

const docs = await getPageByType()( placeType, req.query.cursor, limit );

return res.json(docs);
})
}
};
1 change: 1 addition & 0 deletions api/src/routing.js
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,7 @@ app.postJson('/api/v1/places/:id', function(req, res) {
.catch(err => serverUtils.error(err, req, res));
});

app.get('/api/v1/place', place.v1.getAll);
sugat009 marked this conversation as resolved.
Show resolved Hide resolved
app.get('/api/v1/place/:uuid', place.v1.get);

app.postJson('/api/v1/people', function(req, res) {
Expand Down
2 changes: 1 addition & 1 deletion api/tests/mocha/controllers/person.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ describe('Person Controller', () => {
expect(serverUtilsError.calledOnceWithExactly(error, req, res)).to.be.true;
});

it('returns 400 error when argument is invalid', async () => {
it('returns 400 error when personType is invalid', async () => {
const err = new InvalidArgumentError(`Invalid contact type: [${invalidPersonType}]`);
isOnlineOnly.returns(true);
hasAllPermissions.returns(true);
Expand Down
109 changes: 108 additions & 1 deletion api/tests/mocha/controllers/place.spec.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const sinon = require('sinon');
const { expect } = require('chai');
const { Place, Qualifier } = require('@medic/cht-datasource');
const { Place, Qualifier, InvalidArgumentError} = require('@medic/cht-datasource');
const auth = require('../../../src/auth');
const controller = require('../../../src/controllers/place');
const dataContext = require('../../../src/services/data-context');
Expand Down Expand Up @@ -154,5 +154,112 @@ describe('Place Controller', () => {
expect(serverUtilsError.calledOnceWithExactly(error, req, res)).to.be.true;
});
});

describe('getAll', () => {
let placeGetPageByType;
let qualifierByContactType;
const placeType = 'place';
const invalidPlaceType = 'invalidPlace';
const placeTypeQualifier = { contactType: placeType };
const place = { name: 'Clinic' };
const limit = 100;
const cursor = null;
const places = Array.from({ length: 3 }, () => ({ ...place }));

beforeEach(() => {
req = {
query: {
placeType,
cursor,
limit,
}
};
placeGetPageByType = sinon.stub();
qualifierByContactType = sinon.stub(Qualifier, 'byContactType');
dataContextBind.withArgs(Place.v1.getPage).returns(placeGetPageByType);
qualifierByContactType.returns(placeTypeQualifier);
});

afterEach(() => {
expect(getUserCtx.calledOnceWithExactly(req)).to.be.true;
expect(isOnlineOnly.calledOnceWithExactly(userCtx)).to.be.true;
});

it('returns a page of places with correct query params', async () => {
isOnlineOnly.returns(true);
hasAllPermissions.returns(true);
placeGetPageByType.resolves(places);

await controller.v1.getAll(req, res);

expect(hasAllPermissions.calledOnceWithExactly(userCtx, 'can_view_contacts')).to.be.true;
expect(qualifierByContactType.calledOnceWithExactly(req.query.placeType)).to.be.true;
expect(dataContextBind.calledOnceWithExactly(Place.v1.getPage)).to.be.true;
expect(placeGetPageByType.calledOnceWithExactly(placeTypeQualifier, cursor, limit)).to.be.true;
expect(res.json.calledOnceWithExactly(places)).to.be.true;
expect(serverUtilsError.notCalled).to.be.true;
});

it('returns error if user does not have can_view_contacts permission', async () => {
const error = { code: 403, message: 'Insufficient privileges' };
isOnlineOnly.returns(true);
hasAllPermissions.returns(false);

await controller.v1.getAll(req, res);

expect(hasAllPermissions.calledOnceWithExactly(userCtx, 'can_view_contacts')).to.be.true;
expect(dataContextBind.notCalled).to.be.true;
expect(qualifierByContactType.notCalled).to.be.true;
expect(placeGetPageByType.notCalled).to.be.true;
expect(res.json.notCalled).to.be.true;
expect(serverUtilsError.calledOnceWithExactly(error, req, res)).to.be.true;
});

it('returns error if not an online user', async () => {
const error = { code: 403, message: 'Insufficient privileges' };
isOnlineOnly.returns(false);

await controller.v1.getAll(req, res);

expect(hasAllPermissions.notCalled).to.be.true;
expect(dataContextBind.notCalled).to.be.true;
expect(qualifierByContactType.notCalled).to.be.true;
expect(placeGetPageByType.notCalled).to.be.true;
expect(res.json.notCalled).to.be.true;
expect(serverUtilsError.calledOnceWithExactly(error, req, res)).to.be.true;
});

it('returns 400 error when placeType is invalid', async () => {
const err = new InvalidArgumentError(`Invalid contact type: [${invalidPlaceType}].`);
isOnlineOnly.returns(true);
hasAllPermissions.returns(true);
placeGetPageByType.throws(err);

await controller.v1.getAll(req, res);

expect(hasAllPermissions.calledOnceWithExactly(userCtx, 'can_view_contacts')).to.be.true;
expect(qualifierByContactType.calledOnceWithExactly(req.query.placeType)).to.be.true;
expect(dataContextBind.calledOnceWithExactly(Place.v1.getPage)).to.be.true;
expect(placeGetPageByType.calledOnceWithExactly(placeTypeQualifier, cursor, limit)).to.be.true;
expect(res.json.notCalled).to.be.true;
expect(serverUtilsError.calledOnceWithExactly(err, req, res)).to.be.true;
});

it('rethrows error in case of other errors', async () => {
const err = new Error('error');
isOnlineOnly.returns(true);
hasAllPermissions.returns(true);
placeGetPageByType.throws(err);

await controller.v1.getAll(req, res);

expect(hasAllPermissions.calledOnceWithExactly(userCtx, 'can_view_contacts')).to.be.true;
expect(qualifierByContactType.calledOnceWithExactly(req.query.placeType)).to.be.true;
expect(dataContextBind.calledOnceWithExactly(Place.v1.getPage)).to.be.true;
expect(placeGetPageByType.calledOnceWithExactly(placeTypeQualifier, cursor, limit)).to.be.true;
expect(res.json.notCalled).to.be.true;
expect(serverUtilsError.calledOnceWithExactly(err, req, res)).to.be.true;
});
});
});
});
28 changes: 28 additions & 0 deletions shared-libs/cht-datasource/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,34 @@ export const getDatasource = (ctx: DataContext) => {
* @throws Error if no UUID is provided
*/
getByUuidWithLineage: (uuid: string) => ctx.bind(Place.v1.getWithLineage)(Qualifier.byUuid(uuid)),

/**
* Returns an array of places for the provided page specifications.
* @param placeType the type of place to return
* @param cursor the token identifying which page to retrieve. A `null` value indicates the first page should be
* returned. Subsequent pages can be retrieved by providing the cursor returned with the previous page.
* @param limit the maximum number of place to return. Default is 100.
* @returns a page of place for the provided specifications
sugat009 marked this conversation as resolved.
Show resolved Hide resolved
* @throws Error if no type is provided or if the type is not for a place
sugat009 marked this conversation as resolved.
Show resolved Hide resolved
* @throws Error if the provided limit is `<= 0`
* @throws Error if the provided cursor is not a valid page token or `null`
* @see {@link getByType} which provides the same data, but without having to manually account for paging
*/
getPageByType: (
placeType: string,
cursor: Nullable<string> = null,
limit = 100
) => ctx.bind(Place.v1.getPage)(
Qualifier.byContactType(placeType), cursor, limit
),

/**
* Returns a generator for fetching all places with the given type.
* @param placeType the type of place to return
* @returns a generator for fetching all places with the given type
* @throws Error if no type if provided or if the type is not for a place
*/
getByType: (placeType: string) => ctx.bind(Place.v1.getAll)(Qualifier.byContactType(placeType))
},
person: {
/**
Expand Down
71 changes: 71 additions & 0 deletions shared-libs/cht-datasource/src/libs/core.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import { DataContext } from './data-context';
import { Doc } from './doc';
import { SettingsService } from '../local/libs/data-context';
import logger from '@medic/logger';
import { ContactTypeQualifier, isContactTypeQualifier } from '../qualifier';
import { InvalidArgumentError } from './error';

/**
* A value that could be `null`.
Expand Down Expand Up @@ -144,3 +149,69 @@ export const getPagedGenerator = async function* <S, T>(

return null;
};

/** @internal */
export const assertTypeQualifier: (qualifier: unknown) => asserts qualifier is ContactTypeQualifier = (
qualifier: unknown
) => {
if (!isContactTypeQualifier(qualifier)) {
throw new InvalidArgumentError(`Invalid contact type [${JSON.stringify(qualifier)}].`);
}
};

/** @internal */
export const assertLimit: (limit: unknown) => asserts limit is number = (limit: unknown) => {
if (typeof limit !== 'number' || !Number.isInteger(limit) || limit <= 0) {
throw new InvalidArgumentError(`The limit must be a positive number: [${String(limit)}].`);
}
};

/** @internal */
export const assertCursor: (cursor: unknown) => asserts cursor is Nullable<string> = (cursor: unknown) => {
if (cursor !== null && (typeof cursor !== 'string' || !cursor.length)) {
throw new InvalidArgumentError(`Invalid cursor token: [${String(cursor)}].`);
}
};

/** @internal */
sugat009 marked this conversation as resolved.
Show resolved Hide resolved
export const fetchAndFilter = (
sugat009 marked this conversation as resolved.
Show resolved Hide resolved
getFunction: (limit: number, skip: number) => Promise<Nullable<Doc>[]>,
filterFunction: (settings: SettingsService, doc: Nullable<Doc>, uuid: string | undefined) => unknown,
settings: SettingsService,
sugat009 marked this conversation as resolved.
Show resolved Hide resolved
limit: number,
): typeof recursionInner => {
const recursionInner = async (
currentLimit: number,
currentSkip: number,
currentDocs: Nullable<Doc>[] = [],
): Promise<Page<unknown>> => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to the wonders of generic types, we can do something like this:

Suggested change
export const fetchAndFilter = (
getFunction: (limit: number, skip: number) => Promise<Nullable<Doc>[]>,
filterFunction: (settings: SettingsService, doc: Nullable<Doc>, uuid: string | undefined) => unknown,
settings: SettingsService,
limit: number,
): typeof recursionInner => {
const recursionInner = async (
currentLimit: number,
currentSkip: number,
currentDocs: Nullable<Doc>[] = [],
): Promise<Page<unknown>> => {
export const fetchAndFilter = <T extends Doc> (
getFunction: (limit: number, skip: number) => Promise<Nullable<Doc>[]>,
filterFunction: (settings: SettingsService, doc: Nullable<Doc>, uuid: string | undefined) => doc is T,
settings: SettingsService,
limit: number,
): typeof recursionInner => {
const recursionInner = async (
currentLimit: number,
currentSkip: number,
currentDocs: T[] = [],
): Promise<Page<T>> => {

And then we do not have to have the as Page<Place.v1.Place> assertions in the person/place code! The only other required change is that we update the filter call below to be explicit about the assertion: const newDocs = docs.filter((doc): doc is T => filterFunction(settings, doc, doc?._id));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this would eliminate the use of unknown in the function, I couldn't get this to work without the as Page<Place.v1.Place> assertions because <T extends Doc> would mean they have properties from Doc but Place and Person have additional properties for which errors are thrown in other parts of the code.

const docs = await getFunction(currentLimit, currentSkip);
const noMoreResults = docs.length < currentLimit;
const newDocs = docs.filter((doc) => filterFunction(settings, doc, doc?._id));
const overFetchCount = currentDocs.length + newDocs.length - limit || 0;
const totalDocs = [...currentDocs, ...newDocs].slice(0, limit);

if (noMoreResults) {
return {data: totalDocs, cursor: null};
}

if (totalDocs.length === limit) {
const nextSkip = currentSkip + currentLimit - overFetchCount;

return {data: totalDocs, cursor: nextSkip.toString()};
}

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

return recursionInner(
nextLimit,
nextSkip,
totalDocs,
);
};
return recursionInner;
};
49 changes: 12 additions & 37 deletions shared-libs/cht-datasource/src/local/person.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Doc } from '../libs/doc';
import contactTypeUtils from '@medic/contact-types-utils';
import { deepCopy, isNonEmptyArray, Nullable, Page } from '../libs/core';
import { deepCopy, fetchAndFilter, isNonEmptyArray, Nullable, Page } from '../libs/core';
import { ContactTypeQualifier, UuidQualifier } from '../qualifier';
import * as Person from '../person';
import { getDocById, getDocsByIds, queryDocsByKey } from './libs/doc';
Expand Down Expand Up @@ -73,50 +73,25 @@ export namespace v1 {
const personTypesIds = personTypes.map((item) => item.id);

if (!personTypesIds.includes(personType.contactType)) {
throw new InvalidArgumentError(`Invalid contact type [${personType.contactType}]`);
throw new InvalidArgumentError(`Invalid contact type [${personType.contactType}].`);
}

// Adding a number skip variable here so as not to confuse ourselves
const skip = Number(cursor);
if (isNaN(skip) || skip < 0 || !Number.isInteger(skip)) {
throw new InvalidArgumentError(`Invalid cursor token: [${String(cursor)}]`);
throw new InvalidArgumentError(`Invalid cursor token: [${String(cursor)}].`);
}

const fetchAndFilter = async (
currentLimit: number,
currentSkip: number,
currentPersonDocs: Person.v1.Person[] = [],
): Promise<Page<Person.v1.Person>> => {
const docs = await getDocsByPage([personType.contactType], currentLimit, currentSkip);
const noMoreResults = docs.length < currentLimit;
const newPersonDocs = docs.filter((doc): doc is Person.v1.Person => isPerson(settings, doc, doc?._id));
const overFetchCount = currentPersonDocs.length + newPersonDocs.length - limit || 0;
const totalPeople = [...currentPersonDocs, ...newPersonDocs].slice(0, limit);
const getDocsByPageWithPersonType = (
personType: ContactTypeQualifier
) => (limit: number, skip: number) => getDocsByPage([personType.contactType], limit, skip);
sugat009 marked this conversation as resolved.
Show resolved Hide resolved

if (noMoreResults) {
return { data: totalPeople, cursor: null };
}

if (totalPeople.length === limit) {
const nextSkip = currentSkip + currentLimit - overFetchCount;

return { data: totalPeople, cursor: nextSkip.toString() };
}

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

return fetchAndFilter(
nextLimit,
nextSkip,
totalPeople,
);
};

return fetchAndFilter(limit, skip);
return await fetchAndFilter(
getDocsByPageWithPersonType(personType),
isPerson,
settings,
limit
)(limit, skip) as Page<Person.v1.Person>;
};
};
}
Loading
Loading